Move WebVTT snap-to-lines layout code to a helper class

Move WebVTT snap-to-lines layout code to a helper class

Move the bulk of the cue (post "box generation") layout code to a new
helper SnapToLinesLayouter.
This eliminates transient state from RenderVTTCue, and should make is
easier to cache additional values (like the containing block and it's
bounding box for example.) It could eventually also help breaking the
dependency with the physical VTT cue structure, and make it possible to
move the layout to the cue container.
The code that tries to adjust for margin/border/padding is split out into
a method.

No functional changes intended.

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

[fs, 2015-01-26 17:12:21]

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

[philipj_slow, 2015-01-27 05:06:58]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-01-27 05:06:59]

lgtm

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

https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:325: void
RenderVTTCue::adjustForTopAndBottomMarginBorderAndPadding()
Does this bit make sense to you? It looks like it's checking if the cue box has
ended up outside its containing block, but I think per spec the stepping should
never stop there. When would this function do anything, and does it roughly
correspond to anything in the spec?

[commit-bot: I haz the power, 2015-01-27 05:07:24]

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

[commit-bot: I haz the power, 2015-01-27 05:11:00]

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

[fs, 2015-01-27 12:15:53]

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

https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderVTTCue.cpp:325: void
RenderVTTCue::adjustForTopAndBottomMarginBorderAndPadding()
On 2015/01/27 05:06:59, philipj_UTC7 wrote:
> Does this bit make sense to you?

Not really, no.

> It looks like it's checking if the cue box has
> ended up outside its containing block, but I think per spec the stepping
should
> never stop there. When would this function do anything, and does it roughly
> correspond to anything in the spec?

It doesn't corresponding to anything in the spec. - the 'margin' thing would be
the closest, but that't more like padding on the track container. 
I guess the key is hasInlineDirectionBordersPaddingOrMargin() - which checks for
"has margin, border, padding" - i.e. if the cue box has one of those properties,
you could up undoing the cue layout... It'd seem better to include any relevant
property in the general computation instead (even if it's only UA style.)

I'm considering just ripping this out entirely - and if there's a complaint,
then implementing it like described in the previous paragraph...

[philipj_slow, 2015-01-27 12:19:02]

On 2015/01/27 12:15:53, fs wrote:
>
https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderVTTCue.cpp (right):
> 
>
https://codereview.chromium.org/873483003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderVTTCue.cpp:325: void
> RenderVTTCue::adjustForTopAndBottomMarginBorderAndPadding()
> On 2015/01/27 05:06:59, philipj_UTC7 wrote:
> > Does this bit make sense to you?
> 
> Not really, no.
> 
> > It looks like it's checking if the cue box has
> > ended up outside its containing block, but I think per spec the stepping
> should
> > never stop there. When would this function do anything, and does it roughly
> > correspond to anything in the spec?
> 
> It doesn't corresponding to anything in the spec. - the 'margin' thing would
be
> the closest, but that't more like padding on the track container. 
> I guess the key is hasInlineDirectionBordersPaddingOrMargin() - which checks
for
> "has margin, border, padding" - i.e. if the cue box has one of those
properties,
> you could up undoing the cue layout... It'd seem better to include any
relevant
> property in the general computation instead (even if it's only UA style.)
> 
> I'm considering just ripping this out entirely - and if there's a complaint,
> then implementing it like described in the previous paragraph...

[philipj_slow, 2015-01-27 12:19:52]

Oops. I'd be sympathetic to throwing this code out, perhaps trying it will
reveal some tests where it does something useful?

[fs, 2015-01-27 12:38:04]

On 2015/01/27 12:19:52, philipj_UTC7 wrote:
> Oops. I'd be sympathetic to throwing this code out, perhaps trying it will
> reveal some tests where it does something useful?

Yes, hopefully. (Or maybe hopefully not...)