[builtins] don't create keys for undefined property descriptors in O.gOPDs

[builtins] don't create keys for undefined property descriptors in O.gOPDs

Implements the change proposed at https://github.com/tc39/ecma262/pull/593.

In summary, Object.getOwnPropertyDescriptors can produce results which cause
Object.defineProperties() to throw, by inserting a property with an undefined
descriptor into the result object. This change to the algorithm requires that
the descriptor only be added to the result object if it is not undefined.

BUG=v8:4725
R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com

Committed: https://crrev.com/81349869174c76216e69c511396b1c05c826fa5e
Cr-Commit-Position: refs/heads/master@{#37504}

[caitp (gmail), 2016-07-01 20:50:20]

Hi, PTAL

[adamk, 2016-07-01 21:02:52]

This is definitely the way I'd want this to get resolved, but it's not clear to
me there's any consensus on this particular issue.

[Dan Ehrenberg, 2016-07-01 23:46:55]

On 2016/07/01 21:02:52, adamk (out until june 12) wrote:
> This is definitely the way I'd want this to get resolved, but it's not clear
to
> me there's any consensus on this particular issue.

I think the new semantics are pretty reasonable, and your implementation looks
correct with respect to the new spec to me. Let's not ship this feature until
the semantics are standardized one way or another in TC39, but I think it's fine
to change our implementation for now since it's not shipped; we can always roll
it back later.

lgtm

[caitp (gmail), 2016-07-01 23:56:10]

On 2016/07/01 23:46:55, Dan Ehrenberg wrote:
> On 2016/07/01 21:02:52, adamk (out until june 12) wrote:
> > This is definitely the way I'd want this to get resolved, but it's not clear
> to
> > me there's any consensus on this particular issue.
> 
> I think the new semantics are pretty reasonable, and your implementation looks
> correct with respect to the new spec to me. Let's not ship this feature until
> the semantics are standardized one way or another in TC39, but I think it's
fine
> to change our implementation for now since it's not shipped; we can always
roll
> it back later.
> 
> lgtm

if it lands before the test262 changes land and roll, this needs changes to
test262.status. I'll add those on Monday

[caitp (gmail), 2016-07-04 16:00:18]

On 2016/07/01 23:56:10, caitp wrote:
> On 2016/07/01 23:46:55, Dan Ehrenberg wrote:
> > On 2016/07/01 21:02:52, adamk (out until june 12) wrote:
> > > This is definitely the way I'd want this to get resolved, but it's not
clear
> > to
> > > me there's any consensus on this particular issue.
> > 
> > I think the new semantics are pretty reasonable, and your implementation
looks
> > correct with respect to the new spec to me. Let's not ship this feature
until
> > the semantics are standardized one way or another in TC39, but I think it's
> fine
> > to change our implementation for now since it's not shipped; we can always
> roll
> > it back later.
> > 
> > lgtm
> 
> if it lands before the test262 changes land and roll, this needs changes to
> test262.status. I'll add those on Monday

done --- test expectations for duplicate-keys is [FAIL], so it should be easy to
fix in a future test262 roll incorporating
https://github.com/tc39/test262/pull/662

[caitp (gmail), 2016-07-04 16:00:24]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-07-04 16:00:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp (gmail), 2016-07-04 16:02:50]

On 2016/07/04 16:00:34, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

er, wait, somehow I didn't finish uploading the updated patch... huh. no
problem, this CQ run should fail.

[caitp (gmail), 2016-07-04 17:40:16]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-07-04 17:40:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2118613003/#ps20001
(title: "fix test262.status")

[commit-bot: I haz the power, 2016-07-04 17:40:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 18:02:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-04 18:04:14]

Description was changed from

==========
[builtins] don't create keys for undefined property descriptors in O.gOPDs

Implements the change proposed at https://github.com/tc39/ecma262/pull/593.

In summary, Object.getOwnPropertyDescriptors can produce results which cause
Object.defineProperties() to throw, by inserting a property with an undefined
descriptor into the result object. This change to the algorithm requires that
the descriptor only be added to the result object if it is not undefined.

BUG=v8:4725
R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com
==========

to

==========
[builtins] don't create keys for undefined property descriptors in O.gOPDs

Implements the change proposed at https://github.com/tc39/ecma262/pull/593.

In summary, Object.getOwnPropertyDescriptors can produce results which cause
Object.defineProperties() to throw, by inserting a property with an undefined
descriptor into the result object. This change to the algorithm requires that
the descriptor only be added to the result object if it is not undefined.

BUG=v8:4725
R=littledan@chromium.org, adamk@chromium.org, jwolfe@igalia.com

Committed: https://crrev.com/81349869174c76216e69c511396b1c05c826fa5e
Cr-Commit-Position: refs/heads/master@{#37504}
==========

[commit-bot: I haz the power, 2016-07-04 18:04:15]

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