Separate the text track container from the media controls

Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. 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 37 matching lines @@
 #include "core/html/MediaController.h"
 #include "core/html/MediaError.h"
 #include "core/html/MediaFragmentURIParser.h"
 #include "core/html/TimeRanges.h"
 #include "core/html/shadow/MediaControls.h"
 #include "core/html/track/AudioTrack.h"
 #include "core/html/track/AudioTrackList.h"
 #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"
 #include "core/layout/LayoutVideo.h"
 #include "core/layout/LayoutView.h"
 #include "core/layout/compositing/LayerCompositor.h"
 #include "core/loader/FrameLoader.h"
 #include "platform/ContentType.h"
 #include "platform/Logging.h"
 #include "platform/MIMETypeFromURL.h"
@@ skipping 3109 matching lines @@
         }
     }
     return false;
 }
 
 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)
 {
     WTF_LOG(Media, "HTMLMediaElement::setClosedCaptionsVisible(%p, %s)", this, boolString(closedCaptionVisible));
 
     if (!m_player || !hasClosedCaptions())
         return;
 
     m_closedCaptionsVisible = closedCaptionVisible;
@@ skipping 52 matching lines @@
 
     WTF_LOG(Media, "HTMLMediaElement::setShouldDelayLoadEvent(%p, %s)", this, boolString(shouldDelay));
 
     m_shouldDelayLoadEvent = shouldDelay;
     if (shouldDelay)
         document().incrementLoadEventDelayCount();
     else
         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;
 }
 
 void HTMLMediaElement::ensureMediaControls()
 {
     if (mediaControls())
         return;
 
     RefPtrWillBeRawPtr<MediaControls> mediaControls = MediaControls::create(*this);
 
     mediaControls->reset();
     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();
 }
 
 void HTMLMediaElement::configureMediaControls()
 {
     if (!inDocument()) {
         if (mediaControls())
             mediaControls()->hide();
@@ skipping 35 matching lines @@
         && m_haveVisibleTextTrack == haveVisibleTextTrack) {
         cueTimeline().updateActiveCues(currentTime());
         return;
     }
     m_haveVisibleTextTrack = haveVisibleTextTrack;
     m_closedCaptionsVisible = m_haveVisibleTextTrack;
 
     if (!m_haveVisibleTextTrack && !mediaControls())
         return;
 
-    ensureMediaControls();
-    mediaControls()->changedClosedCaptionsVisibility();
+    if (mediaControls())
+        mediaControls()->changedClosedCaptionsVisibility();
 
     cueTimeline().updateActiveCues(currentTime());
     updateTextTrackDisplay();
 }
 
 void* HTMLMediaElement::preDispatchEventHandler(Event* event)
 {
     if (event && event->type() == EventTypeNames::webkitfullscreenchange)
         configureMediaControls();
 
@@ skipping 241 matching lines @@
 
 #if ENABLE(WEB_AUDIO)
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
 {
     if (!visitor->isAlive(m_audioSourceNode) && audioSourceProvider())
         audioSourceProvider()->setClient(nullptr);
 }
 #endif
 
 }