Re: Delivery logic for Object.observe (issue 11266011)

[ad...@chromium.org]

Note that this is ready for a look, though it does depend on
https://codereview.chromium.org/11274014/ (and the diff is against that
patch,
not trunk)

https://codereview.chromium.org/11266011/

[ross...@chromium.org]

https://codereview.chromium.org/11266011/diff/16001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/11266011/diff/16001/src/contexts.h#newcode297
src/contexts.h:297: DELIVER_CHANGE_RECORDS_INDEX,
Is this perhaps some orphan duplicate of the above?

https://codereview.chromium.org/11266011/diff/16001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/11266011/diff/16001/src/isolate.h#newcode357
src/isolate.h:357: V(bool, has_active_object_observers, false)
\
Maybe put a comment here explaining when this is true (the name is
slightly misleading).

https://codereview.chromium.org/11266011/diff/16001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11266011/diff/16001/src/object-observe.js#newcode116
src/object-observe.js:116: var pendingChangeRecords =
observerInfo.pendingChangeRecords;
What is this variable used for?

https://codereview.chromium.org/11266011/diff/16001/src/object-observe.js#newcode118
src/object-observe.js:118: pendingChangeRecords =
observerInfo.pendingChangeRecords = new InternalArray(changeRecord);
Line length

https://codereview.chromium.org/11266011/diff/16001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11266011/diff/16001/src/objects.cc#newcode1746
src/objects.cc:1746: isolate->set_has_active_object_observers(false);
This should probably be go to the end.

https://codereview.chromium.org/11266011/diff/16001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/11266011/diff/16001/src/v8.cc#newcode228
src/v8.cc:228: if (has_active_object_observers) {
FLAG_harmony_observation && ...

https://codereview.chromium.org/11266011/

[ad...@chromium.org]

https://codereview.chromium.org/11266011/diff/16001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/11266011/diff/16001/src/contexts.h#newcode297
src/contexts.h:297: DELIVER_CHANGE_RECORDS_INDEX,

On 2012/11/06 16:45:57, rossberg wrote:
> Is this perhaps some orphan duplicate of the above?

Yeah, merge fail. Removed.

https://codereview.chromium.org/11266011/diff/16001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/11266011/diff/16001/src/isolate.h#newcode357
src/isolate.h:357: V(bool, has_active_object_observers, false)
\

On 2012/11/06 16:45:57, rossberg wrote:
> Maybe put a comment here explaining when this is true (the name is
slightly
> misleading).

Maybe a rename is in order? How about "observer_delivery_pending"
(object_observer_delivery_pending?)?

https://codereview.chromium.org/11266011/diff/16001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/11266011/diff/16001/src/object-observe.js#newcode116
src/object-observe.js:116: var pendingChangeRecords =
observerInfo.pendingChangeRecords;

On 2012/11/06 16:45:57, rossberg wrote:
> What is this variable used for?

Good question...removed.

https://codereview.chromium.org/11266011/diff/16001/src/object-observe.js#newcode118
src/object-observe.js:118: pendingChangeRecords =
observerInfo.pendingChangeRecords = new InternalArray(changeRecord);

On 2012/11/06 16:45:57, rossberg wrote:
> Line length

Done.

https://codereview.chromium.org/11266011/diff/16001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11266011/diff/16001/src/objects.cc#newcode1746
src/objects.cc:1746: isolate->set_has_active_object_observers(false);

On 2012/11/06 16:45:57, rossberg wrote:
> This should probably be go to the end.

Done.

https://codereview.chromium.org/11266011/diff/16001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/11266011/diff/16001/src/v8.cc#newcode228
src/v8.cc:228: if (has_active_object_observers) {

On 2012/11/06 16:45:57, rossberg wrote:
> FLAG_harmony_observation && ...

Done.

https://codereview.chromium.org/11266011/

[raf...@chromium.org]

lgtm

https://codereview.chromium.org/11266011/diff/16001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/11266011/diff/16001/src/contexts.h#newcode297
src/contexts.h:297: DELIVER_CHANGE_RECORDS_INDEX,

don't need this any more, right?

https://codereview.chromium.org/11266011/diff/16001/test/cctest/test-object-observe.cc
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/11266011/diff/16001/test/cctest/test-object-observe.cc#newcode154
test/cctest/test-object-observe.cc:154: "function observer3() {
ordering.push(3); };"
maybe a comment noting that observer3 gets delivered 'a' and 'b' in one
callback.

https://codereview.chromium.org/11266011/

[ad...@chromium.org]

I've renamed the Isolate member.

On 2012/11/06 17:04:41, rafaelw wrote:
> lgtm

> https://codereview.chromium.org/11266011/diff/16001/src/contexts.h
> File src/contexts.h (right):

> https://codereview.chromium.org/11266011/diff/16001/src/contexts.h#newcode297
> src/contexts.h:297: DELIVER_CHANGE_RECORDS_INDEX,
> don't need this any more, right?

Yeah, already gone.

https://codereview.chromium.org/11266011/diff/16001/test/cctest/test-object-observe.cc
> File test/cctest/test-object-observe.cc (right):


https://codereview.chromium.org/11266011/diff/16001/test/cctest/test-object-observe.cc#newcode154
> test/cctest/test-object-observe.cc:154: "function observer3() {
> ordering.push(3); };"
> maybe a comment noting that observer3 gets delivered 'a' and 'b' in one
> callback.

Added a comment.

https://codereview.chromium.org/11266011/