Implement Object.getNotifier() and remove Object.notify()

Implement Object.getNotifier() and remove Object.notify()

Updated all tests to use getNotifier or actual object mutation instead of notify, and added tests for new behavior of getNotifier.

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

[rossberg, 2012-11-09 14:41:25]

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

https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode65
src/object-observe.js:65: changeObservers: new InternalArray,
Nit: can we perhaps rename that property to just "observers"? Sounds less like a
method. :)

https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode147
src/object-observe.js:147: return;
This is calling the notify method on something that's not a notifier. Shouldn't
this better be an error?

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:121: }, TypeError);
Also:  notifier.notify.call({}, { type: 'a' })
Does nothing, but perhaps should throw? (see other comment)

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:124: assertFalse(recordCreated); // not
observed yet
Nit: 2 spaces before //

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:138: object: notifier, // object property
is ignored
dito

[adamk, 2012-11-09 14:48:41]

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

https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode65
src/object-observe.js:65: changeObservers: new InternalArray,
On 2012/11/09 14:41:25, rossberg wrote:
> Nit: can we perhaps rename that property to just "observers"? Sounds less like
a
> method. :)

It's named like this to match the spec's [[ChangeObservers]]; Rafael, ok with
you if we change this to not match?

https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode147
src/object-observe.js:147: return;
On 2012/11/09 14:41:25, rossberg wrote:
> This is calling the notify method on something that's not a notifier.
Shouldn't
> this better be an error?

Here's what arv said about this when I asked him a few months ago:
"""
No, we are trying to remove nominal typing from ES6.
In this case I have considered adding these internal properties lazily
as needed. This might be required to allow a proxy to be used as a
notifier.
"""

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:121: }, TypeError);
On 2012/11/09 14:41:25, rossberg wrote:
> Also:  notifier.notify.call({}, { type: 'a' })
> Does nothing, but perhaps should throw? (see other comment)

Added a test for this below

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:124: assertFalse(recordCreated); // not
observed yet
On 2012/11/09 14:41:25, rossberg wrote:
> Nit: 2 spaces before //

Damn, WebKit vs google3 style getting me in trouble...

https://codereview.chromium.org/11369154/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:138: object: notifier, // object property
is ignored
On 2012/11/09 14:41:25, rossberg wrote:
> dito

Done.

[rossberg, 2012-11-09 15:06:17]

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

https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode147
src/object-observe.js:147: return;
On 2012/11/09 14:48:41, adamk wrote:
> On 2012/11/09 14:41:25, rossberg wrote:
> > This is calling the notify method on something that's not a notifier.
> Shouldn't
> > this better be an error?
> 
> Here's what arv said about this when I asked him a few months ago:
> """
> No, we are trying to remove nominal typing from ES6.
> In this case I have considered adding these internal properties lazily
> as needed. This might be required to allow a proxy to be used as a
> notifier.
> """

Hm, I don't buy that -- I think "nominal typing" is a red herring here.
Logically, 'target' is best viewed as being a private property of the notifier
(you could actually implement it that way using ES6 private names, if we had
them already). From that POV, it's also natural to throw if the property doesn't
exist on 'this'.

[adamk, 2012-11-09 15:10:20]

On 2012/11/09 15:06:17, rossberg wrote:
> https://codereview.chromium.org/11369154/diff/1/src/object-observe.js
> File src/object-observe.js (right):
> 
>
https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode147
> src/object-observe.js:147: return;
> On 2012/11/09 14:48:41, adamk wrote:
> > On 2012/11/09 14:41:25, rossberg wrote:
> > > This is calling the notify method on something that's not a notifier.
> > Shouldn't
> > > this better be an error?
> > 
> > Here's what arv said about this when I asked him a few months ago:
> > """
> > No, we are trying to remove nominal typing from ES6.
> > In this case I have considered adding these internal properties lazily
> > as needed. This might be required to allow a proxy to be used as a
> > notifier.
> > """
> 
> Hm, I don't buy that -- I think "nominal typing" is a red herring here.
> Logically, 'target' is best viewed as being a private property of the notifier
> (you could actually implement it that way using ES6 private names, if we had
> them already). From that POV, it's also natural to throw if the property
doesn't
> exist on 'this'.

Makes sense to me: calling notify is _never_ going to do anything if called on a
non-Notifier (even a proxy of a Notifier will fail). So I'm not sure the point
of hiding the error by not throwing. Arv?

[arv (Not doing code reviews), 2012-11-09 15:22:22]

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

https://codereview.chromium.org/11369154/diff/1/src/object-observe.js#newcode147
src/object-observe.js:147: return;
On 2012/11/09 15:06:17, rossberg wrote:
> On 2012/11/09 14:48:41, adamk wrote:
> > On 2012/11/09 14:41:25, rossberg wrote:
> > > This is calling the notify method on something that's not a notifier.
> > Shouldn't
> > > this better be an error?
> > 
> > Here's what arv said about this when I asked him a few months ago:
> > """
> > No, we are trying to remove nominal typing from ES6.
> > In this case I have considered adding these internal properties lazily
> > as needed. This might be required to allow a proxy to be used as a
> > notifier.
> > """

I don't remember my logic.

> 
> Hm, I don't buy that -- I think "nominal typing" is a red herring here.
> Logically, 'target' is best viewed as being a private property of the notifier
> (you could actually implement it that way using ES6 private names, if we had
> them already). From that POV, it's also natural to throw if the property
doesn't
> exist on 'this'.

I agree. This is similar to how Map and Set are specified. If the internal
property does not exist it should throw.

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

https://codereview.chromium.org/11369154/diff/1004/src/object-observe.js#newc...
src/object-observe.js:235: InstallFunctions(notifierPrototype, DONT_ENUM,
$Array(
Andreas, why are these using $Array(...) instead of [...]?

[adamk, 2012-11-09 15:46:16]

Okay, I've implemented throwing if passed a non-notifier. And also better match
the spec by throwing earlier if passed a non-object (rather than just checking
for undefined and null).

[rossberg, 2012-11-09 16:02:27]

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

https://codereview.chromium.org/11369154/diff/1004/src/object-observe.js#newc...
src/object-observe.js:235: InstallFunctions(notifierPrototype, DONT_ENUM,
$Array(
On 2012/11/09 15:22:22, arv wrote:
> Andreas, why are these using $Array(...) instead of [...]?

I'd use literals, but it doesn't really matter. Adam, your call. :)

[adamk, 2012-11-09 16:04:17]

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

https://codereview.chromium.org/11369154/diff/1004/src/object-observe.js#newc...
src/object-observe.js:235: InstallFunctions(notifierPrototype, DONT_ENUM,
$Array(
On 2012/11/09 16:02:27, rossberg wrote:
> On 2012/11/09 15:22:22, arv wrote:
> > Andreas, why are these using $Array(...) instead of [...]?
> 
> I'd use literals, but it doesn't really matter. Adam, your call. :)

I'd like to leave these as-is and do a cleanup of all the .js files in this
directory, which all use $Array when calling InstallFunctions during bootstrap.
Agreed that literals would be nicer but I'm going for consistency here.

[rossberg, 2012-11-09 16:06:54]

lgtm