Move deferred loading logic from WebMediaPlayerClientImpl to HTMLMediaElement.

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 026d69d245791efb86d1062b9b882bd5d27e650f..94a33bc1df17ba77742be7ffd90c15f441e0dbe3 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -85,6 +85,7 @@
 using namespace std;
 using blink::WebInbandTextTrack;
 using blink::WebMimeRegistry;
+using blink::WebMediaPlayer;

[philipj_slow, 2014/03/08 19:38:40]

Move up one line to keep list sorted.

[acolwell GONE FROM CHROMIUM, 2014/03/10 21:53:31]

Done.

 
 namespace WebCore {
 
@@ -279,6 +280,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_lastTextTrackUpdateTime(-1)
     , m_textTracks(nullptr)
     , m_ignoreTrackDisplayUpdate(0)
+    , m_delayingLoad(false)
 #if ENABLE(WEB_AUDIO)
     , m_audioSourceNode(0)
 #endif
@@ -413,7 +415,7 @@ void HTMLMediaElement::parseAttribute(const QualifiedName& name, const AtomicStr
 
         // The attribute must be ignored if the autoplay attribute is present
         if (!autoplay() && m_player)
-            m_player->setPreload(m_preload);
+            setPlayerPreload();
 
     } else if (name == mediagroupAttr)
         setMediaGroup(value);
@@ -839,15 +841,13 @@ void HTMLMediaElement::loadResource(const KURL& url, ContentType& contentType, c
 
     WTF_LOG(Media, "HTMLMediaElement::loadResource - m_currentSrc -> %s", urlForLoggingMedia(m_currentSrc).utf8().data());
 
-    blink::WebMediaPlayer::LoadType loadType = blink::WebMediaPlayer::LoadTypeURL;
-
     startProgressEventTimer();
 
     // Reset display mode to force a recalculation of what to show because we are resetting the player.
     setDisplayMode(Unknown);
 
     if (!autoplay())
-        m_player->setPreload(m_preload);
+        setPlayerPreload();
 
     if (fastHasAttribute(mutedAttr))
         m_muted = true;
@@ -859,14 +859,11 @@ void HTMLMediaElement::loadResource(const KURL& url, ContentType& contentType, c
 
     if (url.protocolIs(mediaSourceBlobProtocol)) {
         if (isMediaStreamURL(url.string())) {
-            loadType = blink::WebMediaPlayer::LoadTypeMediaStream;
             m_userGestureRequiredForPlay = false;
         } else {
             m_mediaSource = HTMLMediaSource::lookup(url.string());
 
             if (m_mediaSource) {
-                loadType = blink::WebMediaPlayer::LoadTypeMediaSource;
-
                 if (!m_mediaSource->attachToElement(this)) {
                     // Forget our reference to the MediaSource, so we leave it alone
                     // while processing remainder of load failure.
@@ -878,7 +875,13 @@ void HTMLMediaElement::loadResource(const KURL& url, ContentType& contentType, c
     }
 
     if (attemptLoad && canLoadURL(url, contentType, keySystem)) {
-        m_player->load(loadType, url);
+        ASSERT(!webMediaPlayer());
+
+        if (m_preload == MediaPlayer::None) {
+            m_delayingLoad = true;

[philipj_slow, 2014/03/08 19:38:40]

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...

There are a bunch of things missing here, if you want to fix them. They were
missing before as well, so if you want to keep this refactoring without
observable changes that's fine too.

[acolwell GONE FROM CHROMIUM, 2014/03/10 21:53:31]

I just want to keep this CL as a simple move of existing functionality. I
figured we can do a sweep to bring things into line with the spec in later CLs.
Long-term I'd like to make the code in this file look a lot closer to the spec
text. I'm pretty sure we are aligned on that front. :)

[philipj_slow, 2014/03/12 05:21:01]

Sounds great!

+        } else {
+            m_player->load(loadType(), m_currentSrc, corsMode());
+        }
     } else {
         mediaLoadingFailed(MediaPlayer::FormatError);
     }
@@ -1681,7 +1684,9 @@ void HTMLMediaElement::prepareToPlay()
     if (m_havePreparedToPlay)
         return;
     m_havePreparedToPlay = true;
-    m_player->prepareToPlay();
+
+    if (m_delayingLoad)
+        startDelayedLoad();
 }
 
 void HTMLMediaElement::seek(double time, ExceptionState& exceptionState)
@@ -3139,6 +3144,8 @@ void HTMLMediaElement::clearMediaPlayer(int flags)
 
     closeMediaSource();
 
+    m_delayingLoad = false;
+
     clearMediaPlayerAndAudioSourceProviderClient();
 
     stopPeriodicTimers();
@@ -3602,14 +3609,14 @@ void HTMLMediaElement::applyMediaFragmentURI()
     }
 }
 
-MediaPlayerClient::CORSMode HTMLMediaElement::mediaPlayerCORSMode() const
+WebMediaPlayer::CORSMode HTMLMediaElement::corsMode() const
 {
     const AtomicString& crossOriginMode = fastGetAttribute(crossoriginAttr);
     if (crossOriginMode.isNull())
-        return Unspecified;
+        return WebMediaPlayer::CORSModeUnspecified;
     if (equalIgnoringCase(crossOriginMode, "use-credentials"))
-        return UseCredentials;
-    return Anonymous;
+        return WebMediaPlayer::CORSModeUseCredentials;
+    return WebMediaPlayer::CORSModeAnonymous;
 }
 
 void HTMLMediaElement::mediaPlayerSetWebLayer(blink::WebLayer* webLayer)
@@ -3647,4 +3654,32 @@ bool HTMLMediaElement::isInteractiveContent() const
     return fastHasAttribute(controlsAttr);
 }
 
+void HTMLMediaElement::setPlayerPreload()
+{
+    m_player->setPreload(m_preload);

[philipj_slow, 2014/03/08 19:38:40]

I had to look for a while before I understood why this can't be null. It turns
out we call createMediaPlayer in prepareForLoad() even though me might not need
it. Not in this review, but maybe it should be done at the start of the resource
fetch algorithm instead...

[acolwell GONE FROM CHROMIUM, 2014/03/10 21:53:31]

Agreed. I was surprised about this as well especially since one of the call
sites checks m_player and the other one doesn't. In general, I think we'll
probably want to revisit when m_player is actually created. Just preserving
existing behavior here for now.

+
+    if (m_delayingLoad && m_preload != MediaPlayer::None)
+        startDelayedLoad();
+}
+
+void HTMLMediaElement::startDelayedLoad()
+{
+    ASSERT(m_delayingLoad);
+
+    m_delayingLoad = false;
+
+    m_player->load(loadType(), m_currentSrc, corsMode());
+}
+
+WebMediaPlayer::LoadType HTMLMediaElement::loadType() const

[philipj_slow, 2014/03/08 19:38:40]

Maybe move these functions up together with their relatives, around load() or
loadResource()?

[acolwell GONE FROM CHROMIUM, 2014/03/10 21:53:31]

Done.

+{
+    if (m_mediaSource)
+        return WebMediaPlayer::LoadTypeMediaSource;
+
+    if (isMediaStreamURL(m_currentSrc.string()))
+        return WebMediaPlayer::LoadTypeMediaStream;
+
+    return WebMediaPlayer::LoadTypeURL;
+}
+
 }