Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
7 years, 7 months ago by rafaelw Modified:
7 years, 7 months ago Reviewers:
arv (Not doing code reviews), rossberg, adamk CC:
v8-dev Visibility:
Public. |
DescriptionImplement Object.getNotifier(obj).performChange()
R=rossberg,adamk,arv
BUG=
Committed: https://code.google.com/p/v8/source/detail?r=14696
Patch Set 1 #
Total comments: 5
Patch Set 2 : cr comments #
Total comments: 6
Patch Set 3 : fix removeNullElements #Patch Set 4 : moar cr changes #Patch Set 5 : remove DS_Store #
Total comments: 9
Patch Set 6 : cr changes #
Total comments: 4
Patch Set 7 : cr changes #
Total comments: 2
Created: 7 years, 7 months ago
Messages
Total messages: 15 (0 generated)
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.
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.
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?
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).
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.
Remove .DS_Store
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.
Andreas: ping.
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
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.
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.
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.
LGTM. Will land once I'm back on VPN.
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
