LGTM with comments
https://codereview.chromium.org/47703003/diff/130001/src/object-observe.js
File src/object-observe.js (right):
https://codereview.chromium.org/47703003/diff/130001/src/object-observe.js#newcode447
src/object-observe.js:447: var changeRecord;
I think you can write this more concisely as follows:
var changeRecord = { type: type, object: object };
if (arguments.length >= 3) changeRecord.name = name;
if (arguments.length >= 4) changeRecord.oldValue = oldValue;
https://codereview.chromium.org/47703003/diff/130001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/47703003/diff/130001/src/objects.cc#newcode2171
src/objects.cc:2171: int argc = 4;
int argc = name.is_null() ? 2 : old_value->IsTheHole() ? 3 : 4;
https://codereview.chromium.org/47703003/diff/130001/src/objects.cc#newcode5496
src/objects.cc:5496: EnqueueChangeRecord(object, "preventExtensions",
Handle<Name>::null(),
Nit: Handle<Name>() is enough
https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):
https://codereview.chromium.org/47703003/diff/130001/test/mjsunit/harmony/object-observe.js#newcode303
test/mjsunit/harmony/object-observe.js:303: // Object.preventExtensions
Cases you could add:
- invoking preventExtensions/seal/freeze on an object that already has
preventExtensions (should not create a preventExtensions change record)
- invoking seal/freeze on an object where not all properties can be
sealed/frozen (should still generate a preventExtensions change record,
IIRC)
https://codereview.chromium.org/47703003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
