Eliminate TextTrackCue::updateDisplayTree()

Source/core/html/track/vtt/VTTCue.cpp

Index: Source/core/html/track/vtt/VTTCue.cpp
diff --git a/Source/core/html/track/vtt/VTTCue.cpp b/Source/core/html/track/vtt/VTTCue.cpp
index 92f0990b2c1c00dacbd9b35948d0565e71cda1e8..9276b20b40a82ab615f0b251f8b9c73b5a446b1d 100644
--- a/Source/core/html/track/vtt/VTTCue.cpp
+++ b/Source/core/html/track/vtt/VTTCue.cpp
@@ -744,16 +744,30 @@ VTTDisplayParameters VTTCue::calculateDisplayParameters() const
     return displayParameters;
 }
 
-void VTTCue::markFutureAndPastNodes(ContainerNode* root, double previousTimestamp, double movieTime)
+void VTTCue::updatePastAndFutureNodes(double movieTime)
 {
     DEFINE_STATIC_LOCAL(const String, timestampTag, ("timestamp"));
 
+    ASSERT(isActive());
+
+    // An active cue may still not have a display tree, e.g. if its track is
+    // hidden or if the track belongs to an audio element.
+    if (!m_displayTree)
+        return;
+
+    // FIXME: Per spec it's possible for neither :past nor :future to match, but
+    // as implemented here and in SelectorChecker they are simply each others
+    // negations. For a cue with no internal timestamps, :past will match but
+    // should not per spec. :future is correct, however. See the spec bug to
+    // determine what the correct behavior should be:
+    // https://www.w3.org/Bugs/Public/show_bug.cgi?id=28237
+
     bool isPastNode = true;
-    double currentTimestamp = previousTimestamp;
+    double currentTimestamp = startTime();
     if (currentTimestamp > movieTime)

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

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.)

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

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.

         isPastNode = false;
 
-    for (Node& child : NodeTraversal::descendantsOf(*root)) {
+    for (Node& child : NodeTraversal::descendantsOf(*m_displayTree)) {
         if (child.nodeName() == timestampTag) {
             double currentTimestamp;
             bool check = VTTParser::collectTimeStamp(child.nodeValue(), currentTimestamp);
@@ -772,25 +786,6 @@ void VTTCue::markFutureAndPastNodes(ContainerNode* root, double previousTimestam
     }
 }
 
-void VTTCue::updateDisplayTree(double movieTime)
-{
-    // The display tree may contain WebVTT timestamp objects representing
-    // timestamps (processing instructions), along with displayable nodes.
-
-    if (!track()->isRendered())
-        return;
-
-    // Clear the contents of the set.
-    m_cueBackgroundBox->removeChildren();
-
-    // Update the two sets containing past and future WebVTT objects.
-    createVTTNodeTree();
-    RefPtrWillBeRawPtr<DocumentFragment> referenceTree = DocumentFragment::create(document());
-    m_vttNodeTree->cloneChildNodes(referenceTree.get());
-    markFutureAndPastNodes(referenceTree.get(), startTime(), movieTime);
-    m_cueBackgroundBox->appendChild(referenceTree, ASSERT_NO_EXCEPTION);
-}
-
 PassRefPtrWillBeRawPtr<VTTCueBox> VTTCue::getDisplayTree()
 {
     ASSERT(track() && track()->isRendered() && isActive());
@@ -802,13 +797,14 @@ PassRefPtrWillBeRawPtr<VTTCueBox> VTTCue::getDisplayTree()
 
     ASSERT(m_displayTree->firstChild() == m_cueBackgroundBox);
 
-    // Note: It is updateDisplayTree() which actually adds the child
-    // nodes to m_cueBackgroundBox.
-
     if (!m_displayTreeShouldChange)
         return m_displayTree;
 
     createVTTNodeTree();
+
+    m_cueBackgroundBox->removeChildren();
+    m_vttNodeTree->cloneChildNodes(m_cueBackgroundBox.get());
+
     VTTDisplayParameters displayParameters = calculateDisplayParameters();
     m_displayTree->applyCSSProperties(displayParameters);