Make Function.length and Function.name configurable properties.

Make Function.length and Function.name configurable properties.

ES6 makes the Function object properties "length" and "name"
configurable; switch the implementation over to follow that.

Doing so exposed a problem in the handling of non-writable, but
configurable properties backed by foreign callback accessors
internally. As an optimization, if such an accessor property is
re-defined with a new value, its setter was passed the new value
directly, keeping the property as an accessor property. However, this
is not correct should the property be non-writable, as its setter will
then simply ignore the updated value. Adjust the enabling logic for
this optimization accordingly, along with adding a test.

LOG=N
R=rossberg@chromium.org, rossberg
BUG=v8:3045
Committed: https://code.google.com/p/v8/source/detail?r=19200

[sof, 2013-12-28 16:12:31]

At your leisure, please take a look.

(Addressing issue that came up when changing writability of "name" for strict
function objects.)

[rossberg, 2014-01-09 12:26:39]

LGTM with comments

https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-observe.js:994: if (Object.getPrototypeOf(obj) &&
(prop in Object.getPrototypeOf(obj)))
Only do this fall-back when really necessary, i.e., if the inherited property is
read-only (or a JS accessor, but I don't think that comes up here).

Nit: redundant parens

https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-observe.js:995: Object.defineProperty(obj, prop,
{value: 4, writable: true, enumerable: true, configurable: true});
Nit: line length

[sof, 2014-01-09 14:04:51]

https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
test/mjsunit/harmony/object-observe.js:994: if (Object.getPrototypeOf(obj) &&
(prop in Object.getPrototypeOf(obj)))
On 2014/01/09 12:26:39, rossberg wrote:
> Only do this fall-back when really necessary, i.e., if the inherited property
is
> read-only (or a JS accessor, but I don't think that comes up here).
> 
> Nit: redundant parens

Done, I think (but it feels counterintuitive to "reuse" any property up the
prototype chain in this test.)

[rossberg, 2014-01-09 14:59:24]

On 2014/01/09 14:04:51, sof wrote:
>
https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
> File test/mjsunit/harmony/object-observe.js (right):
> 
>
https://codereview.chromium.org/116083006/diff/1/test/mjsunit/harmony/object-...
> test/mjsunit/harmony/object-observe.js:994: if (Object.getPrototypeOf(obj) &&
> (prop in Object.getPrototypeOf(obj)))
> On 2014/01/09 12:26:39, rossberg wrote:
> > Only do this fall-back when really necessary, i.e., if the inherited
property
> is
> > read-only (or a JS accessor, but I don't think that comes up here).
> > 
> > Nit: redundant parens
> 
> Done, I think (but it feels counterintuitive to "reuse" any property up the
> prototype chain in this test.)

I think there may be a misunderstanding. You don't reuse any inherited property
-- assignment always applies to the derived object, unless it inherits an
accessor. For data properties, only their read-onliness is inherited, not its
value. (Yes, JavaScript is nuts.)

[rossberg, 2014-01-09 14:59:35]

https://codereview.chromium.org/116083006/diff/90001/test/mjsunit/harmony/obj...
File test/mjsunit/harmony/object-observe.js (right):

https://codereview.chromium.org/116083006/diff/90001/test/mjsunit/harmony/obj...
test/mjsunit/harmony/object-observe.js:989: // prototype chain, only update it
if it has compatible attributes.
This comment seems misleading. You never update the inherited property, you only
make sure that assignment will create a new property on the child.

https://codereview.chromium.org/116083006/diff/90001/test/mjsunit/harmony/obj...
test/mjsunit/harmony/object-observe.js:997: if (!desc.configurable ||
!desc.writable || desc.set)
I think this should be more like

  if (!desc.writable || !('value' in desc))

because desc.set will be absent for read-only accessors; configurability doesn't
inherit.

In fact,

  if (!desc.writable)

should be enough, since this will already be true for accessor properties
(because their descriptor has no 'writable').

[sof, 2014-01-09 15:28:15]

On 2014/01/09 14:59:35, rossberg wrote:
>
https://codereview.chromium.org/116083006/diff/90001/test/mjsunit/harmony/obj...
> File test/mjsunit/harmony/object-observe.js (right):
> 
>
https://codereview.chromium.org/116083006/diff/90001/test/mjsunit/harmony/obj...
> test/mjsunit/harmony/object-observe.js:989: // prototype chain, only update it
> if it has compatible attributes.
> This comment seems misleading. You never update the inherited property, you
only
> make sure that assignment will create a new property on the child.
> 
>
https://codereview.chromium.org/116083006/diff/90001/test/mjsunit/harmony/obj...
> test/mjsunit/harmony/object-observe.js:997: if (!desc.configurable ||
> !desc.writable || desc.set)
> I think this should be more like
> 
>   if (!desc.writable || !('value' in desc))
> 
> because desc.set will be absent for read-only accessors; configurability
doesn't
> inherit.
> 
> In fact,
> 
>   if (!desc.writable)
> 
> should be enough, since this will already be true for accessor properties
> (because their descriptor has no 'writable').

Tweaked predicate and the comment just above.

[rossberg, 2014-01-09 15:32:06]

LGTM, will land soon

[sof, 2014-02-07 11:56:56]

On 2014/01/09 15:32:06, rossberg wrote:
> LGTM, will land soon

Delayed by other ES6 related changes?

[rossberg, 2014-02-07 14:55:57]

Committed patchset #3 manually as r19200 (presubmit successful).

[rossberg, 2014-02-07 14:57:31]

On 2014/02/07 11:56:56, sof wrote:
> On 2014/01/09 15:32:06, rossberg wrote:
> > LGTM, will land soon
> 
> Delayed by other ES6 related changes?

No, just by the fact that the tree was closed for weeks due to the upcoming
Chrome branch, then me being sick for a week, and then having forgotten about
it. :) Sorry, landed now.

[rossberg, 2014-02-07 14:58:46]

On 2014/02/07 14:57:31, rossberg wrote:
> On 2014/02/07 11:56:56, sof wrote:
> > On 2014/01/09 15:32:06, rossberg wrote:
> > > LGTM, will land soon
> > 
> > Delayed by other ES6 related changes?
> 
> No, just by the fact that the tree was closed for weeks due to the upcoming
> Chrome branch, then me being sick for a week, and then having forgotten about
> it. :) Sorry, landed now.

PS: Please don't hesitate to ping if we don't land within a couple of days.

[sof, 2014-02-07 15:03:15]

On 2014/02/07 14:58:46, rossberg wrote:
> On 2014/02/07 14:57:31, rossberg wrote:
> > On 2014/02/07 11:56:56, sof wrote:
> > > On 2014/01/09 15:32:06, rossberg wrote:
> > > > LGTM, will land soon
> > > 
> > > Delayed by other ES6 related changes?
> > 
> > No, just by the fact that the tree was closed for weeks due to the upcoming
> > Chrome branch, then me being sick for a week, and then having forgotten
about
> > it. :) Sorry, landed now.
> 
> PS: Please don't hesitate to ping if we don't land within a couple of days.

Excellent, thanks for doing that right away :)