[es6] do not add caller/arguments to ES6 function definitions

[es6] do not add caller/arguments to ES6 function definitions

BUG=v8:3946, v8:3982
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
LOG=N
R=arv@chromium.org, rossberg@chromium.org

Committed: https://crrev.com/9836c34dba61b6bb7732fe78df3ff4e8a4509387
Cr-Commit-Position: refs/heads/master@{#27729}

[caitp (gmail), 2015-03-23 20:57:46]

PTAL --- this doesn't deal with bound functions, which apparently shouldn't have
these properties either.

Unfortunately, it means adding a number (4) of additional maps, and I couldn't
think of a better name for these than "unpoisoned" variants of other function
maps (even sloppy ones which weren't poisoned to begin with).

[caitp (gmail), 2015-03-23 20:58:54]

caitpotter88@gmail.com changed reviewers:
+ dslomov@chromium.org, rossberg@chromium.org

[arv (Not doing code reviews), 2015-03-24 11:30:50]

It is not clear why we need 4 new maps? Can you list the different cases of
functions and how their maps differ?

[caitp (gmail), 2015-03-24 11:35:02]

On 2015/03/24 11:30:50, arv wrote:
> It is not clear why we need 4 new maps? Can you list the different cases of
> functions and how their maps differ?

SLOPPY_FUNCTION_MAP and SLOPPY_FUNCTION_WITHOUT_PROTOTYPE_MAP install
caller/arguments accessors as own properties, which is forbidden. The strict
variants of these install the poisoned accessors as own properties, which is
also forbidden. For legacy syntactic forms, this behaviour is still needed ---
so it seems that you need the extra maps.

If the global maps weren't set up, and the accessors were added during function
instantiation, you'd still end up with new maps implicitly.

[caitp (gmail), 2015-03-24 11:36:23]

On 2015/03/24 11:35:02, caitp wrote:
> On 2015/03/24 11:30:50, arv wrote:
> > It is not clear why we need 4 new maps? Can you list the different cases of
> > functions and how their maps differ?
> 
> SLOPPY_FUNCTION_MAP and SLOPPY_FUNCTION_WITHOUT_PROTOTYPE_MAP install
> caller/arguments accessors as own properties, which is forbidden. The strict
> variants of these install the poisoned accessors as own properties, which is
> also forbidden. For legacy syntactic forms, this behaviour is still needed ---
> so it seems that you need the extra maps.
> 
> If the global maps weren't set up, and the accessors were added during
function
> instantiation, you'd still end up with new maps implicitly.

It may be possible to just reuse the strong mode variants, but that may have
other implications

[rossberg, 2015-03-24 11:40:08]

On 2015/03/24 11:36:23, caitp wrote:
> It may be possible to just reuse the strong mode variants, but that may have
> other implications

Yes, that won't be right. Those will specifically be marked strong in the
future, with implications on the object semantics.

[caitp (gmail), 2015-03-24 13:21:31]

On 2015/03/24 11:40:08, rossberg wrote:
> On 2015/03/24 11:36:23, caitp wrote:
> > It may be possible to just reuse the strong mode variants, but that may have
> > other implications
> 
> Yes, that won't be right. Those will specifically be marked strong in the
> future, with implications on the object semantics.

PTAL --- no more strict/sloppy variants of the unpoisoned stuff, tests passing

[arv (Not doing code reviews), 2015-03-25 13:36:47]

https://codereview.chromium.org/1027283004/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1027283004/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:301: Handle<Map> plain_function_map_writable_prototype_;
Can you rename these. Right now it is not clear how there can be anything but
sloppy and strict.

The concept of "plain" function is something new here. It is never defined what
it means.

https://codereview.chromium.org/1027283004/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:415: if (add_restricted_props) {  // Add caller.
Maybe

if (add_restricted_props) {
  {
    ...
  }
  {
    ...
}

to remove a branch.

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
File test/mjsunit/es6/generators-poisoned-properties.js (right):

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
test/mjsunit/es6/generators-poisoned-properties.js:7: assertEquals(false,
(function*() {}).hasOwnProperty("arguments"));
assertFalse

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
File test/mjsunit/es6/generators-runtime.js (right):

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
removePoisoned(Object.getOwnPropertyNames(f));
Would this test be cleaner if f was strict?

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/harmony/cl...
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/harmony/cl...
test/mjsunit/harmony/classes.js:903: }
Maybe have a derived class too?

[caitp (gmail), 2015-03-25 13:42:59]

https://codereview.chromium.org/1027283004/diff/60001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1027283004/diff/60001/src/bootstrapper.cc#new...
src/bootstrapper.cc:301: Handle<Map> plain_function_map_writable_prototype_;
On 2015/03/25 13:36:47, arv wrote:
> Can you rename these. Right now it is not clear how there can be anything but
> sloppy and strict.
> 
> The concept of "plain" function is something new here. It is never defined
what
> it means.

what would be better here? "newstyle_..."? "unpoisoned_..."?
"without_restricted_properties..."? (just asking for opinions, "plain" seemed to
convey the "no restricted properties" thing nicely)

[caitp (gmail), 2015-03-25 13:58:09]

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
File test/mjsunit/es6/generators-runtime.js (right):

https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
removePoisoned(Object.getOwnPropertyNames(f));
On 2015/03/25 13:36:47, arv wrote:
> Would this test be cleaner if f was strict?

a strict ordinary function still has the properties, they just throw --- unless
you mean the name "removePoisoned" is confusing, since they aren't poisoned in
sloppy fns

[arv (Not doing code reviews), 2015-03-25 14:14:36]

On 2015/03/25 13:58:09, caitp wrote:
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> File test/mjsunit/es6/generators-runtime.js (right):
> 
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
> removePoisoned(Object.getOwnPropertyNames(f));
> On 2015/03/25 13:36:47, arv wrote:
> > Would this test be cleaner if f was strict?
> 
> a strict ordinary function still has the properties, they just throw ---
unless
> you mean the name "removePoisoned" is confusing, since they aren't poisoned in
> sloppy fns

You are right... I was not thinking through the cases correctly.

The functions we have are:

Strict with restricted properties
Strict without restricted properties
Sloppy with those stupid properties

Maybe the Maps should be name strict_restricted, strict_plain and sloppy?

[caitp (gmail), 2015-03-25 14:18:45]

On 2015/03/25 14:14:36, arv wrote:
> On 2015/03/25 13:58:09, caitp wrote:
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > File test/mjsunit/es6/generators-runtime.js (right):
> > 
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
> > removePoisoned(Object.getOwnPropertyNames(f));
> > On 2015/03/25 13:36:47, arv wrote:
> > > Would this test be cleaner if f was strict?
> > 
> > a strict ordinary function still has the properties, they just throw ---
> unless
> > you mean the name "removePoisoned" is confusing, since they aren't poisoned
in
> > sloppy fns
> 
> You are right... I was not thinking through the cases correctly.
> 
> The functions we have are:
> 
> Strict with restricted properties
> Strict without restricted properties
> Sloppy with those stupid properties
> 
> Maybe the Maps should be name strict_restricted, strict_plain and sloppy?

New syntactic forms do not have the properties, regardless of the language mode
(paraphrasing 16.1) --- so strict_plain and sloppy_plain both exist, but they're
essentially the same thing. Maybe "strict_restricted" and "sloppy_restricted"
and "unrestricted" are good name variations?

[arv (Not doing code reviews), 2015-03-25 14:28:38]

On 2015/03/25 14:18:45, caitp wrote:
> On 2015/03/25 14:14:36, arv wrote:
> > On 2015/03/25 13:58:09, caitp wrote:
> > >
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > > File test/mjsunit/es6/generators-runtime.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > > test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
> > > removePoisoned(Object.getOwnPropertyNames(f));
> > > On 2015/03/25 13:36:47, arv wrote:
> > > > Would this test be cleaner if f was strict?
> > > 
> > > a strict ordinary function still has the properties, they just throw ---
> > unless
> > > you mean the name "removePoisoned" is confusing, since they aren't
poisoned
> in
> > > sloppy fns
> > 
> > You are right... I was not thinking through the cases correctly.
> > 
> > The functions we have are:
> > 
> > Strict with restricted properties
> > Strict without restricted properties
> > Sloppy with those stupid properties
> > 
> > Maybe the Maps should be name strict_restricted, strict_plain and sloppy?

> Maybe "strict_restricted" and "sloppy_restricted"
> and "unrestricted" are good name variations?

Those names are good. They give good hints.

[caitp (gmail), 2015-03-25 15:01:20]

On 2015/03/25 14:28:38, arv wrote:
> On 2015/03/25 14:18:45, caitp wrote:
> > On 2015/03/25 14:14:36, arv wrote:
> > > On 2015/03/25 13:58:09, caitp wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > > > File test/mjsunit/es6/generators-runtime.js (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > > > test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
> > > > removePoisoned(Object.getOwnPropertyNames(f));
> > > > On 2015/03/25 13:36:47, arv wrote:
> > > > > Would this test be cleaner if f was strict?
> > > > 
> > > > a strict ordinary function still has the properties, they just throw ---
> > > unless
> > > > you mean the name "removePoisoned" is confusing, since they aren't
> poisoned
> > in
> > > > sloppy fns
> > > 
> > > You are right... I was not thinking through the cases correctly.
> > > 
> > > The functions we have are:
> > > 
> > > Strict with restricted properties
> > > Strict without restricted properties
> > > Sloppy with those stupid properties
> > > 
> > > Maybe the Maps should be name strict_restricted, strict_plain and sloppy?
> 
> > Maybe "strict_restricted" and "sloppy_restricted"
> > and "unrestricted" are good name variations?
> 
> Those names are good. They give good hints.

done

[caitp (gmail), 2015-03-25 15:05:58]

Patchset #10 (id:180001) has been deleted

[caitp (gmail), 2015-03-25 15:36:20]

On 2015/03/25 15:01:20, caitp wrote:
> On 2015/03/25 14:28:38, arv wrote:
> > On 2015/03/25 14:18:45, caitp wrote:
> > > On 2015/03/25 14:14:36, arv wrote:
> > > > On 2015/03/25 13:58:09, caitp wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > > > > File test/mjsunit/es6/generators-runtime.js (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1027283004/diff/60001/test/mjsunit/es6/genera...
> > > > > test/mjsunit/es6/generators-runtime.js:50: var f_own_property_names =
> > > > > removePoisoned(Object.getOwnPropertyNames(f));
> > > > > On 2015/03/25 13:36:47, arv wrote:
> > > > > > Would this test be cleaner if f was strict?
> > > > > 
> > > > > a strict ordinary function still has the properties, they just throw
---
> > > > unless
> > > > > you mean the name "removePoisoned" is confusing, since they aren't
> > poisoned
> > > in
> > > > > sloppy fns
> > > > 
> > > > You are right... I was not thinking through the cases correctly.
> > > > 
> > > > The functions we have are:
> > > > 
> > > > Strict with restricted properties
> > > > Strict without restricted properties
> > > > Sloppy with those stupid properties
> > > > 
> > > > Maybe the Maps should be name strict_restricted, strict_plain and
sloppy?
> > 
> > > Maybe "strict_restricted" and "sloppy_restricted"
> > > and "unrestricted" are good name variations?
> > 
> > Those names are good. They give good hints.
> 
> done

You know what though, it looks like the stuff for poisoning these properties in
strict functions was completely removed from the spec --- it only seems to apply
to "intrinsics", which I guess is any builtin function. So maybe a few more maps
can be gotten rid of --- but that change seems bigger, so maybe it can wait

[caitp (gmail), 2015-03-25 17:08:33]

On 2015/03/25 15:36:20, caitp wrote:
> You know what though, it looks like the stuff for poisoning these properties
in
> strict functions was completely removed from the spec --- it only seems to
apply
> to "intrinsics", which I guess is any builtin function. So maybe a few more
maps
> can be gotten rid of --- but that change seems bigger, so maybe it can wait

The rationale behind this seems to be here
https://github.com/rwaldron/tc39-notes/blob/46d2396e02fd73121b5985d5a0fafbcdb...
--- that looks like more or less where the spec is at now. If the intrinsic
strict empty function's arguments/caller properties are throwers, then I guess
`caller/arguments` still throw for strict variants of new syntactic forms ---
they're just not own properties?

It's sort of confusing

[caitp (gmail), 2015-03-25 22:50:21]

H'okay --- here's a new variation, I've tried to go as close to my understanding
of the draft as possible:

- No "own" "caller" or "callee" on strict functions or new syntactic forms, at
all
- The intrinsic empty function has poison pills --- these get shadowed by sloppy
function maps for the cases where they exist (not spec'd for ES6, but it's an
extension that people use, I guess --- getting rid of these would allow even
fewer prototypes)
- GeneratorFunction's prototype property is spec'd as writable: false --- it
would have used writable: false in strict mode before, but now the strict
version is the only one possible
- Generator methods no longer have a prototype property (approximating the
"non-constructor" kind, sort of --- appropriate for methods)
- maybe some other nicknacks here or there, but I think that's all of them

[caitp (gmail), 2015-03-26 00:32:19]

On 2015/03/25 22:50:21, caitp wrote:
> H'okay --- here's a new variation, I've tried to go as close to my
understanding
> of the draft as possible:
> 
> - No "own" "caller" or "callee" on strict functions or new syntactic forms, at
> all
> - The intrinsic empty function has poison pills --- these get shadowed by
sloppy
> function maps for the cases where they exist (not spec'd for ES6, but it's an
> extension that people use, I guess --- getting rid of these would allow even
> fewer prototypes)
> - GeneratorFunction's prototype property is spec'd as writable: false --- it
> would have used writable: false in strict mode before, but now the strict
> version is the only one possible
> - Generator methods no longer have a prototype property (approximating the
> "non-constructor" kind, sort of --- appropriate for methods)
> - maybe some other nicknacks here or there, but I think that's all of them

Eh, I guess generator methods are constructible, sooooo rolling back some of
those changes.

[caitp (gmail), 2015-03-26 03:13:38]

Patchset #16 (id:310001) has been deleted

[caitp (gmail), 2015-03-26 03:38:16]

Patchset #16 (id:330001) has been deleted

[arv (Not doing code reviews), 2015-03-26 12:46:43]

LGTM

Andreas, can you take a look too?

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1273: Handle<Map> map = use_empty_function_map
What does "empty function" mean here? It does not fit the actual code very well.

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1286: ? isolate()->strict_function_map()
Is there no strict function with readonly prototype?

[caitp (gmail), 2015-03-26 13:00:17]

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1273: Handle<Map> map = use_empty_function_map
On 2015/03/26 12:46:43, arv wrote:
> What does "empty function" mean here? It does not fit the actual code very
well.

originally I was saving the map of the empty function in here --- I guess
"strict" works too though, maybe more clearly

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1286: ? isolate()->strict_function_map()
On 2015/03/26 12:46:43, arv wrote:
> Is there no strict function with readonly prototype?

well, technically strict_function_map() IS a readonly prototype --- the writable
prototype version is only kept around in the bootstrapper, though

[caitp (gmail), 2015-03-26 13:03:02]

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1286: ? isolate()->strict_function_map()
On 2015/03/26 13:00:17, caitp wrote:
> On 2015/03/26 12:46:43, arv wrote:
> > Is there no strict function with readonly prototype?
> 
> well, technically strict_function_map() IS a readonly prototype --- the
writable
> prototype version is only kept around in the bootstrapper, though

wait, I'm wrong --- it's readonly during bootstrapping, and subsequently is
read/write (overwritten with the writable version map). so strict builtins can
have the readonly mode

[arv (Not doing code reviews), 2015-03-26 13:03:30]

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1273: Handle<Map> map = use_empty_function_map
On 2015/03/26 13:00:17, caitp wrote:
> On 2015/03/26 12:46:43, arv wrote:
> > What does "empty function" mean here? It does not fit the actual code very
> well.
> 
> originally I was saving the map of the empty function in here --- I guess
> "strict" works too though, maybe more clearly

strict sounds more correct here.

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1286: ? isolate()->strict_function_map()
On 2015/03/26 13:00:17, caitp wrote:
> On 2015/03/26 12:46:43, arv wrote:
> > Is there no strict function with readonly prototype?
> 
> well, technically strict_function_map() IS a readonly prototype --- the
writable
> prototype version is only kept around in the bootstrapper, though

Maybe add a DCHECK to make sure we don't get the invalid case here?

[caitp (gmail), 2015-03-26 13:46:01]

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/factory.cc#newcode...
src/factory.cc:1286: ? isolate()->strict_function_map()
On 2015/03/26 13:03:30, arv wrote:
> On 2015/03/26 13:00:17, caitp wrote:
> > On 2015/03/26 12:46:43, arv wrote:
> > > Is there no strict function with readonly prototype?
> > 
> > well, technically strict_function_map() IS a readonly prototype --- the
> writable
> > prototype version is only kept around in the bootstrapper, though
> 
> Maybe add a DCHECK to make sure we don't get the invalid case here?

Done --- the DCHECK is now "!strict || !readonly_prototype ||
CheckBootstrapping()", which seems to work well enough (there's a bit of
granularity where it could potentially be wrong sometimes, but it's probably
unlikely since this isn't needed much).

I guess another alternative is just adding the rw and ro variants of the map to
Context, rather than having just one with different meanings at different times

[arv (Not doing code reviews), 2015-03-26 13:59:14]

https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc#newcode...
src/factory.cc:1284: // In strict mode, readonly strict map is only available
during bootstrap
I still find this confusing :-)

strict map with readonly prototype is what we use for classes for example.

[caitp (gmail), 2015-03-26 14:08:46]

https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc#newcode...
src/factory.cc:1284: // In strict mode, readonly strict map is only available
during bootstrap
On 2015/03/26 13:59:14, arv wrote:
> I still find this confusing :-)
> 
> strict map with readonly prototype is what we use for classes for example.

That seems to happen in Runtime_DefineClass --- the constructor's "prototype"
property is explicitly set to [[Enumerable]]=false, [[Writable]]=false,
[[Configurable]]=false --- In strong mode this is probably not necessary (strong
mode has its own constructor map variant), but in strict mode it just uses the
strict function map, which for most of the life of the engine will have a
writable prototype

[caitp (gmail), 2015-03-26 14:10:32]

On 2015/03/26 14:08:46, caitp wrote:
> https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc
> File src/factory.cc (right):
> 
>
https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc#newcode...
> src/factory.cc:1284: // In strict mode, readonly strict map is only available
> during bootstrap
> On 2015/03/26 13:59:14, arv wrote:
> > I still find this confusing :-)
> > 
> > strict map with readonly prototype is what we use for classes for example.
> 
> That seems to happen in Runtime_DefineClass --- the constructor's "prototype"
> property is explicitly set to [[Enumerable]]=false, [[Writable]]=false,
> [[Configurable]]=false --- In strong mode this is probably not necessary
(strong
> mode has its own constructor map variant), but in strict mode it just uses the
> strict function map, which for most of the life of the engine will have a
> writable prototype

oop, it looks like the strong constructor map doesn't deal with the prototype
property descriptor

[arv (Not doing code reviews), 2015-03-26 14:13:49]

On 2015/03/26 14:08:46, caitp wrote:
> https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc
> File src/factory.cc (right):
> 
>
https://codereview.chromium.org/1027283004/diff/370001/src/factory.cc#newcode...
> src/factory.cc:1284: // In strict mode, readonly strict map is only available
> during bootstrap
> On 2015/03/26 13:59:14, arv wrote:
> > I still find this confusing :-)
> > 
> > strict map with readonly prototype is what we use for classes for example.
> 
> That seems to happen in Runtime_DefineClass --- the constructor's "prototype"
> property is explicitly set to [[Enumerable]]=false, [[Writable]]=false,
> [[Configurable]]=false --- In strong mode this is probably not necessary
(strong
> mode has its own constructor map variant), but in strict mode it just uses the
> strict function map, which for most of the life of the engine will have a
> writable prototype

That is a good point. The current class definition code does not create the
function with a readonly prototype but sets the prototype and then make that
readonly.

[rossberg, 2015-03-26 15:06:45]

Look mostly good

https://codereview.chromium.org/1027283004/diff/350001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:360: static const bool kInstallConstructor = false;
Nit: I'm not superexcited about this change. When the need for more clarity for
such parameters comes up, we either put in comments, or define a proper enum.

https://codereview.chromium.org/1027283004/diff/350001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/350001/src/contexts.h#newcode179
src/contexts.h:179: V(GENERATOR_FUNCTION_MAP_INDEX, Map, generator_function_map)
                \
For symmetry and possibly future uses, I would prepare to keep these separate.

[rossberg, 2015-03-26 15:07:14]

On 2015/03/26 15:06:45, rossberg wrote:
> For symmetry and possibly future uses, I would prepare to keep these separate.

Doh, s/prepare/prefer/

[caitp (gmail), 2015-03-26 15:09:46]

https://codereview.chromium.org/1027283004/diff/350001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1027283004/diff/350001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:360: static const bool kInstallConstructor = false;
On 2015/03/26 15:06:45, rossberg wrote:
> Nit: I'm not superexcited about this change. When the need for more clarity
for
> such parameters comes up, we either put in comments, or define a proper enum.

I'm not keen on it either --- but changing it to an enum seems like a lot more
work for this CL. Would you mind if I did that in a followup?

https://codereview.chromium.org/1027283004/diff/350001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/350001/src/contexts.h#newcode179
src/contexts.h:179: V(GENERATOR_FUNCTION_MAP_INDEX, Map, generator_function_map)
                \
On 2015/03/26 15:06:45, rossberg wrote:
> For symmetry and possibly future uses, I would prepare to keep these separate.

Acknowledged.

[caitp (gmail), 2015-03-26 15:35:47]

https://codereview.chromium.org/1027283004/diff/350001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/350001/src/contexts.h#newcode179
src/contexts.h:179: V(GENERATOR_FUNCTION_MAP_INDEX, Map, generator_function_map)
                \
On 2015/03/26 15:09:46, caitp wrote:
> On 2015/03/26 15:06:45, rossberg wrote:
> > For symmetry and possibly future uses, I would prepare to keep these
separate.
> 
> Acknowledged.

Done, in fact

[rossberg, 2015-03-26 15:44:21]

lgtm

[caitp (gmail), 2015-03-26 15:45:25]

On 2015/03/26 15:44:21, rossberg wrote:
> lgtm

Thanks -- I think will wait for Toon's CL to land before CQ-ing this, since
that's going to cause some bitrot

[caitp (gmail), 2015-03-26 17:21:11]

On 2015/03/26 15:45:25, caitp wrote:
> On 2015/03/26 15:44:21, rossberg wrote:
> > lgtm
> 
> Thanks -- I think will wait for Toon's CL to land before CQ-ing this, since
> that's going to cause some bitrot

err---jakob's** CL

[caitp (gmail), 2015-03-26 17:25:34]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-03-26 17:25:35]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1027283004/#ps430001
(title: "Keep the old strict/sloppy generator maps")

[commit-bot: I haz the power, 2015-03-26 17:26:01]

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

[caitp (gmail), 2015-03-26 17:33:51]

The CQ bit was unchecked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-03-26 17:33:58]

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

[commit-bot: I haz the power, 2015-03-26 17:33:59]

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

[caitp (gmail), 2015-03-26 17:35:54]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-03-26 17:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1027283004/#ps450001
(title: "try to make the dcheck work")

[commit-bot: I haz the power, 2015-03-26 17:36:22]

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

[caitp (gmail), 2015-03-26 17:41:48]

Patchset #21 (id:450001) has been deleted

[caitp (gmail), 2015-03-26 17:42:05]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-03-26 17:42:05]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1027283004/#ps470001
(title: "try it again")

[commit-bot: I haz the power, 2015-03-26 17:42:21]

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

[commit-bot: I haz the power, 2015-03-26 18:17:13]

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

[commit-bot: I haz the power, 2015-03-26 18:17:15]

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

[caitp (gmail), 2015-03-26 18:20:32]

On 2015/03/26 18:17:15, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   v8_win64_rel on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel/builds/3331)

I guess this breaks webkit tests, and probably test262 too. I'm fixing the
webkit tests and adding test262 exceptions if any are needed, but I'm not sure
the best approach to fixing the layout tests in blink when v8 rolls

[Jakob Kummerow, 2015-03-26 21:29:56]

On 2015/03/26 18:20:32, caitp wrote:
> I'm not sure
> the best approach to fixing the layout tests in blink when v8 rolls

This may be a reason to put this CL on ice until after the branch (which happens
end of next week). We have a bunch of important but risky stuff going in until
then, and absolutely need to be able to roll V8 back and forth as stability
demands.

Also, the layout tests must be green already when a CL lands, otherwise they'll
block the auto-roller and V8 will never roll into Chromium.

[caitp (gmail), 2015-03-26 22:18:13]

On 2015/03/26 21:29:56, Jakob wrote:
> On 2015/03/26 18:20:32, caitp wrote:
> > I'm not sure
> > the best approach to fixing the layout tests in blink when v8 rolls
> 
> This may be a reason to put this CL on ice until after the branch (which
happens
> end of next week). We have a bunch of important but risky stuff going in until
> then, and absolutely need to be able to roll V8 back and forth as stability
> demands.
> 
> Also, the layout tests must be green already when a CL lands, otherwise
they'll
> block the auto-roller and V8 will never roll into Chromium.

I've run the blink layout tests against a fresh build with this patch applied,
there are a number of failures but they don't seem obviously related to this..
will try it and see if it needs to be reverted, I guess

[caitp (gmail), 2015-03-26 22:19:07]

On 2015/03/26 22:18:13, caitp wrote:
> On 2015/03/26 21:29:56, Jakob wrote:
> > On 2015/03/26 18:20:32, caitp wrote:
> > > I'm not sure
> > > the best approach to fixing the layout tests in blink when v8 rolls
> > 
> > This may be a reason to put this CL on ice until after the branch (which
> happens
> > end of next week). We have a bunch of important but risky stuff going in
until
> > then, and absolutely need to be able to roll V8 back and forth as stability
> > demands.
> > 
> > Also, the layout tests must be green already when a CL lands, otherwise
> they'll
> > block the auto-roller and V8 will never roll into Chromium.
> 
> I've run the blink layout tests against a fresh build with this patch applied,
> there are a number of failures but they don't seem obviously related to this..
> will try it and see if it needs to be reverted, I guess

---

well, or it can go on ice for a bit, either way is fine

[caitp (gmail), 2015-03-27 08:53:31]

On 2015/03/26 22:19:07, caitp wrote:
> On 2015/03/26 22:18:13, caitp wrote:
> > On 2015/03/26 21:29:56, Jakob wrote:
> > > On 2015/03/26 18:20:32, caitp wrote:
> > > > I'm not sure
> > > > the best approach to fixing the layout tests in blink when v8 rolls
> > > 
> > > This may be a reason to put this CL on ice until after the branch (which
> > happens
> > > end of next week). We have a bunch of important but risky stuff going in
> until
> > > then, and absolutely need to be able to roll V8 back and forth as
stability
> > > demands.
> > > 
> > > Also, the layout tests must be green already when a CL lands, otherwise
> > they'll
> > > block the auto-roller and V8 will never roll into Chromium.
> > 
> > I've run the blink layout tests against a fresh build with this patch
applied,
> > there are a number of failures but they don't seem obviously related to
this..
> > will try it and see if it needs to be reverted, I guess
> 
> ---
> 
> well, or it can go on ice for a bit, either way is fine

@jkummerow I'm seeing the same layout test behaviour with a build with the patch
reverted, looks like incorrect baselines or something (in a small % of tests)...
so I think this one is pretty low-risk. That said, will wait anyways if you
prefer

[Jakob Kummerow, 2015-03-27 09:16:54]

> > well, or it can go on ice for a bit, either way is fine
> 
> @jkummerow I'm seeing the same layout test behaviour with a build with the
patch
> reverted, looks like incorrect baselines or something (in a small % of
tests)...
> so I think this one is pretty low-risk. That said, will wait anyways if you
> prefer

Right now we're having so much trouble getting our rolls to stick in Chromium
that I'd indeed prefer if any non-urgent behavior changes could be held back at
least over the weekend. At this time, I'd put it as "when you see a Canary go
out with V8 4.3.60 or later, then you can give this a shot."

[Dmitry Lomov (no reviews), 2015-03-27 10:41:53]

On 2015/03/27 09:16:54, Jakob wrote:
> > > well, or it can go on ice for a bit, either way is fine
> > 
> > @jkummerow I'm seeing the same layout test behaviour with a build with the
> patch
> > reverted, looks like incorrect baselines or something (in a small % of
> tests)...
> > so I think this one is pretty low-risk. That said, will wait anyways if you
> > prefer
> 
> Right now we're having so much trouble getting our rolls to stick in Chromium
> that I'd indeed prefer if any non-urgent behavior changes could be held back
at
> least over the weekend. At this time, I'd put it as "when you see a Canary go
> out with V8 4.3.60 or later, then you can give this a shot."

+1, let's hold off on anything that we can.

[Jakob Kummerow, 2015-04-07 10:49:52]

Branch has been made. Land this now, or rebase onto
https://codereview.chromium.org/1005393004/ ;-)

[caitp (gmail), 2015-04-07 14:27:19]

On 2015/04/07 10:49:52, Jakob wrote:
> Branch has been made. Land this now, or rebase onto
> https://codereview.chromium.org/1005393004/ ;-)

Yup --- arv@ do you want to give it one last look over before checking in?

[arv (Not doing code reviews), 2015-04-07 16:18:39]

We need to make sure length and name of the %ThrowTypeError% function are non
configurable and non writable.

I didn't see a test for that but maybe I just missed it?

> The length property of a %ThrowTypeError% function has the  attributes {
[[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }.

Also, https://bugs.ecmascript.org/show_bug.cgi?id=4267

https://codereview.chromium.org/1027283004/diff/530001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1027283004/diff/530001/src/builtins.cc#newcod...
src/builtins.cc:1019: NewTypeError("strict_poison_pill",
HandleVector<Object>(NULL, 0)));
maybe rename the message key now that we renamed the functions?

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h#newcode594
src/contexts.h:594: : is_strict(language_mode) || !IsAccessorFunction(kind)
I don't understand why accessor functions are special cased here?

Maybe restructure this code to make it clearer what is going on.

https://codereview.chromium.org/1027283004/diff/530001/test/mjsunit/harmony/c...
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/mjsunit/harmony/c...
test/mjsunit/harmony/classes.js:923: (function testRestrictedPropertiesSloppy()
{
Classes are always strict so I'm not sure this adds anything.

https://codereview.chromium.org/1027283004/diff/530001/test/mjsunit/strict-mo...
File test/mjsunit/strict-mode.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/mjsunit/strict-mo...
test/mjsunit/strict-mode.js:1027: assertFalse(d.enumerable);
we need to test name and length too but I'm fine deferring that to a followup
CL.

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/Ob...
File test/webkit/fast/js/Object-getOwnPropertyNames.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/Ob...
test/webkit/fast/js/Object-getOwnPropertyNames.js:63: "parseInt": "['arguments',
'caller', 'length', 'name']",
How come this still has arguments and caller?

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/ca...
File test/webkit/fast/js/caller-property-expected.txt (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/ca...
test/webkit/fast/js/caller-property-expected.txt:33: FAIL
nonStrictCaller(strictCallee) should throw TypeError: Type error. Threw
exception TypeError: 'caller' and 'arguments' are restricted function properties
and cannot be accessed in this context..
Maybe update the test for these?

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/strict-thr...
File test/webkit/strict-throw-type-error.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/strict-thr...
test/webkit/strict-throw-type-error.js:29:
Object.getOwnPropertyDescriptor(object, name).get;
This test is broken. getter always returns undefined and undefined === undefined
:-)

Consider deleting this test.

[caitp (gmail), 2015-04-07 16:29:58]

thanks for the look (I have some questions in here)

https://codereview.chromium.org/1027283004/diff/530001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1027283004/diff/530001/src/builtins.cc#newcod...
src/builtins.cc:1019: NewTypeError("strict_poison_pill",
HandleVector<Object>(NULL, 0)));
On 2015/04/07 16:18:39, arv wrote:
> maybe rename the message key now that we renamed the functions?

Acknowledged.

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h#newcode594
src/contexts.h:594: : is_strict(language_mode) || !IsAccessorFunction(kind)
On 2015/04/07 16:18:39, arv wrote:
> I don't understand why accessor functions are special cased here?
> 
> Maybe restructure this code to make it clearer what is going on.

It was to avoid breaking the old behaviour.

But I'm not sure, it looks like lexically declared accessors are always
MethodDefinitions now, so maybe that's a break from ES5? I'm not really sure if
they should be considered "new syntactic forms" or what

https://codereview.chromium.org/1027283004/diff/530001/test/mjsunit/harmony/c...
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/mjsunit/harmony/c...
test/mjsunit/harmony/classes.js:923: (function testRestrictedPropertiesSloppy()
{
On 2015/04/07 16:18:39, arv wrote:
> Classes are always strict so I'm not sure this adds anything.

you could say it verifies the logic of the map picking in contexts.h, and also
helps verify that classes continue to be 100% strict code. Should I toss it out?

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/Ob...
File test/webkit/fast/js/Object-getOwnPropertyNames.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/Ob...
test/webkit/fast/js/Object-getOwnPropertyNames.js:63: "parseInt": "['arguments',
'caller', 'length', 'name']",
On 2015/04/07 16:18:40, arv wrote:
> How come this still has arguments and caller?

for whatever reason, "parseInt" and "parseFloat" (and the rest of these which
still have the properties) are sloppy functions. But I guess that's a bug.

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/ca...
File test/webkit/fast/js/caller-property-expected.txt (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/ca...
test/webkit/fast/js/caller-property-expected.txt:33: FAIL
nonStrictCaller(strictCallee) should throw TypeError: Type error. Threw
exception TypeError: 'caller' and 'arguments' are restricted function properties
and cannot be accessed in this context..
On 2015/04/07 16:18:40, arv wrote:
> Maybe update the test for these?

Okay --- Is there a good reason this test is kept around? It seems redundant,
and it's no longer in blink's layout tests directory

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/strict-thr...
File test/webkit/strict-throw-type-error.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/strict-thr...
test/webkit/strict-throw-type-error.js:29:
Object.getOwnPropertyDescriptor(object, name).get;
On 2015/04/07 16:18:40, arv wrote:
> This test is broken. getter always returns undefined and undefined ===
undefined
> :-)
> 
> Consider deleting this test.

Acknowledged.

[arv (Not doing code reviews), 2015-04-07 17:00:12]

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h#newcode594
src/contexts.h:594: : is_strict(language_mode) || !IsAccessorFunction(kind)
On 2015/04/07 16:29:57, caitp wrote:
> On 2015/04/07 16:18:39, arv wrote:
> > I don't understand why accessor functions are special cased here?
> > 
> > Maybe restructure this code to make it clearer what is going on.
> 
> It was to avoid breaking the old behaviour.
> 
> But I'm not sure, it looks like lexically declared accessors are always
> MethodDefinitions now, so maybe that's a break from ES5? I'm not really sure
if
> they should be considered "new syntactic forms" or what

Accessors are considered Methods in ES6 so they have no prototype property.

I just don't understand why we would use a sloppy map for a strict mode accessor
function?

My suggestion was to rewrite this ternary into if statementest to make the logic
easier to follow.

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/Ob...
File test/webkit/fast/js/Object-getOwnPropertyNames.js (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/Ob...
test/webkit/fast/js/Object-getOwnPropertyNames.js:63: "parseInt": "['arguments',
'caller', 'length', 'name']",
On 2015/04/07 16:29:57, caitp wrote:
> On 2015/04/07 16:18:40, arv wrote:
> > How come this still has arguments and caller?
> 
> for whatever reason, "parseInt" and "parseFloat" (and the rest of these which
> still have the properties) are sloppy functions. But I guess that's a bug.

I think the spec allows this to be implementation defined but it seems bad that
we are inconsistent. This can be fixed in follow up CL. (Please file bugs so we
do not forget.)

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/ca...
File test/webkit/fast/js/caller-property-expected.txt (right):

https://codereview.chromium.org/1027283004/diff/530001/test/webkit/fast/js/ca...
test/webkit/fast/js/caller-property-expected.txt:33: FAIL
nonStrictCaller(strictCallee) should throw TypeError: Type error. Threw
exception TypeError: 'caller' and 'arguments' are restricted function properties
and cannot be accessed in this context..
On 2015/04/07 16:29:57, caitp wrote:
> On 2015/04/07 16:18:40, arv wrote:
> > Maybe update the test for these?
> 
> Okay --- Is there a good reason this test is kept around? It seems redundant,
> and it's no longer in blink's layout tests directory

We can remove it as long as we have other tests that cover the same code (which
I believe we do).

[caitp (gmail), 2015-04-07 17:10:36]

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h#newcode594
src/contexts.h:594: : is_strict(language_mode) || !IsAccessorFunction(kind)
On 2015/04/07 17:00:11, arv wrote:
> On 2015/04/07 16:29:57, caitp wrote:
> > On 2015/04/07 16:18:39, arv wrote:
> > > I don't understand why accessor functions are special cased here?
> > > 
> > > Maybe restructure this code to make it clearer what is going on.
> > 
> > It was to avoid breaking the old behaviour.
> > 
> > But I'm not sure, it looks like lexically declared accessors are always
> > MethodDefinitions now, so maybe that's a break from ES5? I'm not really sure
> if
> > they should be considered "new syntactic forms" or what
> 
> Accessors are considered Methods in ES6 so they have no prototype property.
> 
> I just don't understand why we would use a sloppy map for a strict mode
accessor
> function?

Oh I see what you're saying. So I guess I was trying to keep compat, and wound
up breaking it, my bad.

But my question is, should this be treated as a new syntactic form (like other
methods), or should the legacy behaviour be kept (where caller/arguments are
added in sloppy mode)

> My suggestion was to rewrite this ternary into if statementest to make the
logic
> easier to follow.

I try to follow the conventions used in the rest of the code, but I have no
problem with rewriting it as an if statement for clarity.

[arv (Not doing code reviews), 2015-04-07 18:22:03]

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h
File src/contexts.h (right):

https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h#newcode594
src/contexts.h:594: : is_strict(language_mode) || !IsAccessorFunction(kind)
On 2015/04/07 17:10:36, caitp wrote:
> On 2015/04/07 17:00:11, arv wrote:
> > On 2015/04/07 16:29:57, caitp wrote:
> > > On 2015/04/07 16:18:39, arv wrote:
> > > > I don't understand why accessor functions are special cased here?
> > > > 
> > > > Maybe restructure this code to make it clearer what is going on.
> > > 
> > > It was to avoid breaking the old behaviour.
> > > 
> > > But I'm not sure, it looks like lexically declared accessors are always
> > > MethodDefinitions now, so maybe that's a break from ES5? I'm not really
sure
> > if
> > > they should be considered "new syntactic forms" or what
> > 
> > Accessors are considered Methods in ES6 so they have no prototype property.
> > 
> > I just don't understand why we would use a sloppy map for a strict mode
> accessor
> > function?
> 
> Oh I see what you're saying. So I guess I was trying to keep compat, and wound
> up breaking it, my bad.
> 
> But my question is, should this be treated as a new syntactic form (like other
> methods), or should the legacy behaviour be kept (where caller/arguments are
> added in sloppy mode)

I think sloppy accessors should be treated as sloppy FunctionExpressions but it
is not clear to me if this is specified.

Maybe file a bug on ES6 and hope that Andre Bargul answers you :-)

[caitp (gmail), 2015-04-07 18:29:35]

On 2015/04/07 18:22:03, arv wrote:
> https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h
> File src/contexts.h (right):
> 
>
https://codereview.chromium.org/1027283004/diff/530001/src/contexts.h#newcode594
> src/contexts.h:594: : is_strict(language_mode) || !IsAccessorFunction(kind)
> On 2015/04/07 17:10:36, caitp wrote:
> > On 2015/04/07 17:00:11, arv wrote:
> > > On 2015/04/07 16:29:57, caitp wrote:
> > > > On 2015/04/07 16:18:39, arv wrote:
> > > > > I don't understand why accessor functions are special cased here?
> > > > > 
> > > > > Maybe restructure this code to make it clearer what is going on.
> > > > 
> > > > It was to avoid breaking the old behaviour.
> > > > 
> > > > But I'm not sure, it looks like lexically declared accessors are always
> > > > MethodDefinitions now, so maybe that's a break from ES5? I'm not really
> sure
> > > if
> > > > they should be considered "new syntactic forms" or what
> > > 
> > > Accessors are considered Methods in ES6 so they have no prototype
property.
> > > 
> > > I just don't understand why we would use a sloppy map for a strict mode
> > accessor
> > > function?
> > 
> > Oh I see what you're saying. So I guess I was trying to keep compat, and
wound
> > up breaking it, my bad.
> > 
> > But my question is, should this be treated as a new syntactic form (like
other
> > methods), or should the legacy behaviour be kept (where caller/arguments are
> > added in sloppy mode)
> 
> I think sloppy accessors should be treated as sloppy FunctionExpressions but
it
> is not clear to me if this is specified.
> 
> Maybe file a bug on ES6 and hope that Andre Bargul answers you :-)

you know what, I think the spec is actually pretty clear on this. " Such own
properties also must not be created for function objects defined using an
ArrowFunction, **MethodDefinition**, GeneratorDeclaration, GeneratorExpression,
ClassDeclaration, or ClassExpression regardless of whether the definition is
contained in strict mode code. "

This is a break from ES5, but so is strict functions not having own "caller" /
"arguments" properties, so it's a break either way. I guess the thing to do is
just take out the extra condition. Still want the if statement instead of the
ternary?

[arv (Not doing code reviews), 2015-04-07 18:31:15]

On 2015/04/07 18:29:35, caitp wrote:
> you know what, I think the spec is actually pretty clear on this. " Such own
> properties also must not be created for function objects defined using an
> ArrowFunction, **MethodDefinition**, GeneratorDeclaration,
GeneratorExpression,
> ClassDeclaration, or ClassExpression regardless of whether the definition is
> contained in strict mode code. "
> 
> This is a break from ES5, but so is strict functions not having own "caller" /
> "arguments" properties, so it's a break either way. I guess the thing to do is
> just take out the extra condition. Still want the if statement instead of the
> ternary?

Nice.

Most people never extract the functions from the accessors.

In ES6 we made the accessor functions non constructable (they have no prototype
property).

[caitp (gmail), 2015-04-07 19:33:08]

On 2015/04/07 18:31:15, arv wrote:
> On 2015/04/07 18:29:35, caitp wrote:
> > you know what, I think the spec is actually pretty clear on this. " Such own
> > properties also must not be created for function objects defined using an
> > ArrowFunction, **MethodDefinition**, GeneratorDeclaration,
> GeneratorExpression,
> > ClassDeclaration, or ClassExpression regardless of whether the definition is
> > contained in strict mode code. "
> > 
> > This is a break from ES5, but so is strict functions not having own "caller"
/
> > "arguments" properties, so it's a break either way. I guess the thing to do
is
> > just take out the extra condition. Still want the if statement instead of
the
> > ternary?
> 
> Nice.
> 
> Most people never extract the functions from the accessors.
> 
> In ES6 we made the accessor functions non constructable (they have no
prototype
> property).

2 webkit tests removed, accessor maps fixed per spec

[arv (Not doing code reviews), 2015-04-07 20:07:04]

LGTM

Consider adding the following before CQ'ing:

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

to the CL description since it is likely that there are Blink tests that needs
to be updated

[caitp (gmail), 2015-04-07 20:08:39]

On 2015/04/07 20:07:04, arv wrote:
> LGTM
> 
> Consider adding the following before CQ'ing:
> 
>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
> 
> to the CL description since it is likely that there are Blink tests that needs
> to be updated

thanks, will do.

I did run the layout tests locally and none of the failures seemed related to
the change though (and failed without the patch applied in the same way). So I'm
not sure any of the layout tests need fixes from this

[caitp (gmail), 2015-04-07 20:09:50]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-04-07 20:09:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1027283004/#ps550001
(title: "Delete some bad webkit tests, syntactic accessors are always strict")

[commit-bot: I haz the power, 2015-04-07 20:10:07]

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

[caitp (gmail), 2015-04-07 23:51:15]

On 2015/04/07 20:10:07, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1027283004/550001

I guess I'm wrong, few tests to fix :l

[commit-bot: I haz the power, 2015-04-08 00:02:09]

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

[commit-bot: I haz the power, 2015-04-08 00:02:11]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[caitp (gmail), 2015-04-08 00:04:20]

On 2015/04/08 00:02:11, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

https://chromium.googlesource.com/chromium/src/+/master/chrome/test/data/exte...
tries to install poison on all properties of all builtins --- so it's probably
safe to skip that for Function.(caller/arguments), since they're poisoned
anyways (and shouldn't break any tests then?)

I'll send a quick patch :l

[caitp (gmail), 2015-04-08 01:24:13]

On 2015/04/08 00:04:20, caitp wrote:
> On 2015/04/08 00:02:11, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
>
https://chromium.googlesource.com/chromium/src/+/master/chrome/test/data/exte...
> tries to install poison on all properties of all builtins --- so it's probably
> safe to skip that for Function.(caller/arguments), since they're poisoned
> anyways (and shouldn't break any tests then?)
> 
> I'll send a quick patch :l

Fix for the Chromium tests is in https://crrev.com/1067223002 if anyone wants to
poke that

[caitp (gmail), 2015-04-08 20:46:23]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-04-08 20:46:56]

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

[commit-bot: I haz the power, 2015-04-08 22:45:25]

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

[commit-bot: I haz the power, 2015-04-08 22:45:27]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

[caitp (gmail), 2015-04-09 01:38:02]

On 2015/04/08 22:45:27, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

so, I'm not sure.

1) add lines to LayoutTests/TestExpectations until a successful v8 roll, or
2) remove assertions about properties of Function/Function.prototype

2) would be easier, 1) would get better coverage for the inspector? I guess

[arv (Not doing code reviews), 2015-04-09 14:20:05]

On 2015/04/09 01:38:02, caitp wrote:
> On 2015/04/08 22:45:27, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)
> 
> so, I'm not sure.
> 
> 1) add lines to LayoutTests/TestExpectations until a successful v8 roll, or
> 2) remove assertions about properties of Function/Function.prototype
> 
> 2) would be easier, 1) would get better coverage for the inspector? I guess

My pattern has been to:

1. File a crbug about the broken tests.
2. Add an entry to TestExpectations, referencing the crbug
3. Once the v8 roll has happened go back and update the test and remove the
entry from TestExpectations.
4. Close bug.

Fun times!

[caitp (gmail), 2015-04-09 20:09:10]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-04-09 20:09:27]

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

[commit-bot: I haz the power, 2015-04-09 20:38:44]

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

[commit-bot: I haz the power, 2015-04-09 20:38:46]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

[caitp (gmail), 2015-04-09 20:43:46]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-04-09 20:43:46]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1027283004/#ps570001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-09 20:44:10]

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

[commit-bot: I haz the power, 2015-04-09 22:40:28]

Committed patchset #26 (id:570001)

[commit-bot: I haz the power, 2015-04-09 22:40:48]

Patchset 26 (id:??) landed as
https://crrev.com/9836c34dba61b6bb7732fe78df3ff4e8a4509387
Cr-Commit-Position: refs/heads/master@{#27729}