[es6] Partially implement Reflect.getOwnPropertyDescriptor.

[es6] Partially implement Reflect.getOwnPropertyDescriptor.

Proxies are not properly supported yet.

R=rossberg, jkummerow@chromium.org
BUG=

Committed: https://crrev.com/98dfe024ca3b573658a5a8572ac1fc06611c3eda
Cr-Commit-Position: refs/heads/master@{#31674}

[Jakob Kummerow, 2015-10-26 13:58:26]

LGTM with nits.

https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc#newcode1525
src/builtins.cc:1525: if (!found) return isolate->heap()->undefined_value();
How about folding this into PropertyDescriptor::ToObject? It would nicely mirror
step 1 "If Desc is undefined, return undefined." of the spec if that function
began with "if (this->IsEmpty()) return isolate->heap()->undefined_value();".

https://codereview.chromium.org/1408163005/diff/1/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1408163005/diff/1/src/property-descriptor.cc#...
src/property-descriptor.cc:124: CreateDataProperty(isolate, result, "get",
get());
I'm a bit concerned about the speed of building objects like this. It's probably
about 10x - 100x slower than using StoreICs. As a reference implementation, it's
OK for now because there is no easy alternative, but I suspect we'll want to
come up with a way to make it faster at some point.

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
File test/mjsunit/harmony/reflect-get-own-property-descriptor.js (right):

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
test/mjsunit/harmony/reflect-get-own-property-descriptor.js:1: // Copyright
2010-2015 the V8 project authors. All rights reserved.
nit: drop "2010-", just "2015"

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
File test/mjsunit/harmony/reflect.js (right):

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
test/mjsunit/harmony/reflect.js:347: var tgt = {bla: 42};
nit: s/tgt/target/

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
test/mjsunit/harmony/reflect.js:356: // See
reflect-get-own-property-descriptor.js for further tests.
Any particular reason why the tests are split across two files?

[neis, 2015-10-29 16:48:39]

https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc#newcode1525
src/builtins.cc:1525: if (!found) return isolate->heap()->undefined_value();
On 2015/10/26 13:58:26, Jakob wrote:
> How about folding this into PropertyDescriptor::ToObject? It would nicely
mirror
> step 1 "If Desc is undefined, return undefined." of the spec if that function
> began with "if (this->IsEmpty()) return isolate->heap()->undefined_value();".

This doesn't quite work without passing in "found" to ToObject (I don't want to
treat the empty descriptor as undefined).

Related to that, I changed ToObject slightly such that it works even for
incomplete descriptors.  There was no good reason for assuming complete inputs
(it only saved a few checks).  Please have another look.

[Jakob Kummerow, 2015-10-29 18:06:02]

On 2015/10/29 16:48:39, neis wrote:
> https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc
> File src/builtins.cc (right):
> 
> https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc#newcode1525
> src/builtins.cc:1525: if (!found) return isolate->heap()->undefined_value();
> On 2015/10/26 13:58:26, Jakob wrote:
> > How about folding this into PropertyDescriptor::ToObject? It would nicely
> mirror
> > step 1 "If Desc is undefined, return undefined." of the spec if that
function
> > began with "if (this->IsEmpty()) return
isolate->heap()->undefined_value();".
> 
> This doesn't quite work without passing in "found" to ToObject (I don't want
to
> treat the empty descriptor as undefined).

Why not? The spec, being JavaScript-y, says nonexistent properties return
"undefined" as their descriptor; in our typed C++ implementation we use an empty
PropertyDescriptor instead. I see no reason not to treat it as what it is: the
sentinel for a nonexistent property. When converting a C++ PropertyDescriptor to
a JavaScript object, converting an empty descriptor to "undefined" is exactly
the right choice.
(That said, I don't feel strongly enough about it to fight your preference; do
as you please.)

> Related to that, I changed ToObject slightly such that it works even for
> incomplete descriptors.  There was no good reason for assuming complete inputs
> (it only saved a few checks).

How about "it'll only ever be used to convert the results of
JSReceiver::GetOwnPropertyDescriptor() calls to JS objects"? Is there a good
reason to make it more flexible than it needs to be? How can you trust the code
to be correct for all corner cases if the flexibility it provides is never
exercised?
(This, too, is a rather harmless detail and you can keep it the way it is if you
wish; however I do want to challenge the idea that making things more generic
than they need to be is somehow beneficial. Even *if* a new use case comes
along, it's better to find a limited implementation and consciously extend it,
than to find an implementation that claims to be generic only to later be told
by ClusterFuzz that it didn't, in fact, support the new use case.)

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
File test/mjsunit/harmony/reflect.js (right):

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
test/mjsunit/harmony/reflect.js:347: var tgt = {bla: 42};
On 2015/10/26 13:58:26, Jakob wrote:
> nit: s/tgt/target/

Forgot this?

https://codereview.chromium.org/1408163005/diff/40001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1408163005/diff/40001/src/property-descriptor...
src/property-descriptor.cc:119: if (has_value()) CreateDataProperty(isolate,
result, "value", value());
Instead of "value", please use isolate->factory()->value_string() here (change
the signature of CreateDataProperty above to take a Handle<String> instead of a
const char*).
You may want to pull out "Factory* factory = isolate->factory()" to make the
code less verbose; it doesn't make a performance difference.

Same for the other property names below.

[neis, 2015-10-30 08:39:41]

On 2015/10/29 18:06:02, Jakob wrote:
> On 2015/10/29 16:48:39, neis wrote:
> > https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc
> > File src/builtins.cc (right):
> > 
> >
https://codereview.chromium.org/1408163005/diff/1/src/builtins.cc#newcode1525
> > src/builtins.cc:1525: if (!found) return isolate->heap()->undefined_value();
> > On 2015/10/26 13:58:26, Jakob wrote:
> > > How about folding this into PropertyDescriptor::ToObject? It would nicely
> > mirror
> > > step 1 "If Desc is undefined, return undefined." of the spec if that
> function
> > > began with "if (this->IsEmpty()) return
> isolate->heap()->undefined_value();".
> > 
> > This doesn't quite work without passing in "found" to ToObject (I don't want
> to
> > treat the empty descriptor as undefined).
> 
> Why not? The spec, being JavaScript-y, says nonexistent properties return
> "undefined" as their descriptor; in our typed C++ implementation we use an
empty
> PropertyDescriptor instead.

GetOwnPropertyDescriptor returns false for a non-existent property (as its
description says).  Whether or not it returns an empty descriptor depends on
what the caller passed in.

> > Related to that, I changed ToObject slightly such that it works even for
> > incomplete descriptors.  There was no good reason for assuming complete
inputs
> > (it only saved a few checks).
> 
> How about "it'll only ever be used to convert the results of
> JSReceiver::GetOwnPropertyDescriptor() calls to JS objects"? Is there a good
> reason to make it more flexible than it needs to be? How can you trust the
code
> to be correct for all corner cases if the flexibility it provides is never
> exercised?

The good reason for me is that the generalization is no more complicated
(perhaps even simpler) and that the code then pretty much corresponds exactly to
the spec.

[neis, 2015-10-30 08:41:30]

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
File test/mjsunit/harmony/reflect.js (right):

https://codereview.chromium.org/1408163005/diff/1/test/mjsunit/harmony/reflec...
test/mjsunit/harmony/reflect.js:347: var tgt = {bla: 42};
On 2015/10/29 18:06:02, Jakob wrote:
> On 2015/10/26 13:58:26, Jakob wrote:
> > nit: s/tgt/target/
> 
> Forgot this?

Done.

https://codereview.chromium.org/1408163005/diff/40001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1408163005/diff/40001/src/property-descriptor...
src/property-descriptor.cc:119: if (has_value()) CreateDataProperty(isolate,
result, "value", value());
On 2015/10/29 18:06:02, Jakob wrote:
> Instead of "value", please use isolate->factory()->value_string() here (change
> the signature of CreateDataProperty above to take a Handle<String> instead of
a
> const char*).
> You may want to pull out "Factory* factory = isolate->factory()" to make the
> code less verbose; it doesn't make a performance difference.
> 
> Same for the other property names below.

Done.

[Jakob Kummerow, 2015-10-30 08:55:20]

lgtm

https://codereview.chromium.org/1408163005/diff/60001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1408163005/diff/60001/src/property-descriptor...
src/property-descriptor.cc:124: isolate->factory()->ToBoolean(writable()));
nit: s/isolate->factory()/factory/, again twice below.

[neis, 2015-10-30 09:42:58]

https://codereview.chromium.org/1408163005/diff/60001/src/property-descriptor.cc
File src/property-descriptor.cc (right):

https://codereview.chromium.org/1408163005/diff/60001/src/property-descriptor...
src/property-descriptor.cc:124: isolate->factory()->ToBoolean(writable()));
On 2015/10/30 08:55:19, Jakob wrote:
> nit: s/isolate->factory()/factory/, again twice below.

Thanks.

[neis, 2015-10-30 09:44:34]

The CQ bit was checked by neis@chromium.org

[neis, 2015-10-30 09:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1408163005/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-10-30 09:44:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408163005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408163005/80001

[commit-bot: I haz the power, 2015-10-30 10:04:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-30 10:05:10]

Patchset 5 (id:??) landed as
https://crrev.com/98dfe024ca3b573658a5a8572ac1fc06611c3eda
Cr-Commit-Position: refs/heads/master@{#31674}

[PhistucK, 2015-11-02 20:42:43]

It would be great if you could always make sure that the BUG= annotation is
filled with the right bug.

Thank you!

[PhistucK, 2015-11-02 20:43:04]

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