Separate the text track container from the media controls

Source/core/html/HTMLMediaElement.cpp

Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp b/Source/core/html/HTMLMediaElement.cpp
index 40724c9baed9ea85a09f19d4290dd712eaedd73b..19a8711cb13db8aa352fc2e5e31003602bfc78e4 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -55,6 +55,7 @@
 #include "core/html/track/AutomaticTrackSelection.h"
 #include "core/html/track/CueTimeline.h"
 #include "core/html/track/InbandTextTrack.h"
+#include "core/html/track/TextTrackContainer.h"
 #include "core/html/track/TextTrackList.h"
 #include "core/html/track/VideoTrack.h"
 #include "core/html/track/VideoTrackList.h"
@@ -3184,12 +3185,52 @@ bool HTMLMediaElement::closedCaptionsVisible() const
     return m_closedCaptionsVisible;
 }
 
+static void assertShadowRootChildren(ShadowRoot& shadowRoot)

[philipj_slow, 2015/02/26 09:51:52]

I don't know if this seems pedantic. I failed to get it right before I added
this assert, so here it is.

+{
+#if ENABLE(ASSERT)
+    // There can be up to two children, either or both of the text
+    // track container and media controls. If both are present, the
+    // text track container must be the first child.
+    unsigned numberOfChildren = shadowRoot.countChildren();
+    ASSERT(numberOfChildren <= 2);
+    Node* firstChild = shadowRoot.firstChild();
+    Node* lastChild = shadowRoot.lastChild();
+    if (numberOfChildren == 1) {
+        ASSERT(firstChild->isTextTrackContainer() || firstChild->isMediaControls());
+    } else if (numberOfChildren == 2) {
+        ASSERT(firstChild->isTextTrackContainer());
+        ASSERT(lastChild->isMediaControls());
+    }
+#endif
+}
+
+TextTrackContainer& HTMLMediaElement::ensureTextTrackContainer()
+{
+    ShadowRoot& shadowRoot = ensureClosedShadowRoot();
+    assertShadowRootChildren(shadowRoot);
+
+    Node* firstChild = shadowRoot.firstChild();
+    if (firstChild && firstChild->isTextTrackContainer())
+        return toTextTrackContainer(*firstChild);
+
+    RefPtrWillBeRawPtr<TextTrackContainer> textTrackContainer = TextTrackContainer::create(document());
+
+    // The text track container should be inserted before the media controls,
+    // so that they are rendered behind them.
+    shadowRoot.insertBefore(textTrackContainer, firstChild);
+
+    assertShadowRootChildren(shadowRoot);
+
+    return *textTrackContainer;
+}
+
 void HTMLMediaElement::updateTextTrackDisplay()
 {
     WTF_LOG(Media, "HTMLMediaElement::updateTextTrackDisplay(%p)", this);
 
-    ensureMediaControls();
-    mediaControls()->updateTextTrackDisplay();
+    // FIXME: this probably doesn't hold

[philipj_slow, 2015/02/26 09:51:52]

Um, right. fs, I think we can get here for audio elements as well. Should the
code on HTMLMediaElement that is only concerned with rendering be moved to
HTMLVideoElement do you think? Could also simply do nothing here if it's an
audio element, which would make the check in TextTrackContainer::updateDisplay
redundant.

[fs, 2015/02/26 12:30:51]

My (loose) plan so far wrt this has been to not even create the text track
container for HTMLAudioElements, and then use that to determine if cue display
should be updated or not. This is not entirely fleshed out yet though.

Current thinking is to have something like a CueDisplayContext which is bound to
the TextTrackContainer and passed to the CueTimeline/processing logic, where
cues get notified and "attaches" (or detaches) themselves to that.

I would support hoisting the check out of TextTrackContainer::updateDisplay as a
first step, so maybe that's something that could be done here?

[philipj_slow, 2015/02/27 07:13:51]

Not creating the container for audio sounds like a good plan. I'm going to opt
for a separate CL, and have thrown some ideas of how it might look into
https://codereview.chromium.org/962923002.

I'll revert the asserts.

[fs, 2015/02/27 09:50:31]

Acknowledged.

+    ASSERT(isHTMLVideoElement());
+    ensureTextTrackContainer().updateDisplay(*this);
 }
 
 void HTMLMediaElement::setClosedCaptionsVisible(bool closedCaptionVisible)
@@ -3262,12 +3303,12 @@ void HTMLMediaElement::setShouldDelayLoadEvent(bool shouldDelay)
         document().decrementLoadEventDelayCount();
 }
 
-
 MediaControls* HTMLMediaElement::mediaControls() const
 {
     if (ShadowRoot* shadowRoot = closedShadowRoot()) {
-        // Note that |shadowRoot->firstChild()| may be null.
-        return toMediaControls(shadowRoot->firstChild());
+        Node* lastChild = shadowRoot->lastChild();
+        if (lastChild && lastChild->isMediaControls())
+            return toMediaControls(lastChild);
     }
 
     return nullptr;
@@ -3284,7 +3325,14 @@ void HTMLMediaElement::ensureMediaControls()
     if (isFullscreen())
         mediaControls->enteredFullscreen();
 
-    ensureClosedShadowRoot().appendChild(mediaControls);
+    ShadowRoot& shadowRoot = ensureClosedShadowRoot();
+    assertShadowRootChildren(shadowRoot);
+
+    // The media controls should be inserted after the text track container,
+    // so that they are rendered in front of captions and subtitles.
+    shadowRoot.appendChild(mediaControls);
+
+    assertShadowRootChildren(shadowRoot);
 
     if (!shouldShowControls() || !inDocument())
         mediaControls->hide();
@@ -3340,8 +3388,8 @@ void HTMLMediaElement::configureTextTrackDisplay(VisibilityChangeAssumption assu
     if (!m_haveVisibleTextTrack && !mediaControls())
         return;
 
-    ensureMediaControls();
-    mediaControls()->changedClosedCaptionsVisibility();
+    if (mediaControls())
+        mediaControls()->changedClosedCaptionsVisibility();
 
     cueTimeline().updateActiveCues(currentTime());
     updateTextTrackDisplay();