Remove Scope::scope_contains_with_ bit

Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/108efd7f544e540cc741fdc3b74fcda650bf0728
Cr-Commit-Position: refs/heads/master@{#34802}

[adamk, 2016-03-14 22:22:10]

adamk@chromium.org changed reviewers:
+ yangguo@chromium.org

[adamk, 2016-03-14 22:22:11]

Hi Yang,

I've added comments for each test modification. As noted, the one that doesn't
quite make sense to me is the scope_name stuff, hopefully you can comment on
that (and tell me if I'm wrong about any of the other bits).

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-evaluate...
File test/mjsunit/debug-evaluate-locals.js (left):

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-evaluate...
test/mjsunit/debug-evaluate-locals.js:92: // Frame 2 (f) has normal variables a
and b (and arguments).
No need to allocate arguments, as it's not referred to in the body and there's
no eval.

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-function...
File test/mjsunit/debug-function-scopes.js (left):

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-function...
test/mjsunit/debug-function-scopes.js:76: CheckScope(mirror.scope(2), { y: 17,
z: 22 }, ScopeType.Closure);
'y' is now in the same state as 'p', as it's not referred to by any inner
closure.

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.js
File test/mjsunit/debug-scopes.js (left):

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.j...
test/mjsunit/debug-scopes.js:840: debug.ScopeType.Closure,
My understanding is that we simply don't need this one anymore.

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.j...
test/mjsunit/debug-scopes.js:864: debug.ScopeType.Closure,
Same here.

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.j...
test/mjsunit/debug-scopes.js:870: CheckScopeChainNames(["inner", "inner",
"closure_in_with_2", "closure_in_with_2", undefined, undefined], exec_state)
This is the main bit I don't understand: why did we lose the name entirely?

[Yang, 2016-03-15 06:53:48]

LGTM if the bug explained below is fixed. And please run LayoutTests before you
commit.

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.js
File test/mjsunit/debug-scopes.js (left):

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.j...
test/mjsunit/debug-scopes.js:870: CheckScopeChainNames(["inner", "inner",
"closure_in_with_2", "closure_in_with_2", undefined, undefined], exec_state)
On 2016/03/14 22:22:11, adamk wrote:
> This is the main bit I don't understand: why did we lose the name entirely?

This seems to be a bug.

In ScopeIterator::MaterializeScopeDetails, we get the js_function value from the
context. If we don't have a context, the js_function is an empty handle. At this
point we decide which closure name the scope will have.

However, a couple lines below, we have another chance to fill js_function from
GetFunction(), which gives us the expected closure.

We should retrieve the name after that.

[adamk, 2016-03-15 17:45:46]

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.js
File test/mjsunit/debug-scopes.js (left):

https://codereview.chromium.org/1804783002/diff/1/test/mjsunit/debug-scopes.j...
test/mjsunit/debug-scopes.js:870: CheckScopeChainNames(["inner", "inner",
"closure_in_with_2", "closure_in_with_2", undefined, undefined], exec_state)
On 2016/03/15 06:53:48, Yang wrote:
> On 2016/03/14 22:22:11, adamk wrote:
> > This is the main bit I don't understand: why did we lose the name entirely?
> 
> This seems to be a bug.
> 
> In ScopeIterator::MaterializeScopeDetails, we get the js_function value from
the
> context. If we don't have a context, the js_function is an empty handle. At
this
> point we decide which closure name the scope will have.
> 
> However, a couple lines below, we have another chance to fill js_function from
> GetFunction(), which gives us the expected closure.
> 
> We should retrieve the name after that.

Thanks for the pointer, just moving the code down seems to pass all our tests.

Will make sure that devtools tests pass before landing.

[adamk, 2016-03-15 17:46:45]

Description was changed from

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
==========

to

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
==========

[adamk, 2016-03-15 21:44:30]

Description was changed from

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
==========

to

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[adamk, 2016-03-15 21:44:33]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-03-15 21:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1804783002/#ps20001
(title: "Fix scope names and GCC build")

[commit-bot: I haz the power, 2016-03-15 21:44:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1804783002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1804783002/20001

[commit-bot: I haz the power, 2016-03-15 22:41:19]

Description was changed from

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-15 22:41:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-15 22:42:10]

Description was changed from

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/108efd7f544e540cc741fdc3b74fcda650bf0728
Cr-Commit-Position: refs/heads/master@{#34802}
==========

[commit-bot: I haz the power, 2016-03-15 22:42:11]

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