Enable Object.observe by default

Enable Object.observe by default

R=rossberg@chromium.org, rossberg
BUG=v8:2409
LOG=Y

Committed: https://code.google.com/p/v8/source/detail?r=19734

[rafaelw, 2014-03-05 00:48:46]

Note that simply setting the flag to true doesn't work because of snapshotting
and enabling getting this working with snapshooting involved:

1) Moving the native functions out of experimental, which made the flag
pointless
2) Ensuring that no per-isolate state was allocated during the first pass of
object-observe.js (all is now lazily created).

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js
File src/object-observe.js (left):

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js#oldcode28
src/object-observe.js:28: "use strict";
it seems like all the built-in native js files are run as a single block and so
"use strict" causes errors elsewhere.

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (left):

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc#old...
test/cctest/test-heap.cc:2035: CHECK(proto->IsTheHole() || proto->IsJSObject());
It seems like the check for the hole here was actually wrong (would have allow
this test to pass if the storage *hadn't* been compacted.

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:2007: CompileRun("var base = {};");
Note: the way this test was structured was asserting more than it needed to.
Specifically it was asserting that no built in created a prototype transition,
which struct me as a needless restriction on implementing self-hosted code.

[rossberg, 2014-03-05 09:55:47]

Hi Rafael, this CL makes sense eventually, but right now it would be skipping
the ES-staging process that we have decided to take new features through.

I have implemented a respective flag two weeks ago
(https://codereview.chromium.org/165723008/), but couldn't land it yet, due to
the tree still being closed. However, I just got special sheriff permission to
do it. :)

Moving something away from under --harmony also requires moving associated JS
tests from harmony/ to an es6/ subdirectory yet to be created (or in your case,
es7/, actually). That is to allow ClusterFuzz to pick them up (which is intended
to skip harmony/).

[rossberg, 2014-03-06 08:41:35]

Toon, do you know more about whether prototype transitions would be an issue for
the snapshot? (See last comment)

https://codereview.chromium.org/183683022/diff/1/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/183683022/diff/1/src/accessors.cc#newcode623
src/accessors.cc:623: bool is_observed =
Nit: does this fit one line now?

https://codereview.chromium.org/183683022/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/183683022/diff/1/src/ic.cc#newcode1675
src/ic.cc:1675: JSObject::cast(*object)->map()->is_observed());
Nit: does it fit previous line?

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js
File src/object-observe.js (left):

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js#oldcode28
src/object-observe.js:28: "use strict";
On 2014/03/05 00:48:46, rafaelw wrote:
> it seems like all the built-in native js files are run as a single block and
so
> "use strict" causes errors elsewhere.

That's surprising. The directive here should only have an effect at all if this
becomes the very first file in that block.

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:2007: CompileRun("var base = {};");
On 2014/03/05 00:48:46, rafaelw wrote:
> Note: the way this test was structured was asserting more than it needed to.
> Specifically it was asserting that no built in created a prototype transition,
> which struct me as a needless restriction on implementing self-hosted code.

Sounds plausible, although you never know if there is a hidden reason in V8.
Invoking Toon, he might know more.

[Toon Verwaest, 2014-03-06 13:01:25]

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/183683022/diff/1/test/cctest/test-heap.cc#new...
test/cctest/test-heap.cc:2007: CompileRun("var base = {};");
Sounds good to me.

On 2014/03/06 08:41:35, rossberg wrote:
> On 2014/03/05 00:48:46, rafaelw wrote:
> > Note: the way this test was structured was asserting more than it needed to.
> > Specifically it was asserting that no built in created a prototype
transition,
> > which struct me as a needless restriction on implementing self-hosted code.
> 
> Sounds plausible, although you never know if there is a hidden reason in V8.
> Invoking Toon, he might know more.

[rafaelw, 2014-03-07 04:33:06]

ptal

https://codereview.chromium.org/183683022/diff/1/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/183683022/diff/1/src/accessors.cc#newcode623
src/accessors.cc:623: bool is_observed =
Yes. done

On 2014/03/06 08:41:35, rossberg wrote:
> Nit: does this fit one line now?

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js
File src/object-observe.js (left):

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js#oldcode28
src/object-observe.js:28: "use strict";
You are right. I tried adding it back and got not failures (not sure what I was
seeing before that I thought this was happening). However, if you're right that
this directive is having no effect (or only an effective if it's the first
file), then it seems best to not have it, so I left it out.

On 2014/03/06 08:41:35, rossberg wrote:
> On 2014/03/05 00:48:46, rafaelw wrote:
> > it seems like all the built-in native js files are run as a single block and
> so
> > "use strict" causes errors elsewhere.
> 
> That's surprising. The directive here should only have an effect at all if
this
> becomes the very first file in that block.

[rossberg, 2014-03-07 07:14:44]

LGTM

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js
File src/object-observe.js (left):

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js#oldcode28
src/object-observe.js:28: "use strict";
On 2014/03/07 04:33:06, rafaelw wrote:
> You are right. I tried adding it back and got not failures (not sure what I
was
> seeing before that I thought this was happening). However, if you're right
that
> this directive is having no effect (or only an effective if it's the first
> file), then it seems best to not have it, so I left it out.

Please keep it nevertheless -- we have a midterm goal of moving all JS files to
strict mode, and this serves as a marker for the ones that already work (not too
many at the moment :( ).

[rafaelw, 2014-03-08 02:48:03]

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

[rafaelw, 2014-03-08 02:48:19]

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js
File src/object-observe.js (left):

https://codereview.chromium.org/183683022/diff/1/src/object-observe.js#oldcode28
src/object-observe.js:28: "use strict";
Ok. Seems wrong, but I'll leave it in.

On 2014/03/07 07:14:44, rossberg wrote:
> On 2014/03/07 04:33:06, rafaelw wrote:
> > You are right. I tried adding it back and got not failures (not sure what I
> was
> > seeing before that I thought this was happening). However, if you're right
> that
> > this directive is having no effect (or only an effective if it's the first
> > file), then it seems best to not have it, so I left it out.
> 
> Please keep it nevertheless -- we have a midterm goal of moving all JS files
to
> strict mode, and this serves as a marker for the ones that already work (not
too
> many at the moment :( ).