VTTCue: Support 'auto' for line

VTTCue: Support 'auto' for line

Since the setter is no longer limitting the value to [0, 100] we need to
store an additional flag for the 'auto' value.
Minor updates to VTTCue::calculateComputedLinePosition to match updated
spec. (this is now where the line value is checked against the above
range.)

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

[fs, 2015-01-20 13:25:08]

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

[philipj_slow, 2015-01-20 14:52:22]

I can take another look, but LGTM if I'm too slow.

[philipj_slow, 2015-01-20 14:52:31]

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left):

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27:
assert_equals(c2.line, -2);
I see that this -2 test used to fail. Was that per spec when you implemented
this in Presto, or what? (Just curious.)

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/interfaces/VTTCue/line.html:30:
assert_equals(c1.line, -5);
Can you add a test for setting line to -1 since that used to be special? And
NaN/Infinity somewhere.

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

https://codereview.chromium.org/850363005/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:226: , m_computedLinePosition(0)
Since this is a valid line position, maybe initialize it to NaN and assert on
use in VTTCue::calculateDisplayParameters() and VTTCue::getPositionCoordinates()
that it's been calculated? Not actually a new concern, but 0 looks less
suspicious than undefinedPosition (-1) if you see it in a debugger.

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

https://codereview.chromium.org/850363005/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.h:192: bool m_lineIsAuto : 1;
How about using NaN for auto? Too clever?

[fs, 2015-01-20 16:35:17]

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left):

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27:
assert_equals(c2.line, -2);
On 2015/01/20 14:52:31, philipj wrote:
> I see that this -2 test used to fail. Was that per spec when you implemented
> this in Presto, or what? (Just curious.)

I'd think that "-2" is still the expected outcome of the computed line position
algorithm [1]. I think that the issue here was that VTTCue::line() just returned
m_linePosition without validating state (see VTTCue.h).

[1] http://dev.w3.org/html5/webvtt/#dfn-text-track-cue-computed-line-position

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/interfaces/VTTCue/line.html:30:
assert_equals(c1.line, -5);
On 2015/01/20 14:52:31, philipj wrote:
> Can you add a test for setting line to -1 since that used to be special? And
> NaN/Infinity somewhere.

Done. (And looks like we found a bindings bug...)

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

https://codereview.chromium.org/850363005/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:226: , m_computedLinePosition(0)
On 2015/01/20 14:52:31, philipj wrote:
> Since this is a valid line position, maybe initialize it to NaN and assert on
> use in VTTCue::calculateDisplayParameters() and
VTTCue::getPositionCoordinates()
> that it's been calculated? Not actually a new concern, but 0 looks less
> suspicious than undefinedPosition (-1) if you see it in a debugger.

Makes sense. Done. (Not in calculateDisplayParameters because there we define it
just before use. Note-to-self: Do this to m_cueSize too.)

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

https://codereview.chromium.org/850363005/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.h:192: bool m_lineIsAuto : 1;
On 2015/01/20 14:52:31, philipj wrote:
> How about using NaN for auto? Too clever?

I considered it, but since this is equally "free" storage-wise and felt slightly
more readable in places (since we can't compare NaN == NaN in the setter for
instance). that way certainly has it's allure though, so I'll code it up and see
what I think in practice...

Uploaded as PS3 for your viewing pleasure.

[philipj_slow, 2015-01-20 20:53:36]

PS4 viewed with great pleasure, LGTM.

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left):

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27:
assert_equals(c2.line, -2);
On 2015/01/20 16:35:16, fs wrote:
> On 2015/01/20 14:52:31, philipj wrote:
> > I see that this -2 test used to fail. Was that per spec when you implemented
> > this in Presto, or what? (Just curious.)
> 
> I'd think that "-2" is still the expected outcome of the computed line
position
> algorithm [1]. I think that the issue here was that VTTCue::line() just
returned
> m_linePosition without validating state (see VTTCue.h).
> 
> [1] http://dev.w3.org/html5/webvtt/#dfn-text-track-cue-computed-line-position

I see that the TextTrackCue.line getter in Presto was the computed line
position, so I guess the spec said this at some point.

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

https://codereview.chromium.org/850363005/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:226: , m_computedLinePosition(0)
On 2015/01/20 16:35:16, fs wrote:
> On 2015/01/20 14:52:31, philipj wrote:
> > Since this is a valid line position, maybe initialize it to NaN and assert
on
> > use in VTTCue::calculateDisplayParameters() and
> VTTCue::getPositionCoordinates()
> > that it's been calculated? Not actually a new concern, but 0 looks less
> > suspicious than undefinedPosition (-1) if you see it in a debugger.
> 
> Makes sense. Done. (Not in calculateDisplayParameters because there we define
it
> just before use. Note-to-self: Do this to m_cueSize too.)

My idea was to actually assert that calculateComputedLinePosition() does not
return NaN. Too much? You get to pick!

[fs, 2015-01-21 09:00:12]

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
File LayoutTests/media/track/opera/interfaces/VTTCue/line.html (left):

https://codereview.chromium.org/850363005/diff/1/LayoutTests/media/track/oper...
LayoutTests/media/track/opera/interfaces/VTTCue/line.html:27:
assert_equals(c2.line, -2);
On 2015/01/20 20:53:36, philipj wrote:
> On 2015/01/20 16:35:16, fs wrote:
> > On 2015/01/20 14:52:31, philipj wrote:
> > > I see that this -2 test used to fail. Was that per spec when you
implemented
> > > this in Presto, or what? (Just curious.)
> > 
> > I'd think that "-2" is still the expected outcome of the computed line
> position
> > algorithm [1]. I think that the issue here was that VTTCue::line() just
> returned
> > m_linePosition without validating state (see VTTCue.h).
> > 
> > [1]
http://dev.w3.org/html5/webvtt/#dfn-text-track-cue-computed-line-position
> 
> I see that the TextTrackCue.line getter in Presto was the computed line
> position, so I guess the spec said this at some point.

Right, I missed to acknowledge that part.

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

https://codereview.chromium.org/850363005/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.cpp:226: , m_computedLinePosition(0)
On 2015/01/20 20:53:36, philipj wrote:
> On 2015/01/20 16:35:16, fs wrote:
> > On 2015/01/20 14:52:31, philipj wrote:
> > > Since this is a valid line position, maybe initialize it to NaN and assert
> on
> > > use in VTTCue::calculateDisplayParameters() and
> > VTTCue::getPositionCoordinates()
> > > that it's been calculated? Not actually a new concern, but 0 looks less
> > > suspicious than undefinedPosition (-1) if you see it in a debugger.
> > 
> > Makes sense. Done. (Not in calculateDisplayParameters because there we
define
> it
> > just before use. Note-to-self: Do this to m_cueSize too.)
> 
> My idea was to actually assert that calculateComputedLinePosition() does not
> return NaN. Too much? You get to pick!

I don't worry too much about at least (it mostly returns integer literals after
all...)

[fs, 2015-01-21 09:00:19]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-01-21 09:01:18]

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

[commit-bot: I haz the power, 2015-01-21 09:04:35]

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