https://codereview.chromium.org/1154233003/diff/1/include/v8.h
File include/v8.h (right):
https://codereview.chromium.org/1154233003/diff/1/include/v8.h#newcode2595
include/v8.h:2595: V8_WARN_UNUSED_RESULT Maybe<bool>
CreateDataProperty(Local<Context> context,
On 2015/05/26 at 18:55:56, adamk wrote:
Please add a comment explaining what this does, how it fails, what it
returns.
You could also reference the spec from the comment.
done
https://codereview.chromium.org/1154233003/diff/1/src/api.cc
File src/api.cc (right):
https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3467
src/api.cc:3467: PREPARE_FOR_EXECUTION_PRIMITIVE(context,
"v8::Object::CreateDataProperty()",
On 2015/05/26 at 19:40:55, Toon Verwaest wrote:
It seems like this is more of a DefineDataProperty or AddDataProperty
than CreateDataProperty. I wouldn't even know what that would mean in
JS...
Can we just expose methods that have a JS equivalent? Doing the
alternative is bringing us trouble as it is; which is why you are
replacing ForceSet.
it's the name of the JS method.
https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3479
src/api.cc:3479: // Special case for Array.length.
On 2015/05/26 at 19:40:55, Toon Verwaest wrote:
On 2015/05/26 16:49:44, jochen wrote:
> Array.prototype.length is not configurable, so this just throws :-/
Shouldn't this be allowed to modify the value of the length of nothing
else is "reconfigured"? Why is there a special path for this one
property? I wouldn't be surprised if that means that either this
property, or all the others of its class are wrongly handled.
see v8natives.js
https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3495
src/api.cc:3495: if (isolate->MayAccess(self)) return Just(false);
On 2015/05/26 at 19:40:55, Toon Verwaest wrote:
On 2015/05/26 17:22:27, noordhuis wrote:
> Should this be `if (!isolate->MayAccess(self)) ...`?
Yes. Seems like a missing test.
done
https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3546
src/api.cc:3546: return Just(false);
On 2015/05/26 at 19:40:55, Toon Verwaest wrote:
Add {} if on new line
done
https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3556
src/api.cc:3556: self, isolate->factory()->Uint32ToString(index),
value_obj, NONE,
On 2015/05/26 at 19:40:55, Toon Verwaest wrote:
You cannot use SetOwn... For elements at this moment. This will do
something entirely random and broken. Add a test and you'll see...
this code path is covered by the test that sets arr[0] = 42 and arr[1] =
23
it's also what DefineObjectProperty does...
https://codereview.chromium.org/1154233003/
--
--
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/d/optout.
