Refactor RenderVTTCue/SnapToLinesLayouter

Refactor RenderVTTCue/SnapToLinesLayouter

Simplify RenderVTTCue::findFirstLineBox, and have it return the flow box
instead of using an out parameter.

Stop mutating the 'switched' bool in the helper methods, getting the
logic more out "in the open" (in the step loop). For the same reason
remove the switchDirection method to expose the interaction with the
'switched' flag.

Split SnapToLinesLayouter::initializeLayoutParameters into one part that
determines the 'step' and one that computes the initial position
adjustment. Move the first part into SnapToLinesLayouter::layout, and
the second into the new method
SnapToLinesLayouter::computeInitialPositionAdjustment.

Generalize moveBoxesByStep to moveBoxesBy and use it for computing the
default/initial position as well. Hoist the call for the latter into
SnapToLinesLayouter::layout and rename placeBoxInDefaultPosition to
initializeFallbackPositioningState.

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

[fs, 2015-01-28 10:15:48]

fs@opera.com changed reviewers:
+ philipj@opera.com

[fs, 2015-01-28 10:15:49]

Adding conflicts for myself while waiting for Godot...

[philipj_slow, 2015-01-28 16:35:58]

lgtm

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

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:84: ASSERT(m_cueBox.firstChild());
This was moved from elsewhere and now it's basically ASSERT(x);x->foo() which is
a bit excessive IMHO. Not sure what the majority style on this matter is and
it's not easy to grep for. You know Soure/core/rendering/ better than I do so
you pick :)

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:106: position -= m_cueBox.size().width();
Off-topic: shouldn't this be firstLineBox->size().width()? Or is m_cueBox
somehow shrink-wrapping?

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:204:
initializeFallbackPositioningState();
I don't know, this is only called here and contains only one statement, might
just as well inline it?

[fs, 2015-01-28 17:03:05]

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

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:84: ASSERT(m_cueBox.firstChild());
On 2015/01/28 16:35:58, philipj_UTC7 wrote:
> This was moved from elsewhere and now it's basically ASSERT(x);x->foo() which
is
> a bit excessive IMHO. Not sure what the majority style on this matter is and
> it's not easy to grep for. You know Soure/core/rendering/ better than I do so
> you pick :)

Yes... maybe RenderVTTCue::layout would be a better place for it... the old
place (just below here) was after the first dereference, so this location the
"last" one that can make any sense (IMHO).

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:106: position -= m_cueBox.size().width();
On 2015/01/28 16:35:58, philipj_UTC7 wrote:
> Off-topic: shouldn't this be firstLineBox->size().width()? Or is m_cueBox
> somehow shrink-wrapping?

'height' (i.e. width in this case because it's vertical) is 'auto', so I guess
you could call it shrink-wrapping (extent determined by content).

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:204:
initializeFallbackPositioningState();
On 2015/01/28 16:35:58, philipj_UTC7 wrote:
> I don't know, this is only called here and contains only one statement, might
> just as well inline it?

This is sort of dependent on how we end up resolving the 'best position' issue.
If 'best position' disappears inlining is absolutely reasonable. (This CL was
originally written as being on top of the 'best position' CL - if that explains
anything...)

[philipj_slow, 2015-01-28 17:21:07]

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

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:106: position -= m_cueBox.size().width();
On 2015/01/28 17:03:05, fs wrote:
> On 2015/01/28 16:35:58, philipj_UTC7 wrote:
> > Off-topic: shouldn't this be firstLineBox->size().width()? Or is m_cueBox
> > somehow shrink-wrapping?
> 
> 'height' (i.e. width in this case because it's vertical) is 'auto', so I guess
> you could call it shrink-wrapping (extent determined by content).

Ah, right.

https://codereview.chromium.org/882023003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:204:
initializeFallbackPositioningState();
On 2015/01/28 17:03:05, fs wrote:
> On 2015/01/28 16:35:58, philipj_UTC7 wrote:
> > I don't know, this is only called here and contains only one statement,
might
> > just as well inline it?
> 
> This is sort of dependent on how we end up resolving the 'best position'
issue.
> If 'best position' disappears inlining is absolutely reasonable. (This CL was
> originally written as being on top of the 'best position' CL - if that
explains
> anything...)

I see. Let's move it around some more in that CL :)

[fs, 2015-01-28 17:27:05]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-01-28 17:27:21]

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

[commit-bot: I haz the power, 2015-01-28 18:39:06]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189128