Eliminate TextTrackCue::updateDisplayTree()

Eliminate TextTrackCue::updateDisplayTree()

updateDisplayTree() cloned and inserted a new tree every time, and was
called continuously from CueTimeline::updateActiveCues(). The net effect
of this was to re-layout the cue continuously, causing cues to move into
areas where a cue was previously. The letter and spirit of the WebVTT
spec is to have a stable layout, where cues do not move once laid out.

TEST=LayoutTests/media/track/track-webvtt-two-cue-layout-after-first-end.html

As the display tree is now kept intact, it becomes necessary to
invalidate the style in setIsPastNode.

Test coverage of past and future cues:
LayoutTests/media/track/cue-style-invalidation.html
LayoutTests/media/track/track-css-matching-timestamps.html
LayoutTests/media/track/track-css-matching.html

Note that per spec a part of the cue could match neither :past nor
:future, but our implementation is mutually exclusive. It's not clear
what the best behavior is, so a spec bug was filed instead:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28237

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

[philipj_slow, 2015-03-19 11:21:36]

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

[philipj_slow, 2015-03-19 11:21:37]

PTAL

[fs, 2015-03-19 12:10:02]

lgtm

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

https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
Source/core/html/track/vtt/VTTCue.cpp:767: if (currentTimestamp > movieTime)
Since you touched upon this above and in the description...

isActive() => movieTime >= cue.startTime [&& movieTime <= cue.endTime]

so:

currentTimestamp > movieTime <==> startTime() > movieTime

So, it appears that the future will be rare... (makes you wonder if this if
really does any good - also note that the inner |currentTimestamp| shadows this
one at that...)

FWIW, I also thinks this illustrates why the specced behavior might be better.
(IIRC we implemented that in Presto by setting a flag if the parser noticed a
timestamp, and then not doing the equivalent of this if said flag was false.)

https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
File Source/core/html/track/vtt/VTTElement.cpp (right):

https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
Source/core/html/track/vtt/VTTElement.cpp:122: if (m_isPastNode == isPastNode)
Make this isPastNode() == isPastNode to please MSVC

[philipj_slow, 2015-03-19 12:36:36]

Thanks for review, now hopefully the controls dodging should be one step
closer...

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

https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
Source/core/html/track/vtt/VTTCue.cpp:767: if (currentTimestamp > movieTime)
On 2015/03/19 12:10:02, fs wrote:
> Since you touched upon this above and in the description...
> 
> isActive() => movieTime >= cue.startTime [&& movieTime <= cue.endTime]
> 
> so:
> 
> currentTimestamp > movieTime <==> startTime() > movieTime
> 
> So, it appears that the future will be rare... (makes you wonder if this if
> really does any good - also note that the inner |currentTimestamp| shadows
this
> one at that...)
> 
> FWIW, I also thinks this illustrates why the specced behavior might be better.
> (IIRC we implemented that in Presto by setting a flag if the parser noticed a
> timestamp, and then not doing the equivalent of this if said flag was false.)

Good catch, this branch looks unreachable, and an added ASSERT_NOT_REACHED isn't
hit while running LayoutTests/media/.

The second |if (currentTimestamp > movieTime) isPastNode = false;| below
(outside the diff) does its job though, I've verified manually that :future does
actually work.

For now I'm going to leave this alone, and hopefully we can replace this entire
function when the spec question is settled. Another bit of ugly is the
setIdAttribute below, seems completely out of place and likely the cause of that
bug.

https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
File Source/core/html/track/vtt/VTTElement.cpp (right):

https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
Source/core/html/track/vtt/VTTElement.cpp:122: if (m_isPastNode == isPastNode)
On 2015/03/19 12:10:02, fs wrote:
> Make this isPastNode() == isPastNode to please MSVC

It would have to be this->isPastNode(), so I'll go with !! coercion instead.
Feels like working with Presto's BOOL :)

[philipj_slow, 2015-03-19 12:37:42]

coerce m_isPastNode to bool

[philipj_slow, 2015-03-19 12:37:51]

The CQ bit was checked by philipj@opera.com

[philipj_slow, 2015-03-19 12:37:51]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1013393004/#ps20001
(title: "coerce m_isPastNode to bool")

[commit-bot: I haz the power, 2015-03-19 12:38:23]

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

[fs, 2015-03-19 12:56:45]

On 2015/03/19 12:36:36, philipj_UTC7 wrote:
> Thanks for review, now hopefully the controls dodging should be one step
> closer...
> 
>
https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
> File Source/core/html/track/vtt/VTTCue.cpp (right):
> 
>
https://codereview.chromium.org/1013393004/diff/1/Source/core/html/track/vtt/...
> Source/core/html/track/vtt/VTTCue.cpp:767: if (currentTimestamp > movieTime)
> On 2015/03/19 12:10:02, fs wrote:
> > Since you touched upon this above and in the description...
> > 
> > isActive() => movieTime >= cue.startTime [&& movieTime <= cue.endTime]
> > 
> > so:
> > 
> > currentTimestamp > movieTime <==> startTime() > movieTime
> > 
> > So, it appears that the future will be rare... (makes you wonder if this if
> > really does any good - also note that the inner |currentTimestamp| shadows
> this
> > one at that...)
> > 
> > FWIW, I also thinks this illustrates why the specced behavior might be
better.
> > (IIRC we implemented that in Presto by setting a flag if the parser noticed
a
> > timestamp, and then not doing the equivalent of this if said flag was
false.)
> 
> Good catch, this branch looks unreachable, and an added ASSERT_NOT_REACHED
isn't
> hit while running LayoutTests/media/.
> 
> The second |if (currentTimestamp > movieTime) isPastNode = false;| below
> (outside the diff) does its job though, I've verified manually that :future
does
> actually work.

Yepp, I would have expected that to work.

> For now I'm going to leave this alone, and hopefully we can replace this
entire
> function when the spec question is settled. Another bit of ugly is the
> setIdAttribute below, seems completely out of place and likely the cause of
that
> bug.

Yes, setting the id on all elements seems a bit excessive...

[commit-bot: I haz the power, 2015-03-19 14:27:33]

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