WebVTT: Support 'auto' text position

WebVTT: Support 'auto' text position

This changes the behavior of the VTTCue.position attribute to match the
current spec. draft. It also changes the default of the same value when
parsed from a VTT file or created via the VTTCue constructor.

A number of tests where updated to reflect the new behavior (50 -> 'auto').

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

[fs, 2015-01-14 12:00:48]

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

[fs, 2015-01-14 13:13:40]

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:860: if (textPositionIsAuto())
Changed this on the assumption that we want to check the default value (which
means this is slightly more narrow than the previous one.)

[philipj_slow, 2015-01-15 09:08:58]

This is great, a few nits, and maybe split it differently.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:355: if (position.getAsString() !=
autoKeyword())
This should throw a TypeError, or whatever cue.align='bah' would throw.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:361: //
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...
Broken link, should be http://dev.w3.org/html5/webvtt/#dfn-vttcue-position
Maybe put the spec text at the top of the function?

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:600: return
static_cast<int>(m_textPosition);
Per spec there is no rounding of position to integer, just line in the
snap-to-lines case. I think it would be best to let the floats flow all the way
in this change, people would be justified in assuming that
'(cue.position=0.5)==0.5' implies that the layout also gets the float.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:860: if (textPositionIsAuto())
On 2015/01/14 13:13:40, fs wrote:
> Changed this on the assumption that we want to check the default value (which
> means this is slightly more narrow than the previous one.)

Yeah. Too bad the name will suck, but oh well.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.h:179: float m_textPosition;
Can you add a test that sets position to a value that can't be represented as
float and test which value is read back? I guess sneaky double->float is already
exposed to the Web a lot via CSSOM?

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (left):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:41: [RaisesException=Setter] attribute
long line;
How about line and size? I think grouping the long->double in line/position/size
into one CL and the introduction of AutoKeyword for line/position in another one
would make the most sense. Probably long->double as the first CL will be
cleanest.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:43: // FIXME: DOMString should be
AutoKeyword. (crbug.com/240176)
Can you comment on the bug so that this specific case can be prioritized?

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:44: [RaisesException=Setter] attribute
(double or DOMString) position;
Can you also add TypeChecking=Unrestricted to the interface and test what
happens when you set cue.position = Infinity/NaN?

[fs, 2015-01-15 10:07:16]

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:355: if (position.getAsString() !=
autoKeyword())
On 2015/01/15 09:08:57, philipj wrote:
> This should throw a TypeError, or whatever cue.align='bah' would throw.

No, assigning an "invalid" enum value to an attribute does not throw (using it
as an argument does though.)

https://heycam.github.io/webidl/#dfn-enumeration-value (the Note)

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:600: return
static_cast<int>(m_textPosition);
On 2015/01/15 09:08:57, philipj wrote:
> Per spec there is no rounding of position to integer, just line in the
> snap-to-lines case. I think it would be best to let the floats flow all the
way
> in this change, people would be justified in assuming that
> '(cue.position=0.5)==0.5' implies that the layout also gets the float.

I was trying to avoid "two steps at once" here by keeping the layout change as
small as possible. With the split, that's probably less of an issue though.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.h:179: float m_textPosition;
On 2015/01/15 09:08:57, philipj wrote:
> Can you add a test that sets position to a value that can't be represented as
> float and test which value is read back? I guess sneaky double->float is
already
> exposed to the Web a lot via CSSOM?

Are you saying that you want me to convert this to double, or just a test that
verifies that round-tripping doesn't work? (99.100001 would be an example -
which also kind of illustrates the sillyness IMHO... that doesn't round-trip
exactly for doubles either though of course)

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (left):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:41: [RaisesException=Setter] attribute
long line;
On 2015/01/15 09:08:57, philipj wrote:
> How about line and size? I think grouping the long->double in
line/position/size
> into one CL and the introduction of AutoKeyword for line/position in another
one
> would make the most sense. Probably long->double as the first CL will be
> cleanest.

Yes, I was thinking about that too... I guess this goes together with the
comment about double vs. float representable values, as it begs the question of
internal representation.
As for AutoKeyword I guess 'line' could be folded into this CL since it should
be mostly trivial since algorithm part is implemented already - but the
test-changes might be daunting enough to warrant a split (I think I'd prefer the
split approach there). Thoughts?

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:43: // FIXME: DOMString should be
AutoKeyword. (crbug.com/240176)
On 2015/01/15 09:08:57, philipj wrote:
> Can you comment on the bug so that this specific case can be prioritized?

I added a comment in the "IDL status" doc, and bashi said he'd added it to his
list.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:44: [RaisesException=Setter] attribute
(double or DOMString) position;
On 2015/01/15 09:08:57, philipj wrote:
> Can you also add TypeChecking=Unrestricted to the interface and test what
> happens when you set cue.position = Infinity/NaN?

Good catch.

[philipj_slow, 2015-01-19 14:44:26]

Some more rambling. I haven't looked at the tests yet, but have to leave early
today. If you go ahead and put the code in shape you want I'll take a look at
the tests next.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:355: if (position.getAsString() !=
autoKeyword())
On 2015/01/15 10:07:16, fs wrote:
> On 2015/01/15 09:08:57, philipj wrote:
> > This should throw a TypeError, or whatever cue.align='bah' would throw.
> 
> No, assigning an "invalid" enum value to an attribute does not throw (using it
> as an argument does though.)
> 
> https://heycam.github.io/webidl/#dfn-enumeration-value (the Note)

Acknowledged.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:600: return
static_cast<int>(m_textPosition);
On 2015/01/15 10:07:16, fs wrote:
> On 2015/01/15 09:08:57, philipj wrote:
> > Per spec there is no rounding of position to integer, just line in the
> > snap-to-lines case. I think it would be best to let the floats flow all the
> way
> > in this change, people would be justified in assuming that
> > '(cue.position=0.5)==0.5' implies that the layout also gets the float.
> 
> I was trying to avoid "two steps at once" here by keeping the layout change as
> small as possible. With the split, that's probably less of an issue though.

As long as the whole change ends up in the same milestone so that the feature
detection works I'm OK with splitting across CLs as you see fit.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.h:179: float m_textPosition;
On 2015/01/15 10:07:16, fs wrote:
> On 2015/01/15 09:08:57, philipj wrote:
> > Can you add a test that sets position to a value that can't be represented
as
> > float and test which value is read back? I guess sneaky double->float is
> already
> > exposed to the Web a lot via CSSOM?
> 
> Are you saying that you want me to convert this to double, or just a test that
> verifies that round-tripping doesn't work? (99.100001 would be an example -
> which also kind of illustrates the sillyness IMHO... that doesn't round-trip
> exactly for doubles either though of course)

I don't know what I want, in
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19836#c8 I was worried about the
size and now I'm worried about the difference between float and double being
observable...

What do you think? double wherever obvserable and float for layout would be OK,
but it's quite pedantic and assume that CSSOM has the exact same "problem". I'll
sit on the fence and let you decide!

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (left):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:41: [RaisesException=Setter] attribute
long line;
On 2015/01/15 10:07:16, fs wrote:
> On 2015/01/15 09:08:57, philipj wrote:
> > How about line and size? I think grouping the long->double in
> line/position/size
> > into one CL and the introduction of AutoKeyword for line/position in another
> one
> > would make the most sense. Probably long->double as the first CL will be
> > cleanest.
> 
> Yes, I was thinking about that too... I guess this goes together with the
> comment about double vs. float representable values, as it begs the question
of
> internal representation.
> As for AutoKeyword I guess 'line' could be folded into this CL since it should
> be mostly trivial since algorithm part is implemented already - but the
> test-changes might be daunting enough to warrant a split (I think I'd prefer
the
> split approach there). Thoughts?

Split it is!

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:43: // FIXME: DOMString should be
AutoKeyword. (crbug.com/240176)
On 2015/01/15 10:07:16, fs wrote:
> On 2015/01/15 09:08:57, philipj wrote:
> > Can you comment on the bug so that this specific case can be prioritized?
> 
> I added a comment in the "IDL status" doc, and bashi said he'd added it to his
> list.

Thanks!

[fs, 2015-01-19 15:09:06]

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:600: return
static_cast<int>(m_textPosition);
On 2015/01/19 14:44:26, philipj wrote:
> On 2015/01/15 10:07:16, fs wrote:
> > On 2015/01/15 09:08:57, philipj wrote:
> > > Per spec there is no rounding of position to integer, just line in the
> > > snap-to-lines case. I think it would be best to let the floats flow all
the
> > way
> > > in this change, people would be justified in assuming that
> > > '(cue.position=0.5)==0.5' implies that the layout also gets the float.
> > 
> > I was trying to avoid "two steps at once" here by keeping the layout change
as
> > small as possible. With the split, that's probably less of an issue though.
> 
> As long as the whole change ends up in the same milestone so that the feature
> detection works I'm OK with splitting across CLs as you see fit.

Yes, that should be feasible. I'm currently looking at landing this as:

1) long->double
2) Update VTTCue.line
3) Update VTTCue.position

So still 2 CLs to go before returning to this one.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.h:179: float m_textPosition;
On 2015/01/19 14:44:26, philipj wrote:
> On 2015/01/15 10:07:16, fs wrote:
> > On 2015/01/15 09:08:57, philipj wrote:
> > > Can you add a test that sets position to a value that can't be represented
> as
> > > float and test which value is read back? I guess sneaky double->float is
> > already
> > > exposed to the Web a lot via CSSOM?
> > 
> > Are you saying that you want me to convert this to double, or just a test
that
> > verifies that round-tripping doesn't work? (99.100001 would be an example -
> > which also kind of illustrates the sillyness IMHO... that doesn't round-trip
> > exactly for doubles either though of course)
> 
> I don't know what I want, in
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=19836#c8 I was worried about
the
> size and now I'm worried about the difference between float and double being
> observable...
> 
> What do you think? double wherever obvserable and float for layout would be
OK,
> but it's quite pedantic and assume that CSSOM has the exact same "problem".
I'll
> sit on the fence and let you decide!

I've gone with floats for now and added a test as "documentation" - will land
that separately (as requested elsewhere). I think it would be impossible to
avoid detection (you can always, say, add another 0 after the decimal point),
and wasting 12-16 bytes (~5% of VTTCue) on precision...
I'm considering splitting out display-state from VTTCue and allocate that
on-demand - maybe then the growth could be hidden in the reduction.

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.idl (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.idl:43: // FIXME: DOMString should be
AutoKeyword. (crbug.com/240176)
On 2015/01/19 14:44:26, philipj wrote:
> On 2015/01/15 10:07:16, fs wrote:
> > On 2015/01/15 09:08:57, philipj wrote:
> > > Can you comment on the bug so that this specific case can be prioritized?
> > 
> > I added a comment in the "IDL status" doc, and bashi said he'd added it to
his
> > list.
> 
> Thanks!

(This is now supported, so expect this FIXME to disappear when I upload the next
PS.)

[fs, 2015-01-19 18:10:40]

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTCue.cpp (right):

https://codereview.chromium.org/851933003/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTCue.cpp:355: if (position.getAsString() !=
autoKeyword())
On 2015/01/19 14:44:26, philipj wrote:
> On 2015/01/15 10:07:16, fs wrote:
> > On 2015/01/15 09:08:57, philipj wrote:
> > > This should throw a TypeError, or whatever cue.align='bah' would throw.
> > 
> > No, assigning an "invalid" enum value to an attribute does not throw (using
it
> > as an argument does though.)
> > 
> > https://heycam.github.io/webidl/#dfn-enumeration-value (the Note)
> 
> Acknowledged.

Actually, now I'm not so sure anymore... [1] (step 4) seems to special case
enumerations ("If the type of the attribute is an enumeration"), but not unions
(with enumerations)...

So:

attribute Foo bar; // Does not throw
attribute (double or Foo) baz; // Throws

I think I'll just go with whatever the impl. thinks is god for now...

[1] http://heycam.github.io/webidl/#dfn-attribute-setter

[fs, 2015-01-21 12:35:08]

Back here again after the interlude. PTAL (this now looks very similar to the
.line change - except for the s/m_textPosition/computedTextPosition/ bit.)

[philipj_slow, 2015-01-21 15:02:14]

LGTM, minor nits only. Do you also plan to make the parser changes? And in the
snap-to-lines case, I think in some CL there must be the equivalent of "Round
line position to an integer by adding 0.5 and then flooring it."

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
File LayoutTests/media/track/opera/interfaces/VTTCue/position.html (right):

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
LayoutTests/media/track/opera/interfaces/VTTCue/position.html:20: cue.position =
50;
Also try setting to 'auto'.

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
File LayoutTests/media/track/track-cue-rendering-position-auto-expected.html
(right):

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
LayoutTests/media/track/track-cue-rendering-position-auto-expected.html:38:
<video><script>document.querySelectorAll('video')[0].src =
mediaFile;</script></video>
Same, or maybe Array.prototype.forEach.call(document.querySelectorAll('video'),
...) at the end?

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
File LayoutTests/media/track/track-cue-rendering-position-auto.html (right):

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
LayoutTests/media/track/track-cue-rendering-position-auto.html:24: var video =
document.querySelectorAll('video')[0];
Given the structure, document.currentScript.parentNode would work as well.
Otherwise there isn't much reason to have the scripts inside the videos AFAICT.

[fs, 2015-01-21 15:28:24]

On 2015/01/21 15:02:14, philipj wrote:
> Do you also plan to make the parser changes?

If by that you mean "handle fractional values", then yes I have that queued.
(Have not implemented the alignment flags.)

> And in the
> snap-to-lines case, I think in some CL there must be the equivalent of "Round
> line position to an integer by adding 0.5 and then flooring it."

I noticed that change, and am working through the rendering steps ATM, but have
not reached that far yet.

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
File LayoutTests/media/track/opera/interfaces/VTTCue/position.html (right):

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
LayoutTests/media/track/opera/interfaces/VTTCue/position.html:20: cue.position =
50;
On 2015/01/21 15:02:14, philipj wrote:
> Also try setting to 'auto'.

Done.

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
File LayoutTests/media/track/track-cue-rendering-position-auto-expected.html
(right):

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
LayoutTests/media/track/track-cue-rendering-position-auto-expected.html:38:
<video><script>document.querySelectorAll('video')[0].src =
mediaFile;</script></video>
On 2015/01/21 15:02:14, philipj wrote:
> Same, or maybe
Array.prototype.forEach.call(document.querySelectorAll('video'),
> ...) at the end?

Done. (currentScript)

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
File LayoutTests/media/track/track-cue-rendering-position-auto.html (right):

https://codereview.chromium.org/851933003/diff/40001/LayoutTests/media/track/...
LayoutTests/media/track/track-cue-rendering-position-auto.html:24: var video =
document.querySelectorAll('video')[0];
On 2015/01/21 15:02:14, philipj wrote:
> Given the structure, document.currentScript.parentNode would work as well.
> Otherwise there isn't much reason to have the scripts inside the videos
AFAICT.

Done.

[philipj_slow, 2015-01-21 15:45:50]

Even more LGTM

[fs, 2015-01-21 16:06:06]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-01-21 16:07:01]

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

[commit-bot: I haz the power, 2015-01-21 16:35:59]

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