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

[raf...@chromium.org]

Reviewers: adamk, rossberg,

Message:
FYI. Still need to fill out TypeError messages and write tests.

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


Please review this at https://codereview.chromium.org/11225058/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/bootstrapper.cc
M src/flag-definitions.h
A src/object-observe.js
M tools/gyp/v8.gyp

[ad...@chromium.org]

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.

https://codereview.chromium.org/11225058/

[raf...@chromium.org]

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/

[ad...@chromium.org]

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 this is true of all MakeTypeErrors
in this file.

https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/object-observe.js#newcode28
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/object-observe.js#newcode75
test/mjsunit/harmony/object-observe.js:75: // Object.unobserve
Seems like a test for unobserve's functionality would be good too.

https://codereview.chromium.org/11225058/

[raf...@chromium.org]

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#newcode35
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/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11225058/diff/4001/test/mjsunit/harmony/object-observe.js#newcode28
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/object-observe.js#newcode75
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/

[ross...@chromium.org]

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:
> 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#newcode148
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#newcode39
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#newcode43
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#newcode52
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#newcode59
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#newcode70
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#newcode92
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#newcode100
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#newcode101
src/object-observe.js:101: if (!IS_STRING(changeRecord['type']))
Preferable: changeRecord.type

https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#newcode108
src/object-observe.js:108: var newRecord = new $Object();
{}

https://codereview.chromium.org/11225058/diff/13001/src/object-observe.js#newcode109
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#newcode109
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/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/object-observe.js#newcode30
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/object-observe.js#newcode99
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/object-observe.js#newcode140
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/

[ross...@chromium.org]

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 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).

https://codereview.chromium.org/11225058/

[raf...@chromium.org]

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#newcode35
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#newcode148
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#newcode39
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#newcode43
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#newcode52
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#newcode59
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#newcode70
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#newcode92
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#newcode100
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#newcode101
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#newcode108
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#newcode109
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/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11225058/diff/13001/test/mjsunit/harmony/object-observe.js#newcode30
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/object-observe.js#newcode99
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/object-observe.js#newcode140
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/object-observe.js#newcode140
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?

[ross...@chromium.org]

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#newcode59
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/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 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/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11225058/diff/18001/test/mjsunit/harmony/object-observe.js#newcode199
test/mjsunit/harmony/object-observe.js:199: Object.observe(obj2, obs);
Nit: is there a reason you observe 3 before 2?

https://codereview.chromium.org/11225058/

[veg...@google.com]

Drive-by comment:

$InternalArray is used to protect against (malicious) monkey patching.

https://codereview.chromium.org/11225058/

[ross...@chromium.org]

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.



https://codereview.chromium.org/11225058/

[Vyacheslav Egorov]

Yes, I think it's correct.

Good example of InternalArray usage can be seen in ArrayMap.

We create InternalArray instance to accumulate results and then
reattach collected elements from it to a normal Array to return it
outside.

--
Vyacheslav Egorov

> --
> v8-dev mailing list
> v8-...@googlegroups.com
> http://groups.google.com/group/v8-dev

[raf...@chromium.org]

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, 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/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11225058/diff/18001/test/mjsunit/harmony/object-observe.js#newcode199
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.

[ross...@chromium.org]

LGTM

I'll land it tomorrow.

https://codereview.chromium.org/11225058/

[ross...@chromium.org]

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 count an argument.

https://codereview.chromium.org/11225058/diff/26002/test/mjsunit/harmony/object-observe.js#newcode69
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/object-observe.js#newcode77
test/mjsunit/harmony/object-observe.js:77: assertEquals("function",
typeof observer.callback);
Nit: this seems a bit redundant.

https://codereview.chromium.org/11225058/

[raf...@chromium.org]

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 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/object-observe.js#newcode69
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/object-observe.js#newcode77
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.

[ross...@chromium.org]

lgtm



https://codereview.chromium.org/11225058/

[a...@chromium.org]

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#newcode38
src/object-observe.js:38: }
semicolon

https://codereview.chromium.org/11225058/diff/14016/src/object-observe.js#newcode94
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/

[raf...@chromium.org]

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#newcode94
src/object-observe.js:94: changeObservers.splice(index, 1);

Unfortunately, sets don't have iteration yet.