[Android, Media] Don't unlock MediaStream elements; instead allow MS to autoplay without a gesture.

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index bff62e7bfdb250473405fbd5bc2215a8d99b8bf2..a7a7914e5b1edadc0b15d364035c0f0d809dcafe 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -1031,21 +1031,17 @@ void HTMLMediaElement::loadResource(const WebMediaPlayerSource& source, const Co
 
     bool attemptLoad = true;
 
+    // MediaSource is a blob URL that is not a media stream.

[foolip, 2016/06/28 15:28:51]

This comment doesn't seem quite accurate, if it's explaining when
isStreamOrBlobUrl is true at least.

Given that you've changed loadType() to always (?) work, may I suggest
simplifying like this?

m_mediaSource = HTMLMediaSource::lookup(url.getString());
if (m_mediaSource) { ... attach ... }
bool isObject = loadType() != WebMediaPlayer::LoadTypeURL;

AFAICT, there's no reason to check the protocol at all, and the constant name
mediaSourceBlobProtocol is misleading.

Then, use isObject below for canLoadResource and for bypassing deferLoad().

[whywhat, 2016/06/28 16:13:50]

Done. I inlined |isObject| since it made the expression positive (==
LoadTypeURL). Can we be sure a null url will not crash HTMLMediaSource::lookup?
StringImpl::rawHash() crashed on a null string in loadType().

[whywhat, 2016/06/28 18:06:34]

So loadType() doesn't catch corrupt or revoked media source URLs (see the
failing mediasource-preload.html test in patchset 7) so I changed it to
isStreamOrBlobUrl only inlined it in the if statement.

[foolip, 2016/06/29 09:30:27]

Actually I think that it's the test that should change here. Because
https://url.spec.whatwg.org/#concept-url-object talks about an associated object
and not about the URL scheme, the most natural way to spec this in
https://github.com/whatwg/html/pull/1037 was to just check if there's an object
associated with the URL, like what you had in PS6.

Can you revert back to PS6 and change the test to assert that an invalid "blob"
URL is treated like any other invalid URL? For example, the URL "blobzzz:x"
together with preload none would fire the suspend event, and then when the load
really starts you get an error.

MediaSourceRegistry::lookup(const String& url) has a null check, which is why it
didn't crash there.

     bool isStreamOrBlobUrl = source.isMediaStream() || url.protocolIs(mediaSourceBlobProtocol);

[foolip, 2016/06/28 15:28:51]

While you're here, may I suggest replacing mediaSourceBlobProtocol with just
"blob"? It's inline elsewhere in core/, and the name of this constant is
misleading.

[whywhat, 2016/06/28 16:13:50]

Done.

-    if (isStreamOrBlobUrl) {
-        bool isMediaStream = source.isMediaStream() || (source.isURL() && isMediaStreamURL(url.getString()));
-        if (isMediaStream) {
-            m_autoplayHelper->unlockUserGesture(GesturelessPlaybackEnabledByStream);
-        } else {
-            m_mediaSource = HTMLMediaSource::lookup(url.getString());
-
-            if (m_mediaSource) {
-                if (!m_mediaSource->attachToElement(this)) {
-                    // Forget our reference to the MediaSource, so we leave it alone
-                    // while processing remainder of load failure.
-                    m_mediaSource = nullptr;
-                    attemptLoad = false;
-                }
+    if (isStreamOrBlobUrl && !source.isMediaStream() && (!source.isURL() || !isMediaStreamURL(url.getString()))) {

[sof, 2016/06/27 19:28:48]

Can (at least) be reduced to

if (url.protocolIs(mediaSourceBlobProtocol) &&
!isMediaStreamURL(url.getString()))

..but perhaps the RHS of (&&) is redundant?

[whywhat, 2016/06/28 16:13:50]

I believe now it's even simpler.

+        m_mediaSource = HTMLMediaSource::lookup(url.getString());
+
+        if (m_mediaSource) {
+            if (!m_mediaSource->attachToElement(this)) {

[sof, 2016/06/27 19:28:48]

nit: the two ifs could be fused.

[whywhat, 2016/06/28 16:13:50]

Ditto.

[whywhat, 2016/06/28 18:06:34]

Actually fused these diffs :)

+                // Forget our reference to the MediaSource, so we leave it alone
+                // while processing remainder of load failure.
+                m_mediaSource = nullptr;
+                attemptLoad = false;
             }
         }
     }
@@ -1223,7 +1219,7 @@ WebMediaPlayer::LoadType HTMLMediaElement::loadType() const
     if (m_mediaSource)
         return WebMediaPlayer::LoadTypeMediaSource;
 
-    if (m_srcObject || isMediaStreamURL(m_currentSrc.getString()))
+    if (m_srcObject || (!m_currentSrc.isNull() && isMediaStreamURL(m_currentSrc.getString())))

[foolip, 2016/06/28 15:28:51]

Did this assert somewhere before?

[whywhat, 2016/06/28 16:13:50]

isMediaStreamURL is using a hash map that eventually calls into
StringImpl::rawHash() which crashes on a null string. I guess current src could
be null. I observed the crash in one of the Android WebRTC tests (you can find
it in the second patchset CQ results -
WebRtcGetUserMediaBrowserTest.RenderVideoTrackInMultipleTagsAndPause).

         return WebMediaPlayer::LoadTypeMediaStream;
 
     return WebMediaPlayer::LoadTypeURL;
@@ -3725,6 +3721,9 @@ bool HTMLMediaElement::isGestureNeededForPlayback() const
     if (!m_lockedPendingUserGesture)
         return false;
 
+    if (loadType() == WebMediaPlayer::LoadTypeMediaStream)
+        return false;
+
     // We want to allow muted video to autoplay if:
     // - the flag is enabled;
     // - Data Saver is not enabled;