Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionInitial 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
Messages
Total messages: 18 (0 generated)
FYI. Still need to fill out TypeError messages and write 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] = callback; Even if we need $Array to avoid array literals here, this can be one line: objectInfo.changeObservers = $Array(callback); https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode51 src/object-observe.js:51: observerInfo = new $Object(); This doesn't seem right, here you want to see if observerInfo IS_UNDEFINED otherwise you could overwrite an existing observerInfo. And actually, there's no reason for this whole thing to be inside this if statement, it should happen before any objectInfo initialization (to let the rest of this block return early). https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode59 src/object-observe.js:59: for (var i = 0; i < changeObservers.length; i++) { Can you use changeObservers.indexOf() instead of this for loop? https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode72 src/object-observe.js:72: var observerInfo = observerInfoMap.get(callback); Can move this hash lookup after the undefined check below to avoid unnecessary lookups. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode77 src/object-observe.js:77: for (var i = 0; i < changeObservers.length; i++) { Same question re: indexOf https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode94 src/object-observe.js:94: pendingChangeRecords = observerInfo.pendingChangeRecords = new $Array(1); Again, $Array(changeRecord) should suffice if literals are out. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode104 src/object-observe.js:104: var type = changeRecord['type']; Nit: no need to copy this into a local, methinkgs: if (!IS_STRING(changeRecord['type'])) seems clear enough to me. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode118 src/object-observe.js:118: if (IS_UNDEFINED(objectInfo)) Probably want to move this before the creation of the record? That's both more efficient and matches the spec. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode129 src/object-observe.js:129: if (!observerInfo) Should be if (IS_UNDEFINED()), I think? https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode130 src/object-observe.js:130: return false; Should be just return, per spec: Note: The user facing function Object.deliverChangeRecords returns undefined to prevent detection if anything was delivered or not. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode134 src/object-observe.js:134: return false; Ditto, should return undefined. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode139 src/object-observe.js:139: return true; ditto, should return undefined.
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] = callback; On 2012/10/23 15:35:57, adamk wrote: > Even if we need $Array to avoid array literals here, this can be one line: > > objectInfo.changeObservers = $Array(callback); Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode51 src/object-observe.js:51: observerInfo = new $Object(); Good catch. For some reason I was thinking these always get initialized together -- which of course is wrong. Jet lag ;-(. On 2012/10/23 15:35:57, adamk wrote: > This doesn't seem right, here you want to see if observerInfo IS_UNDEFINED > otherwise you could overwrite an existing observerInfo. And actually, there's no > reason for this whole thing to be inside this if statement, it should happen > before any objectInfo initialization (to let the rest of this block return > early). https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode59 src/object-observe.js:59: for (var i = 0; i < changeObservers.length; i++) { On 2012/10/23 15:35:57, adamk wrote: > Can you use changeObservers.indexOf() instead of this for loop? Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode72 src/object-observe.js:72: var observerInfo = observerInfoMap.get(callback); On 2012/10/23 15:35:57, adamk wrote: > Can move this hash lookup after the undefined check below to avoid unnecessary > lookups. Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode77 src/object-observe.js:77: for (var i = 0; i < changeObservers.length; i++) { On 2012/10/23 15:35:57, adamk wrote: > Same question re: indexOf Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode94 src/object-observe.js:94: pendingChangeRecords = observerInfo.pendingChangeRecords = new $Array(1); On 2012/10/23 15:35:57, adamk wrote: > Again, $Array(changeRecord) should suffice if literals are out. Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode104 src/object-observe.js:104: var type = changeRecord['type']; On 2012/10/23 15:35:57, adamk wrote: > Nit: no need to copy this into a local, methinkgs: > > if (!IS_STRING(changeRecord['type'])) > > seems clear enough to me. Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode118 src/object-observe.js:118: if (IS_UNDEFINED(objectInfo)) Done. Also updated spec to clarify this behavior On 2012/10/23 15:35:57, adamk wrote: > Probably want to move this before the creation of the record? That's both more > efficient and matches the spec. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode129 src/object-observe.js:129: if (!observerInfo) On 2012/10/23 15:35:57, adamk wrote: > Should be if (IS_UNDEFINED()), I think? Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode130 src/object-observe.js:130: return false; On 2012/10/23 15:35:57, adamk wrote: > Should be just return, per spec: > > Note: The user facing function Object.deliverChangeRecords returns undefined to > prevent detection if anything was delivered or not. Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode134 src/object-observe.js:134: return false; On 2012/10/23 15:35:57, adamk wrote: > Ditto, should return undefined. Done. https://codereview.chromium.org/11225058/diff/1/src/object-observe.js#newcode139 src/object-observe.js:139: return true; On 2012/10/23 15:35:57, adamk wrote: > ditto, should return undefined. Done.
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#newc... src/object-observe.js:35: throw MakeTypeError("observe_non_object", ["observe"]) Nit: need trailing semicolon. I think this is true of all MakeTypeErrors in this file. https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/objec... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:28: // Copyright 2011 the V8 project authors. All rights reserved. Woops, duplicated license https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:75: // Object.unobserve Seems like a test for unobserve's functionality would be good too.
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 src/object-observe.js (right): https://codereview.chromium.org/11225058/diff/4001/src/object-observe.js#newc... src/object-observe.js:35: throw MakeTypeError("observe_non_object", ["observe"]) On 2012/10/23 16:29:44, adamk wrote: > Nit: need trailing semicolon. I think this is true of all MakeTypeErrors in this > file. Done. Although proxy.js doesn't have hardly *any* semicolons. might be a style issue. andreas? https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/objec... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:28: // Copyright 2011 the V8 project authors. All rights reserved. On 2012/10/23 16:29:44, adamk wrote: > Woops, duplicated license Done. https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/objec... test/mjsunit/harmony/object-observe.js:75: // Object.unobserve On 2012/10/23 16:29:44, adamk wrote: > Seems like a test for unobserve's functionality would be good too. Done.
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#newc... src/object-observe.js:35: throw MakeTypeError("observe_non_object", ["observe"]) On 2012/10/23 21:21:35, rafaelw wrote: > On 2012/10/23 16:29:44, adamk wrote: > > Nit: need trailing semicolon. I think this is true of all MakeTypeErrors in > this > > file. > > Done. Although proxy.js doesn't have hardly *any* semicolons. might be a style > issue. andreas? I don't care either way, just be consistent within a single file (unless you are testing parsing in particular :) ). https://codereview.chromium.org/11225058/diff/13001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/11225058/diff/13001/src/flag-definitions.h#ne... src/flag-definitions.h:148: "enable harmony object observation") Nit: add "(implies harmony collections)" 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#new... src/object-observe.js:39: throw MakeTypeError("observe_callback_frozen"); I wonder, is this actually necessary? Why does the proposal require it? It seems useful to be able to use a frozen function as observer. Would it be a side channel? https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:43: observerInfo = new $Object(); Nit: {} is clearer and potentially more efficient (same below). https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:52: objectInfo.changeObservers = new $Array(callback); Nit: [callback] instead of invoking Array constructor. https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:59: if (changeObservers.indexOf(callback) >= 0) Why not use a Set for this? https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:70: if (IS_UNDEFINED(objectInfo)) OT: Is there a reason why the spec does not make this in error? https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:92: pendingChangeRecords = observerInfo.pendingChangeRecords = new $Array(changeRecord); Line too long. https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:100: // TODO: notifier needs to be [[THIS]] Check that this is an object. https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:101: if (!IS_STRING(changeRecord['type'])) Preferable: changeRecord.type https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:108: var newRecord = new $Object(); {} https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:109: newRecord['object'] = object; // TODO: Needs to be 'object' retreived from notifier newRecord.object https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new... src/object-observe.js:109: newRecord['object'] = object; // TODO: Needs to be 'object' retreived from notifier Line too long https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:30: (function() { Nit: We don't usually turn test cases into iffies, since they are all executed in a fresh environment anyway. https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:99: assertEquals(1, callbackCount); Better reset callback count and test for 0. Otherwise it gets messy when you refactor tests (similarly below). https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:140: })() Other scenarios to test: - One function observing multiple objects, and check that change records arrive in order. - An object being modified, unobserved, modified, reobserved, modified, all before delivering change records, and check that intermediate changes don't appear.
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:140: })() On 2012/10/24 09:57:24, rossberg wrote: > Other scenarios to test: > > - One function observing multiple objects, and check that change records arrive > in order. > > - An object being modified, unobserved, modified, reobserved, modified, all > before delivering change records, and check that intermediate changes don't > appear. - Objects observed by multiple observers (perhaps more interesting once you have implemented priorities).
All done.
I'd like to understand when to use $Array/$Object and when to use [], {}. I was
under the impression that the former were used to protect from monkey-patching.
Let's talk about that at GVC.
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#newc...
src/object-observe.js:35: throw MakeTypeError("observe_non_object", ["observe"])
Ok. Going "with semicolons" ;-)
On 2012/10/24 09:57:24, rossberg wrote:
> On 2012/10/23 21:21:35, rafaelw wrote:
> > On 2012/10/23 16:29:44, adamk wrote:
> > > Nit: need trailing semicolon. I think this is true of all MakeTypeErrors
in
> > this
> > > file.
> >
> > Done. Although proxy.js doesn't have hardly *any* semicolons. might be a
style
> > issue. andreas?
>
> I don't care either way, just be consistent within a single file (unless you
are
> testing parsing in particular :) ).
https://codereview.chromium.org/11225058/diff/13001/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/11225058/diff/13001/src/flag-definitions.h#ne...
src/flag-definitions.h:148: "enable harmony object observation")
On 2012/10/24 09:57:24, rossberg wrote:
> Nit: add "(implies harmony collections)"
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#new...
src/object-observe.js:39: throw MakeTypeError("observe_callback_frozen");
Sadly, yes. This was necessary to prevent a side-channel.
On 2012/10/24 09:57:24, rossberg wrote:
> I wonder, is this actually necessary? Why does the proposal require it? It
seems
> useful to be able to use a frozen function as observer. Would it be a side
> channel?
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:43: observerInfo = new $Object();
On 2012/10/24 09:57:24, rossberg wrote:
> Nit: {} is clearer and potentially more efficient (same below).
Done.
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:52: objectInfo.changeObservers = new $Array(callback);
On 2012/10/24 09:57:24, rossberg wrote:
> Nit: [callback] instead of invoking Array constructor.
Done.
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:59: if (changeObservers.indexOf(callback) >= 0)
AFAIC, The V8 implementation for Map & Set isn't complete. There's no way yet to
iterate over the keys of either.
On 2012/10/24 09:57:24, rossberg wrote:
> Why not use a Set for this?
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:70: if (IS_UNDEFINED(objectInfo))
You mean: unobserving something that isn't observed? Probably to be consistent
with existing listener patterns (e.g. DOM Event Listeners, which also silently
succeed even if the call had no effect).
On 2012/10/24 09:57:24, rossberg wrote:
> OT: Is there a reason why the spec does not make this in error?
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:92: pendingChangeRecords =
observerInfo.pendingChangeRecords = new $Array(changeRecord);
On 2012/10/24 09:57:24, rossberg wrote:
> Line too long.
Done.
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:100: // TODO: notifier needs to be [[THIS]]
I asked Arv about this in the spec. He pointed out that its unnecessary. E.g.
Number.prototype.type = 'updated';
var foo = 5;
foo.type; // 'updated';
On 2012/10/24 09:57:24, rossberg wrote:
> Check that this is an object.
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:101: if (!IS_STRING(changeRecord['type']))
On 2012/10/24 09:57:24, rossberg wrote:
> Preferable: changeRecord.type
Done.
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:108: var newRecord = new $Object();
On 2012/10/24 09:57:24, rossberg wrote:
> {}
Done.
https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#new...
src/object-observe.js:109: newRecord['object'] = object; // TODO: Needs to be
'object' retreived from notifier
On 2012/10/24 09:57:24, rossberg wrote:
> newRecord.object
Done.
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:30: (function() {
On 2012/10/24 09:57:24, rossberg wrote:
> Nit: We don't usually turn test cases into iffies, since they are all executed
> in a fresh environment anyway.
Done.
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:99: assertEquals(1, callbackCount);
On 2012/10/24 09:57:24, rossberg wrote:
> Better reset callback count and test for 0. Otherwise it gets messy when you
> refactor tests (similarly below).
Done.
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:140: })()
On 2012/10/24 09:57:24, rossberg wrote:
> Other scenarios to test:
>
> - One function observing multiple objects, and check that change records
arrive
> in order.
>
> - An object being modified, unobserved, modified, reobserved, modified, all
> before delivering change records, and check that intermediate changes don't
> appear.
Done.
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:140: })()
Yeah. I think I'd like to add that in the next patch which will implement
prioritized delivery. Cool?
On 2012/10/24 10:06:52, rossberg wrote:
> On 2012/10/24 09:57:24, rossberg wrote:
> > Other scenarios to test:
> >
> > - One function observing multiple objects, and check that change records
> arrive
> > in order.
> >
> > - An object being modified, unobserved, modified, reobserved, modified, all
> > before delivering change records, and check that intermediate changes don't
> > appear.
>
> - Objects observed by multiple observers (perhaps more interesting once you
have
> implemented priorities).
On 2012/10/24 11:18:03, rafaelw wrote:
> I'd like to understand when to use $Array/$Object and when to use [], {}. I
was
> under the impression that the former were used to protect from
monkey-patching.
Fortunately, the semantics of literals is immune against monkey patching -- they
always use the "original" Object/Array/Function.
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#new...
src/object-observe.js:59: if (changeObservers.indexOf(callback) >= 0)
On 2012/10/24 11:18:04, rafaelw wrote:
> AFAIC, The V8 implementation for Map & Set isn't complete. There's no way yet
to
> iterate over the keys of either.
>
> On 2012/10/24 09:57:24, rossberg wrote:
> > Why not use a Set for this?
>
Ah, you are right of course. Maybe put in a TODO then.
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):
https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:140: })()
On 2012/10/24 11:18:04, rafaelw wrote:
> Yeah. I think I'd like to add that in the next patch which will implement
> prioritized delivery. Cool?
Sounds good.
https://codereview.chromium.org/11225058/diff/18001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):
https://codereview.chromium.org/11225058/diff/18001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:199: Object.observe(obj2, obs);
Nit: is there a reason you observe 3 before 2?
Drive-by comment: $InternalArray is used to protect against (malicious) monkey patching.
On 2012/10/24 12:11:33, Vyacheslav Egorov (Google) wrote: > Drive-by comment: > > $InternalArray is used to protect against (malicious) monkey patching. To clarify (please somebody correct me if I'm wrong): $Array refers to the original Array object, protecting against somebody reassigning global_object.Array. If somebody patches the Array object itself, however, that is equally visible in $Array. $InternalArray refers to a completely separate instance of the Array object that user code has no access to. You can use its methods if you you want to protect against monkey patching, but you should never create instances of it and pass them to user code (because then user code _does_ get access to $InternalArray via .constructor and can mess with it; also, instanceof would not work as expected). Literals essentially refer to the $Array and friends versions, as required per the spec. So you should always be able to use literals instead of $Array etc.
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#new... src/object-observe.js:59: if (changeObservers.indexOf(callback) >= 0) On 2012/10/24 12:03:13, rossberg wrote: > On 2012/10/24 11:18:04, rafaelw wrote: > > AFAIC, The V8 implementation for Map & Set isn't complete. There's no way yet > to > > iterate over the keys of either. > > > > On 2012/10/24 09:57:24, rossberg wrote: > > > Why not use a Set for this? > > > > Ah, you are right of course. Maybe put in a TODO then. Done. https://codereview.chromium.org/11225058/diff/18001/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/18001/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:199: Object.observe(obj2, obs); Because the naive implementation might have ordered changeRecord sequence by observation sequence. I wanted to assert that that wasn't happening. On 2012/10/24 12:03:13, rossberg wrote: > Nit: is there a reason you observe 3 before 2?
LGTM I'll land it tomorrow.
A couple of nits. https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:48: assertEquals(1, this.callbackCount); Suggestion: make the count an argument. https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:69: }.bind(observer); Perhaps not bind to the observer, but rather assert that this===undefined inside. https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:77: assertEquals("function", typeof observer.callback); Nit: this seems a bit redundant.
https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... File test/mjsunit/harmony/object-observe.js (right): https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:48: assertEquals(1, this.callbackCount); I don't think we'll ever want to have been called more than once. On 2012/10/25 13:22:32, rossberg wrote: > Suggestion: make the count an argument. https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:69: }.bind(observer); On 2012/10/25 13:22:32, rossberg wrote: > Perhaps not bind to the observer, but rather assert that this===undefined > inside. Done. https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/obje... test/mjsunit/harmony/object-observe.js:77: assertEquals("function", typeof observer.callback); Good catch. That was throw away code I used to track down a typo. On 2012/10/25 13:22:32, rossberg wrote: > Nit: this seems a bit redundant.
lgtm
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", " non-object"], 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:38: } semicolon https://codereview.chromium.org/11225058/diff/14016/src/object-observe.js#new... src/object-observe.js:94: changeObservers.splice(index, 1); This can be a Set. Sets are ordered and we will get an O(1) removal.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
