Add missing reset calls after continue in layoutRunsAndFloatsInRange().

Add missing reset calls after continue in layoutRunsAndFloatsInRange().

The loop in layoutRunsAndFloatsInRange expects to run certain functions
before the next iteration. These functions are not run before the
continue.

BUG=425517
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185676

[rhogan, 2014-11-03 18:58:12]

robhogan@gmail.com changed reviewers:
+ robhogan@gmail.com

[rhogan, 2014-11-03 18:58:12]

https://codereview.chromium.org/685623005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/685623005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockLineLayout.cpp:884:
resolver.setPosition(endOfLine, numberOfIsolateAncestors(endOfLine));
This looks right to me. The lineMidpointState.reset() looks superfluous but if
you want to remove it you need to look closely at the logic that breaks before
it gets called at the top of the function.

In the bug you note that there's another superfluous-looking call to
lineMidpointState.reset() at the end of the function - that is definitely worth
addressing in a follow-up patch. If I were you I would keep the change above as
it is and address the superfluous calls in a follow-up.

You also need to add a test here. Once you have that you should ping an OWNER
like leviw or jchaffraix.

[zherczeg, 2014-11-13 15:02:59]

zherczeg.u-szeged@partner.samsung.com changed reviewers:
+ jchaffraix+rendering@chromium.org, leviw@chromium.org

[zherczeg, 2014-11-13 15:07:26]

On 2014/11/13 15:03:00, zherczeg wrote:

I don't understand why the expected called as a copy:
copy from LayoutTests/fast/html/pending-stylesheet-crash-expected.txt
copy to LayoutTests/fast/html/layout-runs-and-floats-crash-expected.txt

It is not in my side of the patch. Something happened when "git cl upload" is
executed.

[dsinclair, 2014-11-13 15:08:59]

On 2014/11/13 15:07:26, zherczeg wrote:
> On 2014/11/13 15:03:00, zherczeg wrote:
> 
> I don't understand why the expected called as a copy:
> copy from LayoutTests/fast/html/pending-stylesheet-crash-expected.txt
> copy to LayoutTests/fast/html/layout-runs-and-floats-crash-expected.txt
> 
> It is not in my side of the patch. Something happened when "git cl upload" is
> executed.


If you pass --no-find-copies to git cl upload it should work correctly.

[zherczeg, 2014-11-17 07:21:17]

> If you pass --no-find-copies to git cl upload it should work correctly.

Thank you. What do you think about the patch? Shall I upload it again, or do I
need to do some changes before that?

[dsinclair, 2014-11-17 14:12:21]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2014-11-17 14:12:22]

https://codereview.chromium.org/685623005/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/685623005/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlockLineLayout.cpp:885:
resolver.setPosition(endOfLine, numberOfIsolateAncestors(endOfLine));
This seems like it could potentially cause issues in the future. If we add
something to reset at the end of the loop but forget to add here.  Can we
re-structure this a bit so we don't continue? Set a flag and skip the remaining
work other then cleanup if the flag is set?

[zherczeg, 2014-11-18 07:30:26]

> This seems like it could potentially cause issues in the future. If we add
> something to reset at the end of the loop but forget to add here.  Can we
> re-structure this a bit so we don't continue? Set a flag and skip the
remaining
> work other then cleanup if the flag is set?

This would require an extra indentation level for a lot of code. Perhaps a goto
could be a cleaner solution. Which one do you prefer?

[dsinclair, 2014-11-18 14:51:43]

On 2014/11/18 07:30:26, zherczeg wrote:
> > This seems like it could potentially cause issues in the future. If we add
> > something to reset at the end of the loop but forget to add here.  Can we
> > re-structure this a bit so we don't continue? Set a flag and skip the
> remaining
> > work other then cleanup if the flag is set?
> 
> This would require an extra indentation level for a lot of code. Perhaps a
goto
> could be a cleaner solution. Which one do you prefer?


I don't think a lot of code requires re-indenting. Move the setLogicalHeight
into an else clause, set a flag as needed and the the 3 following blocks (the
for and two if's) get indented based on the flag.

Please don't use a goto.

[zherczeg, 2014-11-20 12:43:26]

Thank you for the review. I changed the patch to use a flag.

[dsinclair, 2014-11-20 14:41:24]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2014-11-20 14:41:24]

lgtm

[commit-bot: I haz the power, 2014-11-20 14:41:45]

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

[commit-bot: I haz the power, 2014-11-20 16:17:12]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185676