Introduce v8::Object::CreateDataProperty

Introduce v8::Object::CreateDataProperty

Also deprecate ForceSet

BUG=chromium:475206
R=adamk@chromium.org,verwaest@chromium.org
LOG=y

Committed: https://crrev.com/2a058de88fa6cfa6d7a76783269f39043f0d8afc
Cr-Commit-Position: refs/heads/master@{#28660}

[jochen (gone - plz use gerrit), 2015-05-26 16:49:44]

ptal

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#newcode3479
src/api.cc:3479: // Special case for Array.length.
Array.prototype.length is not configurable, so this just throws :-/

[noordhuis, 2015-05-26 17:22:27]

info@bnoordhuis.nl changed reviewers:
+ info@bnoordhuis.nl

[noordhuis, 2015-05-26 17:22:28]

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#newcode3495
src/api.cc:3495: if (isolate->MayAccess(self)) return Just(false);
Should this be `if (!isolate->MayAccess(self)) ...`?

[adamk, 2015-05-26 18:55:57]

High-level question: why did you choose to make this throw on failure? It could
just return false if the property already exists. That'd make your array case
simpler, for one.

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,
Please add a comment explaining what this does, how it fails, what it returns.


You could also reference the spec from the comment.

[Toon Verwaest, 2015-05-26 19:40:55]

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()",
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.

https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3469
src/api.cc:3469: auto self = Utils::OpenHandle(this);
Note that I'm in the process of merging all named property and element handing.
This introduces duplication again...

https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3479
src/api.cc:3479: // Special case for Array.length.
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.

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 17:22:27, noordhuis wrote:
> Should this be `if (!isolate->MayAccess(self)) ...`?

Yes. Seems like a missing test.

https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3546
src/api.cc:3546: return Just(false);
Add {} if on new line

https://codereview.chromium.org/1154233003/diff/1/src/api.cc#newcode3556
src/api.cc:3556: self, isolate->factory()->Uint32ToString(index), value_obj,
NONE,
You cannot use SetOwn... For elements at this moment. This will do something
entirely random and broken. Add a test and you'll see...

[Toon Verwaest, 2015-05-26 19:44:05]

OK I see from the other issue that the name comes from the spec, my bad.

[jochen (gone - plz use gerrit), 2015-05-27 07:28:30]

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...

[Toon Verwaest, 2015-05-27 08:03:57]

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#newcode3556
src/api.cc:3556: self, isolate->factory()->Uint32ToString(index), value_obj,
NONE,
On 2015/05/27 07:28:29, jochen wrote:
> 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...

I can guarantee you that that code path doesn't work. I wrote it.

[jochen (gone - plz use gerrit), 2015-05-27 09:18:34]

On 2015/05/27 at 08:03:57, verwaest wrote:
> 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#newcode3556
> src/api.cc:3556: self, isolate->factory()->Uint32ToString(index), value_obj,
NONE,
> On 2015/05/27 07:28:29, jochen wrote:
> > 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...
> 
> I can guarantee you that that code path doesn't work. I wrote it.

oh, I see what you're saying. This code is just never invoked in the indexed
case.

[Toon Verwaest, 2015-05-27 09:50:07]

Looking good. Some final comments.

https://codereview.chromium.org/1154233003/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1154233003/diff/80001/src/api.cc#newcode3482
src/api.cc:3482: isolate->factory()->NewStringFromStaticChars("length")))) {
isolate->factory()->length_string()

https://codereview.chromium.org/1154233003/diff/80001/src/api.cc#newcode3496
src/api.cc:3496: if (it.property_details().attributes() & DONT_DELETE) return
Just(false);
!it.IsConfigurable()

https://codereview.chromium.org/1154233003/diff/80001/src/api.cc#newcode3539
src/api.cc:3539: if (self->IsAccessCheckNeeded() && !isolate->MayAccess(self)) {
According to  https://etherpad.mozilla.org/html5-cross-origin-objects  this
should throw an exception. If so, you'll have to move this up to the top of the
function and check it first.

[jochen (gone - plz use gerrit), 2015-05-27 09:56:44]

ptal

[Toon Verwaest, 2015-05-27 10:07:51]

lgtm

[jochen (gone - plz use gerrit), 2015-05-27 10:49:42]

Adam, please CQ+ if you don't have additional comments

[adamk, 2015-05-27 15:01:35]

lgtm

[adamk, 2015-05-27 15:01:39]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-05-27 15:01:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154233003/100001

[commit-bot: I haz the power, 2015-05-27 15:03:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-27 15:03:45]

Patchset 6 (id:??) landed as
https://crrev.com/2a058de88fa6cfa6d7a76783269f39043f0d8afc
Cr-Commit-Position: refs/heads/master@{#28660}

[Honestly, 2017-07-13 16:44:45]

dewydew1166@gmail.com changed reviewers:
+ dewydew1166@gmail.com

[Honestly, 2017-07-13 16:44:45]

dewydew1166@gmail.com ExtensionsExtension were preventing me from accessing
chrome for help. Someone is in my phone. I haven't added extensions to my
device.