Implement Object.getNotifier(obj).performChange()

Implement Object.getNotifier(obj).performChange()

R=rossberg,adamk,arv
BUG=

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

[arv (Not doing code reviews), 2013-05-09 14:41:44]

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

https://codereview.chromium.org/14779011/diff/1/src/object-observe.js#newcode70
src/object-observe.js:70: performing: { __proto__: null },
Google3 JS style is {key: value} (no whitespace inside {}) but stick to the v8
style.

https://codereview.chromium.org/14779011/diff/1/src/object-observe.js#newcode128
src/object-observe.js:128: from.splice(count, 1);
splice is O(n). Usually it is better to put some kind of sentinel in the array
and then filter out the sentinels after the loop. That makes it O(n) instead of
O(n^2).

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

https://codereview.chromium.org/14779011/diff/1/test/mjsunit/harmony/object-o...
test/mjsunit/harmony/object-observe.js:473: observer2.assertCallbackRecords([
Awesome test. Makes it really clear what the intent is.

[rafaelw, 2013-05-09 15:33:31]

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

https://codereview.chromium.org/14779011/diff/1/src/object-observe.js#newcode70
src/object-observe.js:70: performing: { __proto__: null },
this appears to be v8 style. leaving

On 2013/05/09 14:41:44, arv wrote:
> Google3 JS style is {key: value} (no whitespace inside {}) but stick to the v8
> style.

https://codereview.chromium.org/14779011/diff/1/src/object-observe.js#newcode128
src/object-observe.js:128: from.splice(count, 1);
On 2013/05/09 14:41:44, arv wrote:
> splice is O(n). Usually it is better to put some kind of sentinel in the array
> and then filter out the sentinels after the loop. That makes it O(n) instead
of
> O(n^2).

Done.

[rafaelw, 2013-05-09 15:39:50]

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

https://codereview.chromium.org/14779011/diff/6001/src/object-observe.js#newc...
src/object-observe.js:185: function AcceptArgIsValid(arg) {
Arv,

what do you think about 

a) Accepting Array-likes?
b) Not accepting anything other than strings within the Array-like?

https://codereview.chromium.org/14779011/diff/6001/src/object-observe.js#newc...
src/object-observe.js:343: try {
Andreas,

Is crankshaft going to ignore this try/finally? If so, how should be approach
this?

We need to be sure to clean-up in the case that the the changeFn throws?

[adamk, 2013-05-09 16:37:06]

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

https://codereview.chromium.org/14779011/diff/6001/src/object-observe.js#newc...
src/object-observe.js:246: ensureObserverRemoved(objectInfo.inactiveObservers,
callback);
This pair of calls happens twice. Seems like it should just be a single function
that takes an objectInfo and removed the observer from whichever list it's in
(that'd also be a very minor optimization, as you wouldn't need to walk through
the inactiveObservers if it was found in changeObservers).

[rafaelw, 2013-05-09 18:47:47]

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

https://codereview.chromium.org/14779011/diff/6001/src/object-observe.js#newc...
src/object-observe.js:246: ensureObserverRemoved(objectInfo.inactiveObservers,
callback);
On 2013/05/09 16:37:07, adamk wrote:
> This pair of calls happens twice. Seems like it should just be a single
function
> that takes an objectInfo and removed the observer from whichever list it's in
> (that'd also be a very minor optimization, as you wouldn't need to walk
through
> the inactiveObservers if it was found in changeObservers).

Done.

[arv (Not doing code reviews), 2013-05-09 20:19:44]

Remove .DS_Store

[arv (Not doing code reviews), 2013-05-09 20:23:50]

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

https://codereview.chromium.org/14779011/diff/6001/src/object-observe.js#newc...
src/object-observe.js:185: function AcceptArgIsValid(arg) {
On 2013/05/09 15:39:50, rafaelw wrote:
> Arv,
> 
> what do you think about 
> 
> a) Accepting Array-likes?
> b) Not accepting anything other than strings within the Array-like?

This looks right to me. We should just iterate over 0 - length - 1 and verify
that they are strings. Anything else is irrelevant.

[rafaelw, 2013-05-13 12:06:59]

Andreas: ping.

[rossberg, 2013-05-13 16:39:17]

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

https://codereview.chromium.org/14779011/diff/6001/src/object-observe.js#newc...
src/object-observe.js:343: try {
On 2013/05/09 15:39:50, rafaelw wrote:
> Andreas,
> 
> Is crankshaft going to ignore this try/finally? If so, how should be approach
> this?
> 
> We need to be sure to clean-up in the case that the the changeFn throws?

Yes, Crankshaft won't be able to handle it, hence will never optimise this
function. I'm not sure how performance-critical it actually is, though. In any
case, you can always pull out the 'try' into an auxiliary function.

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

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:86: function createObserver(callback, accept) {
Nit: can we capitalise functions consistently?

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:124: for (; i < from.length; i++, j++) {
I think you can simplify the logic of this loop by not doing j++ here.

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:136: function moveObserversWhichAre(conditionFn, from, to,
objectInfo) {
Aren't all observers? :)

Can we perhaps call this RepartitionObservers or something like that?

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:152: if (IS_UNDEFINED(objectInfo.performing[type]))
How about writing this as:

objectInfo.performing[type] = (objectInfo.performing[type] || 0) + 1

[rafaelw, 2013-05-13 17:52:44]

thanks for the review. please land for me if lgty =-)

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

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:86: function createObserver(callback, accept) {
On 2013/05/13 16:39:17, rossberg wrote:
> Nit: can we capitalise functions consistently?

Done.

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:124: for (; i < from.length; i++, j++) {
On 2013/05/13 16:39:17, rossberg wrote:
> I think you can simplify the logic of this loop by not doing j++ here.

Done.

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:136: function moveObserversWhichAre(conditionFn, from, to,
objectInfo) {
On 2013/05/13 16:39:17, rossberg wrote:
> Aren't all observers? :)
> 
> Can we perhaps call this RepartitionObservers or something like that?

Done.

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:136: function moveObserversWhichAre(conditionFn, from, to,
objectInfo) {
On 2013/05/13 16:39:17, rossberg wrote:
> Aren't all observers? :)
> 
> Can we perhaps call this RepartitionObservers or something like that?

Done.

https://codereview.chromium.org/14779011/diff/13001/src/object-observe.js#new...
src/object-observe.js:152: if (IS_UNDEFINED(objectInfo.performing[type]))
On 2013/05/13 16:39:17, rossberg wrote:
> How about writing this as:
> 
> objectInfo.performing[type] = (objectInfo.performing[type] || 0) + 1

Done.

[rossberg, 2013-05-14 14:19:59]

Oops, sorry, I forgot to save a comment about testing performChange.

https://codereview.chromium.org/14779011/diff/18001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/14779011/diff/18001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:380: function Thingy (a, b, c) {
Nit: spurious space

https://codereview.chromium.org/14779011/diff/18001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:484: ]);
Can we also add a test that nests performChanges of the _same_ type, just to
make sure that does not confuse the logic either?

It might also be good to have a test that tries a broader range of modifications
to 'this' inside a performChange, in particular, Array stuff.

[rafaelw, 2013-05-14 18:20:24]

https://codereview.chromium.org/14779011/diff/18001/test/mjsunit/harmony/obje...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/14779011/diff/18001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:380: function Thingy (a, b, c) {
On 2013/05/14 14:19:59, rossberg wrote:
> Nit: spurious space

Done.

https://codereview.chromium.org/14779011/diff/18001/test/mjsunit/harmony/obje...
test/mjsunit/harmony/object-observe.js:484: ]);
On 2013/05/14 14:19:59, rossberg wrote:
> Can we also add a test that nests performChanges of the _same_ type, just to
> make sure that does not confuse the logic either?
> 
> It might also be good to have a test that tries a broader range of
modifications
> to 'this' inside a performChange, in particular, Array stuff.

Done.

[rossberg, 2013-05-15 12:27:09]

LGTM. Will land once I'm back on VPN.

[arv (Not doing code reviews), 2013-05-15 13:52:34]

I see that we are using for-in loops. I just wanted to ask if the order of these
loops matter? Remember that we iterate over index-like property names first.
Does that screw up the expected order?

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

https://codereview.chromium.org/14779011/diff/25001/src/object-observe.js#new...
src/object-observe.js:195: if (!IS_STRING(arg[i]))
Is this what we really want? Generally the platform just stringifies non
strings.

https://codereview.chromium.org/14779011/diff/25001/src/object-observe.js#new...
src/object-observe.js:324: if (!IS_STRING(changeType))
Same here... maybe just String(changeType) as needed.