Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(205)

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

Created:
8 years, 2 months ago by adamk
Modified:
8 years, 1 month ago
Reviewers:
rossberg, rafaelw
CC:
v8-dev
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) Patch
M src/bootstrapper.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M src/contexts.h View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments Download
M src/isolate.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M src/object-observe.js View 1 2 3 4 5 6 7 8 9 5 chunks +26 lines, -11 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 1 chunk +14 lines, -0 lines 0 comments Download
M src/runtime.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M src/v8.cc View 1 2 3 4 5 6 7 8 9 2 chunks +12 lines, -3 lines 0 comments 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 Download

Messages

Total messages: 9 (0 generated)
adamk
Initial delivery logic, including end-of-microtask delivery (and a test for the latter). Still needs tests ...
8 years, 1 month ago (2012-10-25 13:28:20 UTC) #1
adamk
Heads up that this is now updated with more tests. I'd be interested in feedback ...
8 years, 1 month ago (2012-11-05 13:03:30 UTC) #2
adamk
Note that this is ready for a look, though it does depend on https://codereview.chromium.org/11274014/ (and ...
8 years, 1 month ago (2012-11-06 10:30:11 UTC) #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 ...
8 years, 1 month ago (2012-11-06 16:45:57 UTC) #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 ...
8 years, 1 month ago (2012-11-06 17:00:09 UTC) #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 ...
8 years, 1 month ago (2012-11-06 17:04:41 UTC) #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 ...
8 years, 1 month ago (2012-11-06 17:25:20 UTC) #7
adamk
I think this change is all ready for final review/landing as well.
8 years, 1 month ago (2012-11-08 09:59:14 UTC) #8
rossberg
8 years, 1 month ago (2012-11-08 13:07:51 UTC) #9
lgtm

Powered by Google App Engine
This is Rietveld 408576698