ES6: Make function name configurable

ES6: Make function name configurable

This is partially based on r21609 but that CL was incomplete.

Function name is still non writable so one has to use defineProperty
to change the actual value.

BUG=v8:3333
LOG=N
R=adamk, mstarzinger@chromium.org

Committed: https://crrev.com/f7790f7670c8d859455a98fcb90ff1b66af1eca7
Cr-Commit-Position: refs/heads/master@{#26924}

[arv (Not doing code reviews), 2015-02-26 20:44:13]

more tests

[arv (Not doing code reviews), 2015-02-26 21:20:58]

Make sure we test both sloppy and strict

[arv (Not doing code reviews), 2015-02-26 21:24:05]

remove debug prints

[arv (Not doing code reviews), 2015-02-26 21:25:12]

whitespace

[arv (Not doing code reviews), 2015-02-26 21:25:42]

Patchset #4 (id:60001) has been deleted

[arv (Not doing code reviews), 2015-02-26 21:25:48]

Patchset #2 (id:20001) has been deleted

[arv (Not doing code reviews), 2015-02-26 21:26:24]

PTAL

I'll also apply this to Blink to see what LayoutTest this needs updating.

https://codereview.chromium.org/960343002/diff/80001/test/mjsunit/es7/object-...
File test/mjsunit/es7/object-observe.js (right):

https://codereview.chromium.org/960343002/diff/80001/test/mjsunit/es7/object-...
test/mjsunit/es7/object-observe.js:1146: (obj instanceof Function && prop ===
"name")
Adam, do you know why this is needed?

[arv (Not doing code reviews), 2015-02-26 21:39:00]

git rebase

[arv (Not doing code reviews), 2015-02-26 22:27:35]

Relax assert

[arv (Not doing code reviews), 2015-02-26 22:30:36]

arv@chromium.org changed reviewers:
+ verwaest@chromium.org

[arv (Not doing code reviews), 2015-02-26 22:30:37]

https://codereview.chromium.org/960343002/diff/120001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/960343002/diff/120001/src/lookup.cc#newcode316
src/lookup.cc:316: DCHECK(DATA == state_ || ACCESSOR == state_);
Toon, this cannot be right.

What is the right way to replace an accessor with a data property?

[arv (Not doing code reviews), 2015-02-26 23:22:04]

On 2015/02/26 21:26:24, arv wrote:
> PTAL
> 
> I'll also apply this to Blink to see what LayoutTest this needs updating.

Here is the Blink test expectation change.

[adamk, 2015-02-26 23:31:51]

Answered the Object.observe part of this. Basically we treat these
accessor-backed data properties just the same as API-level accessor-backed data
properties, and thus we don't want to notify about changes to them in general.
Instead, your accessor needs to call EnqueueChangeRecord.

https://codereview.chromium.org/960343002/diff/80001/test/mjsunit/es7/object-...
File test/mjsunit/es7/object-observe.js (right):

https://codereview.chromium.org/960343002/diff/80001/test/mjsunit/es7/object-...
test/mjsunit/es7/object-observe.js:1146: (obj instanceof Function && prop ===
"name")
On 2015/02/26 21:26:24, arv wrote:
> Adam, do you know why this is needed?

Because this sort of "internal" accessor needs to do its own "notifying". See
SetFunctionPrototype for an example.

https://codereview.chromium.org/960343002/diff/120001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/960343002/diff/120001/src/accessors.cc#newcod...
src/accessors.cc:1151: }
You're going to want an EnqueueChangeRecord call here similar to the one for
SetFunctionPrototype.

[Toon Verwaest, 2015-02-27 12:44:11]

https://codereview.chromium.org/960343002/diff/120001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/960343002/diff/120001/src/accessors.cc#newcod...
src/accessors.cc:1151: }
On 2015/02/26 23:31:51, adamk wrote:
> You're going to want an EnqueueChangeRecord call here similar to the one for
> SetFunctionPrototype.

Unlike Function::SetPrototype, SetDataProperty already does this. You might want
to add a test though.

https://codereview.chromium.org/960343002/diff/120001/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/960343002/diff/120001/src/lookup.cc#newcode316
src/lookup.cc:316: DCHECK(DATA == state_ || ACCESSOR == state_);
On 2015/02/26 22:30:37, arv wrote:
> Toon, this cannot be right.
> 
> What is the right way to replace an accessor with a data property?

This isn't right indeed. Are you sure you need this? it.PrepareForDataProperty()
should have reconfigured and reloaded the propertyinfo, so you should be in
state_ == DATA by the time you SetDataProperty.

[Toon Verwaest, 2015-02-27 12:45:39]

(I'm not 100% sure about the O.o thing, I guess adamk knows what he's talking
about ... :))

[arv (Not doing code reviews), 2015-02-27 15:31:06]

Handle O.o

[arv (Not doing code reviews), 2015-02-27 15:37:33]

cleanup

[arv (Not doing code reviews), 2015-02-27 15:44:33]

https://codereview.chromium.org/960343002/diff/160001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/960343002/diff/160001/src/accessors.cc#newcod...
src/accessors.cc:1147: value = it.WriteDataValue(value);
Turns out that I could not use SetDataProperty since it does not know how to get
the old value.

https://codereview.chromium.org/960343002/diff/160001/test/mjsunit/es7/object...
File test/mjsunit/es7/object-observe.js (right):

https://codereview.chromium.org/960343002/diff/160001/test/mjsunit/es7/object...
test/mjsunit/es7/object-observe.js:1001:
assertTrue(Object.getOwnPropertyDescriptor(obj, prop).configurable);
This happens in the stress test. Not sure why yet. Still investigating.

[arv (Not doing code reviews), 2015-02-27 16:23:15]

https://codereview.chromium.org/960343002/diff/160001/test/mjsunit/es7/object...
File test/mjsunit/es7/object-observe.js (right):

https://codereview.chromium.org/960343002/diff/160001/test/mjsunit/es7/object...
test/mjsunit/es7/object-observe.js:1001:
assertTrue(Object.getOwnPropertyDescriptor(obj, prop).configurable);
On 2015/02/27 15:44:33, arv wrote:
> This happens in the stress test. Not sure why yet. Still investigating.

I figured it out. This failing has nothing to do with the C++ code. It is becase
F.p.name is non writable so line 993, 994, 995 do not create an own property.
The we hit line 996 and create a non configurable property. Yikes.

I'll black list Function name and write a dedicated test for it instead.

[arv (Not doing code reviews), 2015-02-27 16:50:21]

Fix O.o test

[arv (Not doing code reviews), 2015-02-27 16:51:41]

PTAL

This is now ready to go. The Blink surpression has also landed.

[adamk, 2015-02-27 17:27:51]

lgtm

[Toon Verwaest, 2015-02-27 17:49:36]

lgtm

[arv (Not doing code reviews), 2015-02-27 17:53:49]

Return directly from EnqueueChangeRecord

[arv (Not doing code reviews), 2015-02-27 17:53:59]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-02-27 17:54:00]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/960343002/#ps200001
(title: "Return directly from EnqueueChangeRecord")

[commit-bot: I haz the power, 2015-02-27 17:55:19]

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

[commit-bot: I haz the power, 2015-02-27 18:13:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-27 18:13:50]

Try jobs failed on following builders:
  v8_linux64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel/builds/3418)

[arv (Not doing code reviews), 2015-02-27 18:55:48]

Use reconfigure instead

[arv (Not doing code reviews), 2015-02-27 19:17:28]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-02-27 19:17:29]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/960343002/#ps220001
(title: "Use reconfigure instead")

[commit-bot: I haz the power, 2015-02-27 19:18:28]

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

[commit-bot: I haz the power, 2015-02-27 19:29:01]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2015-02-27 19:29:11]

Patchset 10 (id:??) landed as
https://crrev.com/f7790f7670c8d859455a98fcb90ff1b66af1eca7
Cr-Commit-Position: refs/heads/master@{#26924}

[arv (Not doing code reviews), 2015-03-02 00:18:36]

A revert of this CL (patchset #10 id:220001) has been created in
https://codereview.chromium.org/969683002/ by arv@chromium.org.

The reason for reverting is: Breaks Chrome browser test that checks Object.name

[16509:16509:0228/030150:INFO:CONSOLE(43)] "Uncaught Error: Clobbered
Object.name getter", source: http://www.chromium.org:33611/assertions.js (43)

http://build.chromium.org/p/client.v8/builders/Linux%20Tests%20%28dbg%29%281%....