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

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

Created:
8 years, 2 months ago by rafaelw
Modified:
8 years, 1 month ago
CC:
v8-dev
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) Patch
M src/array.js View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
M src/bootstrapper.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M src/messages.js View 1 1 chunk +4 lines, -0 lines 2 comments Download
A src/object-observe.js View 1 2 3 4 5 6 7 8 9 1 chunk +164 lines, -0 lines 3 comments 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 Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
rafaelw
FYI. Still need to fill out TypeError messages and write tests.
8 years, 2 months ago (2012-10-23 14:54:12 UTC) #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 ...
8 years, 2 months ago (2012-10-23 15:35:56 UTC) #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] ...
8 years, 2 months ago (2012-10-23 16:18:20 UTC) #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 ...
8 years, 2 months ago (2012-10-23 16:29:44 UTC) #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 ...
8 years, 2 months ago (2012-10-23 21:21:34 UTC) #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: > ...
8 years, 2 months ago (2012-10-24 09:57:24 UTC) #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 ...
8 years, 2 months ago (2012-10-24 10:06:52 UTC) #7
rafaelw
All done. I'd like to understand when to use $Array/$Object and when to use [], ...
8 years, 2 months ago (2012-10-24 11:18:03 UTC) #8
rossberg
On 2012/10/24 11:18:03, rafaelw wrote: > I'd like to understand when to use $Array/$Object and ...
8 years, 2 months ago (2012-10-24 12:03:13 UTC) #9
Vyacheslav Egorov (Google)
Drive-by comment: $InternalArray is used to protect against (malicious) monkey patching.
8 years, 2 months ago (2012-10-24 12:11:33 UTC) #10
rossberg
On 2012/10/24 12:11:33, Vyacheslav Egorov (Google) wrote: > Drive-by comment: > > $InternalArray is used ...
8 years, 2 months ago (2012-10-24 12:43:15 UTC) #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, ...
8 years, 2 months ago (2012-10-24 14:56:05 UTC) #12
rossberg
LGTM I'll land it tomorrow.
8 years, 2 months ago (2012-10-24 16:06:42 UTC) #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 ...
8 years, 1 month ago (2012-10-25 13:22:32 UTC) #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 ...
8 years, 1 month ago (2012-10-25 14:02:19 UTC) #15
rossberg
lgtm
8 years, 1 month ago (2012-10-25 14:04:54 UTC) #16
arv (Not doing code reviews)
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", " ...
8 years, 1 month ago (2012-10-25 20:43:44 UTC) #17
rafaelw
8 years, 1 month ago (2012-10-26 05:34:14 UTC) #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.

Powered by Google App Engine
This is Rietveld 408576698