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

Issue 11225058: Initial JS stub implementation of Object.observe. Adds support for .object/.unobserve/.notify/.deli… (Closed)

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

Description

Initial JS stub implementation of Object.observe. Adds support for .object/.unobserve/.notify/.deliverChangeRecords. No delivery mechanism is implemented for end-of-microtask.

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

Patch Set 1 #

Total comments: 24

Patch Set 2 : cr changes, added tests, renamed flag #

Total comments: 8

Patch Set 3 : moar tests #

Patch Set 4 : cr changes #

Total comments: 34

Patch Set 5 : cr changes #

Total comments: 2

Patch Set 6 : cr changes including protection from monkeypatching #

Patch Set 7 : cleanup #

Patch Set 8 : js test cleanup #

Total comments: 6

Patch Set 9 : more cr changes #

Patch Set 10 : install functions on Object, not Object.prototype #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+422 lines, -1 line) Lint Patch
M src/array.js View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments ? errors Download
M src/bootstrapper.cc View 1 1 chunk +5 lines, -0 lines 0 comments ? errors Download
M src/flag-definitions.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments 3 errors Download
M src/messages.js View 1 1 chunk +4 lines, -0 lines 2 comments ? errors Download
A src/object-observe.js View 1 2 3 4 5 6 7 8 9 1 chunk +164 lines, -0 lines 3 comments ? errors Download
A test/mjsunit/harmony/object-observe.js View 1 2 3 4 5 6 7 8 1 chunk +241 lines, -0 lines 0 comments ? errors Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 18
rafaelw
FYI. Still need to fill out TypeError messages and write tests.
1 year, 5 months ago #1
adamk
https://codereview.chromium.org/11225058/diff/1/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode47 src/object-observe.js:47: objectInfo.changeObservers[0] = callback; Even if we need $Array to ...
1 year, 5 months ago #2
rafaelw
done. also, added TypeError messages and initial tests. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode47 src/object-observe.js:47: objectInfo.changeObservers[0] ...
1 year, 5 months ago #3
adamk
https://codereview.chromium.org/11225058/diff/4001/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11225058/diff/4001/src/object-observe.js#newcode35 src/object-observe.js:35: throw MakeTypeError("observe_non_object", ["observe"]) Nit: need trailing semicolon. I think ...
1 year, 5 months ago #4
rafaelw
ok. done. i think it's ready for andreas to have a look at. https://codereview.chromium.org/11225058/diff/4001/src/object-observe.js File ...
1 year, 5 months ago #5
rossberg
https://codereview.chromium.org/11225058/diff/4001/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11225058/diff/4001/src/object-observe.js#newcode35 src/object-observe.js:35: throw MakeTypeError("observe_non_object", ["observe"]) On 2012/10/23 21:21:35, rafaelw wrote: > ...
1 year, 5 months ago #6
rossberg
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/object-observe.js File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/object-observe.js#newcode140 test/mjsunit/harmony/object-observe.js:140: })() On 2012/10/24 09:57:24, rossberg wrote: > Other scenarios ...
1 year, 5 months ago #7
rafaelw
All done. I'd like to understand when to use $Array/$Object and when to use [], ...
1 year, 5 months ago #8
rossberg
On 2012/10/24 11:18:03, rafaelw wrote: > I'd like to understand when to use $Array/$Object and ...
1 year, 5 months ago #9
Vyacheslav Egorov (Google)
Drive-by comment: $InternalArray is used to protect against (malicious) monkey patching.
1 year, 5 months ago #10
rossberg
On 2012/10/24 12:11:33, Vyacheslav Egorov (Google) wrote: > Drive-by comment: > > $InternalArray is used ...
1 year, 5 months ago #11
rafaelw
all done https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js File src/object-observe.js (right): https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#newcode59 src/object-observe.js:59: if (changeObservers.indexOf(callback) >= 0) On 2012/10/24 12:03:13, ...
1 year, 5 months ago #12
rossberg
LGTM I'll land it tomorrow.
1 year, 5 months ago #13
rossberg
A couple of nits. https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/object-observe.js File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/object-observe.js#newcode48 test/mjsunit/harmony/object-observe.js:48: assertEquals(1, this.callbackCount); Suggestion: make the ...
1 year, 5 months ago #14
rafaelw
https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/object-observe.js File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/object-observe.js#newcode48 test/mjsunit/harmony/object-observe.js:48: assertEquals(1, this.callbackCount); I don't think we'll ever want to ...
1 year, 5 months ago #15
rossberg
lgtm
1 year, 5 months ago #16
arv
FYI https://codereview.chromium.org/11225058/diff/14016/src/messages.js File src/messages.js (right): https://codereview.chromium.org/11225058/diff/14016/src/messages.js#newcode206 src/messages.js:206: "observe_non_object", ["Object.", "%0", " cannot ", "%0", " ...
1 year, 5 months ago #17
rafaelw
1 year, 5 months ago #18
https://codereview.chromium.org/11225058/diff/14016/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/11225058/diff/14016/src/messages.js#newcode206
src/messages.js:206: "observe_non_object",           ["Object.", "%0", " cannot
", "%0", " non-object"],
This gets concat with the %0 value, e.g "Object.observe". The '.' is needed.

On 2012/10/25 20:43:44, arv wrote:
> Should this be "Object " instead of "Object.". Same on next line?

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

https://codereview.chromium.org/11225058/diff/14016/src/object-observe.js#new...
src/object-observe.js:94: changeObservers.splice(index, 1);
Unfortunately, sets don't have iteration yet.

On 2012/10/25 20:43:44, arv wrote:
> This can be a Set. Sets are ordered and we will get an O(1) removal.
Sign in to reply to this message.

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