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: } On 2012/10/31 14:44:31, rafaelw wrote:
do we have JS debug-only ASSERTS? If so, it'd be nice to
ASSERT(%IsObserved) Unfortunately, no. https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode115 src/object-observe.js:115: if (IS_UNDEFINED(objectInfo)) return; On 2012/10/31 14:44:31, rafaelw wrote:
Seems like we should CHECK() here - can we crash from JS? This should
never
happen, right?
There is no direct way to do that, but I just removed the line, so that the access below would throw an exception. Then the !threw assertion in JSObject::Notify would fire. https://codereview.chromium.org/11347037/diff/1/src/object-observe.js#newcode118 src/object-observe.js:118: if (arguments.length == 4) changeRecord.oldValue = oldValue; On 2012/10/31 14:44:31, rafaelw wrote:
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.
Done. 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#newcode1680 src/objects.cc:1680: MaybeObject* ret; On 2012/11/05 13:33:22, Toon Verwaest wrote:
Can we call this maybe_result for consistency?
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1699 src/objects.cc:1699: JSFunction::cast(value), On 2012/10/31 14:44:31, rafaelw wrote:
whitespace
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1709 src/objects.cc:1709: if (!ret->ToObject(&obj)) return ret; On 2012/11/05 13:33:22, Toon Verwaest wrote:
if (ret->IsFailure()) return ret;
I actually prefer: MaybeObject* maybe_failure = NormalizeProperties(...; if (maybe_failure->IsFailure()) return maybe_failure;
since ret will be overwritten in the next line anyway. The values seem independent.
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1720 src/objects.cc:1720: return ret; On 2012/10/31 14:44:31, rafaelw wrote:
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;
Indeed. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1720 src/objects.cc:1720: return ret; On 2012/11/05 13:33:22, Toon Verwaest wrote:
Yes. We should not jump into handlified code from non-handlified code.
This is
very prone to errors, especially since the caller of the function does
not know
that handlified code may cause GC without returning failure,
potentially
invalidating all raw pointers in the client code.
Either we have to ensure that no code is handlified for implementing NotifyObservers, or we'll have to pull it up to the handlified client
of this
functionality.
Most of the methods in question are already documented as "can cause GC", and I added respective comments to the remaining ones (plus, checked all their call sites). As discussed offline, there is no real alternative short of "handlifying all the code" -- which I agree we should do at some point. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode1732 src/objects.cc:1732: if (oldValue_raw->IsTheHole()) { On 2012/10/31 14:44:31, rafaelw wrote:
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(). Well, that would make the code pretty verbose as well. I removed the duplication using a simpler trick. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode2990 src/objects.cc:2990: return ret; On 2012/10/31 14:44:31, rafaelw wrote:
same question about returning MaybeObject after invoking script.
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode3121 src/objects.cc:3121: return ret; On 2012/10/31 14:44:31, rafaelw wrote:
same question about returning MaybeObject
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4069 src/objects.cc:4069: return ret; On 2012/10/31 14:44:31, rafaelw wrote:
ditto
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode4716 src/objects.cc:4716: return ret; On 2012/10/31 14:44:31, rafaelw wrote:
ditto
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.cc#newcode7620 src/objects.cc:7620: (other->bit_field3() & IsObserved::kMask) && On 2012/11/05 13:33:22, Toon Verwaest wrote:
Can we just use is_observed() == other->is_observed()?
Done. 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); On 2012/11/05 13:33:22, Toon Verwaest wrote:
Yes, use old_value in the C++ code. Also in other files.
On 2012/10/31 17:27:24, rafaelw wrote: > also, are arguments unix_hacker style?
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279 src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object* oldValue); On 2012/10/31 17:27:24, rafaelw wrote:
also, are arguments unix_hacker style?
Done. https://codereview.chromium.org/11347037/diff/1/src/objects.h#newcode2279 src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object* oldValue); On 2012/10/31 14:44:31, rafaelw wrote:
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.
Done. https://codereview.chromium.org/11347037/diff/1/src/runtime.cc File src/runtime.cc (right): https://codereview.chromium.org/11347037/diff/1/src/runtime.cc#newcode13240 src/runtime.cc:13240: if (!maybe->To<Map>(&map)) return maybe; On 2012/11/05 13:33:22, Toon Verwaest wrote:
The <Map> is not needed; maybe->To(&map) works just fine.
Done. https://codereview.chromium.org/11347037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
