WebVTT: Support fractional percentage values for position/line/size

WebVTT: Support fractional percentage values for position/line/size

This updates the WebVTT (cue settings) parser to accept fractional
percentage values for the 'position', 'line' and 'size' settings -
per the current spec draft.
Also updates spec steps and rewraps some comments.

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

[fs, 2015-01-21 18:00:44]

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

[philipj_slow, 2015-01-22 10:31:03]

Heading out for lunch now, but I've looked through VTTCue.cpp.

https://codereview.chromium.org/864043002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/864043002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:1063: if (!input.isAt(valueRun.end()))
Can you move this up closer to input.scanDigits? I think it's only needed in
that code path, and you can assert here that both cases have consumed the input.
Assigning number when scanDigits didn't consume all of the input made me scratch
my head a bit.

https://codereview.chromium.org/864043002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:1084: if (!input.isAt(valueRun.end()))
I think scanPercentage takes care of this case and it can be asserted here
instead. Same for the size case if I'm right.

[fs, 2015-01-22 11:22:38]

https://codereview.chromium.org/864043002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/864043002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:1063: if (!input.isAt(valueRun.end()))
On 2015/01/22 10:31:03, philipj wrote:
> Can you move this up closer to input.scanDigits? I think it's only needed in
> that code path, and you can assert here that both cases have consumed the
input.

No, this is needed in both code-paths (scanPercentage used to do this check, but
now it no longer does.)

> Assigning number when scanDigits didn't consume all of the input made me
scratch
> my head a bit.

It's only the "temporary" number though - i.e. not m_linePosition.

https://codereview.chromium.org/864043002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:1084: if (!input.isAt(valueRun.end()))
On 2015/01/22 10:31:03, philipj wrote:
> I think scanPercentage takes care of this case and it can be asserted here
> instead. Same for the size case if I'm right.

It used to, but no longer does.

[philipj_slow, 2015-01-22 12:49:35]

That's what I get for only reading part of the CL :)

[philipj_slow, 2015-01-22 12:55:44]

lgtm

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
File
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-position.vtt
(right):

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-position.vtt:12:
00:00:00.000 --> 00:00:01.000 position:1.5
Maybe test with 1.0 to check that it's not just a matter of the result having a
zero fractional part.

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
File
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-size.vtt
(right):

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-size.vtt:36:
00:00:00.000 --> 00:00:01.000 size:1e1%
On the same theme "5.%" and ".5%"?

[fs, 2015-01-22 13:17:07]

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
File
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-position.vtt
(right):

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-position.vtt:12:
00:00:00.000 --> 00:00:01.000 position:1.5
On 2015/01/22 12:55:44, philipj wrote:
> Maybe test with 1.0 to check that it's not just a matter of the result having
a
> zero fractional part.

Done.

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
File
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-size.vtt
(right):

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-size.vtt:36:
00:00:00.000 --> 00:00:01.000 size:1e1%
On 2015/01/22 12:55:44, philipj wrote:
> On the same theme "5.%" and ".5%"?

Added. (On the same theme though? Those are both valid...)

[fs, 2015-01-22 14:08:33]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-01-22 14:09:20]

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

[commit-bot: I haz the power, 2015-01-22 14:26:32]

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

[philipj_slow, 2015-01-22 15:00:22]

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
File
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-size.vtt
(right):

https://codereview.chromium.org/864043002/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/track/webvtt/parsing/support/settings-size.vtt:36:
00:00:00.000 --> 00:00:01.000 size:1e1%
On 2015/01/22 13:17:06, fs wrote:
> On 2015/01/22 12:55:44, philipj wrote:
> > On the same theme "5.%" and ".5%"?
> 
> Added. (On the same theme though? Those are both valid...)

Oops, I was thinking about the syntax there:
http://dev.w3.org/html5/webvtt/#dfn-webvtt-percentage