[inspector] removed call break location from for-of loop

[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

Additionally location for for..of loops is moved from before "of" to before each variable expression.

We should not report first implicit call to avoid user confusion. User still able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org,jgruber@chromium.org

Review-Url: https://codereview.chromium.org/2893313002
Cr-Commit-Position: refs/heads/master@{#45509}
Committed: https://chromium.googlesource.com/v8/v8/+/fb78710c06743a19fb7161794ee611527a8be1af

[kozy, 2017-05-22 14:53:05]

Patchset #1 (id:1) has been deleted

[kozy, 2017-05-22 14:57:50]

Description was changed from

==========
[inspector] removed call break location from for-of loop

This location is produced by desugaring - V8 calls iterator.next internally.

BUG=v8:6425
==========

to

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org
==========

[kozy, 2017-05-22 14:57:51]

kozyatinskiy@chromium.org changed reviewers:
+ dgozman@chromium.org

[kozy, 2017-05-22 14:59:07]

Description was changed from

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org
==========

to

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org,jgruber@chromium.org
==========

[kozy, 2017-05-22 14:59:07]

kozyatinskiy@chromium.org changed reviewers:
+ jgruber@chromium.org

[kozy, 2017-05-22 15:00:04]

Dmitry and Jacob, please take a look. You can compare inspector test
expectations with 1 patchset to see a difference between expectations with and
without CL.

[kozy, 2017-05-22 15:00:09]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 15:00:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 15:28:32]

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

[commit-bot: I haz the power, 2017-05-22 15:28:33]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-05-22 23:00:36]

Inspector part lgtm, but I think we can go even further.

https://codereview.chromium.org/2893313002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/for-of-loops-expected.txt (right):

https://codereview.chromium.org/2893313002/diff/40001/test/inspector/debugger...
test/inspector/debugger/for-of-loops-expected.txt:10: for (var k |_|of |_|arr) {
|_|all.|C|push(k); }
Can we align location with for-in? Do we even need two locations here at all?

[kozy, 2017-05-23 11:44:43]

all done, thanks.

https://codereview.chromium.org/2893313002/diff/40001/test/inspector/debugger...
File test/inspector/debugger/for-of-loops-expected.txt (right):

https://codereview.chromium.org/2893313002/diff/40001/test/inspector/debugger...
test/inspector/debugger/for-of-loops-expected.txt:10: for (var k |_|of |_|arr) {
|_|all.|C|push(k); }
On 2017/05/22 23:00:36, dgozman wrote:
> Can we align location with for-in? Do we even need two locations here at all?

Done.

Reason for second position is loading arr variable, I'll fix it in later CLs.

[jgruber, 2017-05-23 12:33:56]

jgruber@chromium.org changed reviewers:
+ marja@chromium.org

[jgruber, 2017-05-23 12:33:57]

LGTM but please wait for 

+marja to take a look at parser/.

https://codereview.chromium.org/2893313002/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2893313002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:5663: final_loop =
impl()->InitializeForEachStatement(loop, each_variable,
I'm trying to understand the logic change this CL implements; so here we will
create a break location since each_variable's location is initialized by
DesugarBindingInForEachStatement, and break locations depend on whether source
locations are set.

https://codereview.chromium.org/2893313002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:5722: impl()->InitializeForEachStatement(loop,
expression, enumerable, body);
While here it all depends on whether expression's location has been initialized.
That's pretty subtle - I still don't understand if there's a logic change here
or this is a pure refactoring.

Maybe it'd help if we made it more explicit, e.g. by keeping the position
argument in InitializeForEachStatement. WDYT?

[kozy, 2017-05-23 12:55:47]

Description was changed from

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org,jgruber@chromium.org
==========

to

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

Additionally location for for..of loops is moved from before "of" to before each
variable expression.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org,jgruber@chromium.org
==========

[kozy, 2017-05-23 12:56:29]

thanks!
I updated CL with information about moving location inside for..of loop.

https://codereview.chromium.org/2893313002/diff/80001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2893313002/diff/80001/src/parsing/parser-base...
src/parsing/parser-base.h:5722: impl()->InitializeForEachStatement(loop,
expression, enumerable, body);
On 2017/05/23 12:33:57, jgruber wrote:
> While here it all depends on whether expression's location has been
initialized.
> That's pretty subtle - I still don't understand if there's a logic change here
> or this is a pure refactoring.
> 
> Maybe it'd help if we made it more explicit, e.g. by keeping the position
> argument in InitializeForEachStatement. WDYT?

It's not just refactoring, location for for..of loops is moved from before "of"
to before each variable / expression and I'd like to use this expression
location as break location since it looks more natural.

[marja, 2017-05-24 11:01:49]

Parser lgtm as the change is trivial (afaics it's collecting the locations only
for the inspector, so if they're correct from the inspector point of view, this
lgtm).

Q: why is the test which is in the first patch set not a part of the later patch
sets?

[kozy, 2017-05-24 11:06:28]

On 2017/05/24 11:01:49, marja wrote:
> Parser lgtm as the change is trivial (afaics it's collecting the locations
only
> for the inspector, so if they're correct from the inspector point of view,
this
> lgtm).
> 
> Q: why is the test which is in the first patch set not a part of the later
patch
> sets?

Thanks!

It's part, at least part of raw diff in latest patchset:
https://codereview.chromium.org/download/issue2893313002_80001.diff.

[kozy, 2017-05-24 11:07:47]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 11:07:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2017-05-24 11:24:13]

Right, it is, I somehow didn't see it :)

[commit-bot: I haz the power, 2017-05-24 11:36:49]

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

[commit-bot: I haz the power, 2017-05-24 11:36:50]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-05-24 11:45:04]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2017-05-24 11:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2893313002/#ps80001
(title: "removed each_keyword_pos")

[commit-bot: I haz the power, 2017-05-24 11:45:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-05-24 11:45:22]

The CQ bit was unchecked by kozyatinskiy@chromium.org

[kozy, 2017-05-24 12:11:21]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-05-24 12:11:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 12:12:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495627881131660,
"parent_rev": "14fa66b7a308d6c637f15cda07cb3f33a0b79fd3", "commit_rev":
"fb78710c06743a19fb7161794ee611527a8be1af"}

[commit-bot: I haz the power, 2017-05-24 12:13:03]

Description was changed from

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

Additionally location for for..of loops is moved from before "of" to before each
variable expression.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org,jgruber@chromium.org
==========

to

==========
[inspector] removed call break location from for-of loop

There are two break locations at the same source location by desugaring:
- call iterator.next,
- before variable assignment.

Additionally location for for..of loops is moved from before "of" to before each
variable expression.

We should not report first implicit call to avoid user confusion. User still
able to go into .next function with both scenarios:
- when this call is reached by stepOver or stepInto from previous line,
- when this call is reached because of breakpoint at current line.

BUG=v8:6425
R=dgozman@chromium.org,jgruber@chromium.org

Review-Url: https://codereview.chromium.org/2893313002
Cr-Commit-Position: refs/heads/master@{#45509}
Committed:
https://chromium.googlesource.com/v8/v8/+/fb78710c06743a19fb7161794ee611527a8...
==========

[commit-bot: I haz the power, 2017-05-24 12:13:04]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/fb78710c06743a19fb7161794ee611527a8...