Chromium Code Reviews
Help | Chromium Project | Sign in
(66)

Issue 11266011: Delivery logic for Object.observe (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by adamk
Modified:
1 year, 5 months ago
Reviewers:
rossberg, rafaelw
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

Delivery logic for Object.observe

This CL has two parts: the first is the logic itself, whereby each observer callback is assigned
a "priority" number the first time it's passed as an observer to Object.observe(), and that
priority is used to determine the order of delivery.

The second part invokes the above logic as part of the API, when the JS stack winds down to
zero.

Added several tests via the API, as the delivery logic isn't testable from a JS test
(it runs after such a test would exit).

Committed: https://code.google.com/p/v8/source/detail?r=12902

Patch Set 1 #

Patch Set 2 : Merged with per-isolate #

Patch Set 3 : Added end-of-microtask delivery and test #

Patch Set 4 : Remove unneeded testing hook #

Patch Set 5 : Merged #

Patch Set 6 : More tests #

Patch Set 7 : Rebased against newest per-isolate patch #

Total comments: 14

Patch Set 8 : Move clearing of has_active_observers to the end of DeliverChangeRecords #

Patch Set 9 : Responded to rossberg comments #

Patch Set 10 : Renamed and added comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+156 lines, -19 lines) Lint Patch
M src/bootstrapper.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments 0 errors Download
M src/contexts.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments ? errors Download
M src/isolate.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments ? errors Download
M src/object-observe.js View 1 2 3 4 5 6 7 8 9 5 chunks +26 lines, -11 lines 0 comments ? errors Download
M src/objects.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments ? errors Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 1 chunk +14 lines, -0 lines 0 comments ? errors Download
M src/runtime.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments 0 errors Download
M src/v8.cc View 1 2 3 4 5 6 7 8 9 2 chunks +12 lines, -3 lines 0 comments 1 errors Download
M test/cctest/test-object-observe.cc View 1 2 3 4 5 6 7 8 9 1 chunk +88 lines, -5 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 9
adamk
Initial delivery logic, including end-of-microtask delivery (and a test for the latter). Still needs tests ...
1 year, 5 months ago #1
adamk
Heads up that this is now updated with more tests. I'd be interested in feedback ...
1 year, 5 months ago #2
adamk
Note that this is ready for a look, though it does depend on https://codereview.chromium.org/11274014/ (and ...
1 year, 5 months ago #3
rossberg
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 ...
1 year, 5 months ago #4
adamk
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 ...
1 year, 5 months ago #5
rafaelw
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 ...
1 year, 5 months ago #6
adamk
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 ...
1 year, 5 months ago #7
adamk
I think this change is all ready for final review/landing as well.
1 year, 5 months ago #8
rossberg
1 year, 5 months ago #9
lgtm
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6