Refactor LineBreaker::nextSegmentBreak and add a BreakingContext that holds all its state

Refactor LineBreaker::nextSegmentBreak and add a BreakingContext that holds all its state

LineBreaker::nextSegmentBreak was around 600 lines of code, including multiple
gotos and an enormous amount of state variables. In an effort to make it easier
to follow what's going on in this very hot and very important function, I've
attempted to refactor it into a series of inline functions representing how it
processes different types of inline content. To hold onto all the state, I've
added a BreakingContext. This let me get rid of the gotos and bring nextSegmentBreak
down to about 50 lines.

There's still a lot that can be done to make working with this code not awful.
I believe there's a lot of simplification that can continue to occur in handleText,
which is still pretty monolithic.

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

[leviw_travelin_and_unemployed, 2013-09-27 22:47:20]

The diff here is a mess. If you can think of ways to improve BreakingContext
that you'd like to see in rev1 let me know. There's a long way to go, but I
think this is a big improvement.

[eae, 2013-09-27 22:51:50]

This is a great change, thanks for doing it.

While you are cleaning up this code though is there nay chance I could convince
you to move the BreakingContext class and supporting helper functions into a
separate file? I.e. a BreakingContextInlineHeaders.h file or something. That way
we get the benefit of better separation without breaking inlining.

[leviw_travelin_and_unemployed, 2013-09-27 23:05:31]

Doing that will cause this patch to balloon out into oblivion. I agree that this
is a great idea, and I'm happy to do that in a separate patch. If you'd like, I
can refactor LineBreaker into a separate file first in another patch and then
update this patch.

I can also update this patch accordingly, but I worry it'll be even harder to
review :-/

[eae, 2013-09-27 23:08:57]

On 2013/09/27 23:05:31, Levi wrote:
> Doing that will cause this patch to balloon out into oblivion. I agree that
this
> is a great idea, and I'm happy to do that in a separate patch. If you'd like,
I
> can refactor LineBreaker into a separate file first in another patch and then
> update this patch.
> 
> I can also update this patch accordingly, but I worry it'll be even harder to
> review :-/

Makes sense, a separate patch is perfectly fine :)

[eae, 2013-09-27 23:09:04]

https://codereview.chromium.org/25054004/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlockLineLayout.cpp (left):

https://codereview.chromium.org/25054004/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlockLineLayout.cpp:2533: m_clear = CNONE;
Where did this go?

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

https://codereview.chromium.org/25054004/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlockLineLayout.cpp:1263: // This copies BidiStatus
and may churn the ref on Bidim_ I doubt it matters.
Over reaching search/replace?

[leviw_travelin_and_unemployed, 2013-09-27 23:21:32]

PTAL

[eae, 2013-09-27 23:23:39]

LGTM

[commit-bot: I haz the power, 2013-09-27 23:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leviw@chromium.org/25054004/10001

[commit-bot: I haz the power, 2013-09-28 04:29:43]

Change committed as 158487