https://codereview.chromium.org/11347037/diff/1/src/object-observe.js File src/object-observe.js (right):
https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode73 src/object-observe.js:73: } do we have JS debug-only ASSERTS? If so, it'd be nice to ASSERT(%IsObserved) https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode115 src/object-observe.js:115: if (IS_UNDEFINED(objectInfo)) return; Seems like we should CHECK() here - can we crash from JS? This should never happen, right? https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode118 src/object-observe.js:118: if (arguments.length == 4) changeRecord.oldValue = oldValue; For safety, I think that you should switch on arguments.length and create one of two object literals. $Object is theoritically exposed and user script could have installed an accessor on Object.prototype.oldValue. https://codereview.chromium.org/11347037/diff/1/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1699 src/objects.cc:1699: JSFunction::cast(value), whitespace https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1720 src/objects.cc:1720: return ret; Isn't this unsafe? The notifyObservers could have called GC. Don't you need to: if (ret->IsFailure()) return ret; Handle<Object> return_handle(return_value->ToObjectUnchecked()); // call NotifyObservers return *return_handle; https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1732 src/objects.cc:1732: if (oldValue_raw->IsTheHole()) { nit: This can be cleaner if you make argv a ScopedVector<Handle<Object>
, which contains either 3 or 4 values based on whether
oldValue_raw->IsTheHole(). https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode2990 src/objects.cc:2990: return ret; same question about returning MaybeObject after invoking script. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode3121 src/objects.cc:3121: return ret; same question about returning MaybeObject https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4069 src/objects.cc:4069: return ret; ditto https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4716 src/objects.cc:4716: return ret; ditto https://codereview.chromium.org/11347037/diff/1/src/objects.h File src/objects.h (right): https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279 src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object* oldValue); suggestion: the name "NotifyObservers" is a bit misleading here because observers don't get notified via this function. If you look at our new "working" branch: https://github.com/rafaelw/v8/tree/object-observe2 We added something similar: https://github.com/rafaelw/v8/blob/object-observe2/src/object-observe.cc#L49 But called it EnqueueChangeRecord. This name matches the spec text. https://codereview.chromium.org/11347037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
