Fix expression positions for for-loops.

Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests,
and removes text expectations for Ignition.

---
An alternative would be to emulate the existing behaviour in Ignition, but:
- The new behaviour seems more logical,
- Ignition generates no bytecodes for the 'var', meaning there is no code position to attach the break position to.

BUG=v8:4690
LOG=Y

Committed: https://crrev.com/de4f3d3eff50f0cdd53a6cdf9d4a389d575b17aa
Cr-Commit-Position: refs/heads/master@{#34717}

[vogelheim, 2016-03-10 17:24:24]

Description was changed from

==========
Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests, and removes
exceptions for Ignition.

BUG=v8:4690
LOG=Y
==========

to

==========
Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests,
and removes text expectations for Ignition.

---
An alternative would be to emulate the existing behaviour in Ignition, but:
- The new behaviour seems more logical,
- Ignition generates no bytecodes for the 'var', meaning there is no code
position to attach the break position to.


BUG=v8:4690
LOG=Y
==========

[vogelheim, 2016-03-10 17:35:59]

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

[vogelheim, 2016-03-10 17:36:00]

Not sure if this solutions makes sense, but... please have a look. :)

[Yang, 2016-03-10 19:27:00]

LGTM. Thanks.

This solution indeed makes sense! The old behavior was in place because the
assignment of which the init statement usually consists used to not have a
source position, so we did not insert a break location for it. Now that I fixed
that a week ago, we can do this.

However, make sure to run blink layout tests. If there are test failures, and I
expect there to be some, make sure to
- file a crbug
- add a [ NeedsManualRebaseline ] entry for the test in
chromium/src/third_party/WebKit/LayoutTests/TestExpectations
- land this CL
- wait for V8 to roll into chromium
- change the test expectation entry to [ NeedsRebaseline ]
- close crbug

[vogelheim, 2016-03-11 11:59:35]

On 2016/03/10 19:27:00, Yang wrote:
> LGTM. Thanks.
> 
> This solution indeed makes sense! The old behavior was in place because the
> assignment of which the init statement usually consists used to not have a
> source position, so we did not insert a break location for it. Now that I
fixed
> that a week ago, we can do this.
> 
> However, make sure to run blink layout tests. If there are test failures, and
I
> expect there to be some, make sure to
> - file a crbug
> - add a [ NeedsManualRebaseline ] entry for the test in
> chromium/src/third_party/WebKit/LayoutTests/TestExpectations
> - land this CL
> - wait for V8 to roll into chromium
> - change the test expectation entry to [ NeedsRebaseline ]
> - close crbug

Patched into tip-of-tree chromium; built blink_tests; ran
Script/Tools/run_webkit_tests. There were two unexpected text-diff failures, but
neither related to this change. I take it the blink layout tests are
unaffected...

Will commit...

[vogelheim, 2016-03-11 12:01:02]

On 2016/03/11 11:59:35, vogelheim wrote:
> On 2016/03/10 19:27:00, Yang wrote:
> > LGTM. Thanks.
> > 
> > This solution indeed makes sense! The old behavior was in place because the
> > assignment of which the init statement usually consists used to not have a
> > source position, so we did not insert a break location for it. Now that I
> fixed
> > that a week ago, we can do this.
> > 
> > However, make sure to run blink layout tests. If there are test failures,
and
> I
> > expect there to be some, make sure to
> > - file a crbug
> > - add a [ NeedsManualRebaseline ] entry for the test in
> > chromium/src/third_party/WebKit/LayoutTests/TestExpectations
> > - land this CL
> > - wait for V8 to roll into chromium
> > - change the test expectation entry to [ NeedsRebaseline ]
> > - close crbug
> 
> Patched into tip-of-tree chromium; built blink_tests; ran
> Script/Tools/run_webkit_tests. There were two unexpected text-diff failures,
but
> neither related to this change. I take it the blink layout tests are
> unaffected...
> 
> Will commit...

Will also watch the v8 fyi bots, just in case.

[vogelheim, 2016-03-11 12:01:08]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2016-03-11 12:01:11]

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

[commit-bot: I haz the power, 2016-03-11 12:02:42]

Description was changed from

==========
Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests,
and removes text expectations for Ignition.

---
An alternative would be to emulate the existing behaviour in Ignition, but:
- The new behaviour seems more logical,
- Ignition generates no bytecodes for the 'var', meaning there is no code
position to attach the break position to.


BUG=v8:4690
LOG=Y
==========

to

==========
Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests,
and removes text expectations for Ignition.

---
An alternative would be to emulate the existing behaviour in Ignition, but:
- The new behaviour seems more logical,
- Ignition generates no bytecodes for the 'var', meaning there is no code
position to attach the break position to.


BUG=v8:4690
LOG=Y
==========

[commit-bot: I haz the power, 2016-03-11 12:02:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-11 12:02:56]

Description was changed from

==========
Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests,
and removes text expectations for Ignition.

---
An alternative would be to emulate the existing behaviour in Ignition, but:
- The new behaviour seems more logical,
- Ignition generates no bytecodes for the 'var', meaning there is no code
position to attach the break position to.


BUG=v8:4690
LOG=Y
==========

to

==========
Fix expression positions for for-loops.

FullCodegen generates 2 statement positions for the loop init block, like so:

  for(var i = 0; i....
      ^   ^

This change removes the first of those, updates unit tests,
and removes text expectations for Ignition.

---
An alternative would be to emulate the existing behaviour in Ignition, but:
- The new behaviour seems more logical,
- Ignition generates no bytecodes for the 'var', meaning there is no code
position to attach the break position to.

BUG=v8:4690
LOG=Y

Committed: https://crrev.com/de4f3d3eff50f0cdd53a6cdf9d4a389d575b17aa
Cr-Commit-Position: refs/heads/master@{#34717}
==========

[commit-bot: I haz the power, 2016-03-11 12:02:57]

Patchset 3 (id:??) landed as
https://crrev.com/de4f3d3eff50f0cdd53a6cdf9d4a389d575b17aa
Cr-Commit-Position: refs/heads/master@{#34717}