Eliminate TextTrackCue::updateDisplayTree()

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

 /*
  * Copyright (c) 2013, Opera Software ASA. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 726 matching lines @@
     }
 
     // Step 9 not implemented (margin == 0).
 
     ASSERT(std::isfinite(displayParameters.size));
     ASSERT(displayParameters.direction != CSSValueNone);
     ASSERT(displayParameters.writingMode != CSSValueNone);
     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);
             ASSERT_UNUSED(check, check);
 
             if (currentTimestamp > movieTime)
                 isPastNode = false;
         }
 
         if (child.isVTTElement()) {
             toVTTElement(child).setIsPastNode(isPastNode);
             // Make an elemenet id match a cue id for style matching purposes.
             if (!id().isEmpty())
                 toElement(child).setIdAttribute(id());
         }
     }
 }
 
-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());
 
     if (!m_displayTree) {
         m_displayTree = VTTCueBox::create(document(), this);
         m_displayTree->appendChild(m_cueBackgroundBox);
     }
 
     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);
 
     m_displayTreeShouldChange = false;
 
     return m_displayTree;
 }
 
 void VTTCue::removeDisplayTree(RemovalNotification removalNotification)
 {
@@ skipping 276 matching lines @@
 
 DEFINE_TRACE(VTTCue)
 {
     visitor->trace(m_vttNodeTree);
     visitor->trace(m_cueBackgroundBox);
     visitor->trace(m_displayTree);
     TextTrackCue::trace(visitor);
 }
 
 } // namespace blink