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

Fixes the issue when the media element could be assigned a MediaStream object and then a src= URL that would autoplay as the element was unlocked for MediaStream.

BUG=620319
TEST=manual + a layout test

Committed: https://crrev.com/49e3b076201542b38774c2137e097f52ea15110f
Cr-Commit-Position: refs/heads/master@{#402777}

[whywhat, 2016-06-16 11:17:17]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2016-06-16 11:17:18]

PTaL

[whywhat, 2016-06-16 11:17:24]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 11:17:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075563003/1

[commit-bot: I haz the power, 2016-06-16 13:15:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 13:15:45]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-06-22 15:18:43]

Description was changed from

==========
[Android, Media] Lock the media element whenever it's source changes and it was
unlocked for MediaStream

Keep track whenever the user gesture requirement has been removed due to the
source being a MediaStream (via srcObject or src) and locking the element
again as soon as one of these attributes is changed.

BUG=620319
TEST=manual+a layout test
==========

to

==========
[Android, Media] Lock the media element whenever its source changes and it was
unlocked for MediaStream

Keep track whenever the user gesture requirement has been removed due to the
source being a MediaStream (via srcObject or src) and locking the element
again as soon as one of these attributes is changed.

BUG=620319
TEST=manual+a layout test
==========

[foolip, 2016-06-22 15:25:56]

foolip@chromium.org changed reviewers:
+ foolip@chromium.org

[foolip, 2016-06-22 15:25:56]

Might it work to instead change HTMLMediaElement::isGestureNeededForPlayback to
never require a user gesture for MediaStreams, and drop the unlock in
HTMLMediaElement::loadResource?

A potential problem would be that the paused state isn't reset when loading a
new resource, but I think that should be fixable, look for the
HTMLMediaElementLoadNetworkEmptyNotPaused use counter.

https://codereview.chromium.org/2075563003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2075563003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:597: bool
m_unlockedUserGestureForMediaStream : 1;
more state bits :)

[mlamouri (slow - plz ping), 2016-06-22 15:48:31]

So I was thinking of a similar solution. We should always autoplay if srcObject
is set (must be a MediaStream) OR src attribute is a MediaStream URL.

It seems that we rarely load new resource while playing so it's fine IMO. The
issue is definitely smaller than "unlocking" by setting a random MediaStream.

Also, Anton, can you update the test to use a MediaStream from Web Audio. I
think it's a better test than gUM because there is no user gesture + permission
involved.

[foolip, 2016-06-22 16:08:14]

On 2016/06/22 15:48:31, Mounir Lamouri (slow) wrote:
> So I was thinking of a similar solution. We should always autoplay if
srcObject
> is set (must be a MediaStream) OR src attribute is a MediaStream URL.

Sounds good. Since both the src attribute and srcObject can be present, care
must be taken to check the right one to not open up a new hole.

[whywhat, 2016-06-24 19:10:16]

Changed the implementation to the suggested one

[whywhat, 2016-06-24 19:11:43]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 19:12:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 20:20:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 20:20:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[whywhat, 2016-06-26 19:44:36]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-26 19:44:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2016-06-26 19:52:46]

Changed the test to not use getUserMedia

[whywhat, 2016-06-26 19:55:08]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-26 19:55:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2016-06-26 19:55:37]

PTAL

[commit-bot: I haz the power, 2016-06-26 22:21:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-26 22:21:27]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-06-27 12:28:09]

lgtm. I would like foolip@ to have a look in case of there is obvious issues we
missed here. Look really nice otherwise :)

https://codereview.chromium.org/2075563003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/autoplay-from-mediastream-to-src.html
(right):

https://codereview.chromium.org/2075563003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/autoplay-from-mediastream-to-src.html:12:
async_test(function(t) {
Can you add some empty lines. Like separate the `test`, the `window.internals`
call and the `async_test`. It's a bit hard to read as is.

[whywhat, 2016-06-27 14:02:14]

Whitespace changes

[whywhat, 2016-06-27 14:49:52]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 14:50:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2016-06-27 14:50:16]

https://codereview.chromium.org/2075563003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/autoplay-from-mediastream-to-src.html
(right):

https://codereview.chromium.org/2075563003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/autoplay-from-mediastream-to-src.html:12:
async_test(function(t) {
On 2016/06/27 at 12:28:09, Mounir Lamouri (slow) wrote:
> Can you add some empty lines. Like separate the `test`, the `window.internals`
call and the `async_test`. It's a bit hard to read as is.

Done.

[sof, 2016-06-27 15:10:46]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-06-27 15:10:47]

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1035: if
(isStreamOrBlobUrl) {
The logic looks a bit strange now - more readable as

  if (isStreamOrBlobUrl && !source.isMediaStream()) 

perhaps?

[whywhat, 2016-06-27 15:35:58]

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1035: if
(isStreamOrBlobUrl) {
On 2016/06/27 at 15:10:47, sof wrote:
> The logic looks a bit strange now - more readable as
> 
>   if (isStreamOrBlobUrl && !source.isMediaStream()) 
> 
> perhaps?

Ok, I guess the overall condition triggering the code below comes down to

((source.isMediaStream() || url.protocolIs(mediaSourceBlobProtocol) &&
!source.isMediaStream() && (!source.isURL() ||
!isMediaStreamURL(url.getString()))

We could definitely remove source.isMediaStrem() from the first bracket:

url.protocolIs(mediaSourceBlobProtocol) && !source.isMediaStream() &&
(!source.isURL() || !isMediaStreamURL(url.getString()))

Maybe replace with:

isMediaStream = source.isMediaStream() || (source.isURL() &&
isMediaStreamURL(url.getString()));
isMediaSourceUrl = url.protocolIs(mediaSourceBlobProtocol) && !isMediaStream;
if (isMediaSourceUrl) {
  m_mediaSource = ...
  ...
}

WDYT?

Hm, isStreamOrBlobUrl is used elsewhere :(

Seems like "source.isURL() && isMediaStreamURL(url.getString())" is important
for m_mediaSource logic nested below but not for isStreamOrBlobUrl below? Does
it mean registered media stream URLs are ignored sometimes?

[commit-bot: I haz the power, 2016-06-27 16:07:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 16:07:34]

Dry run: This issue passed the CQ dry run.

[sof, 2016-06-27 18:54:06]

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1035: if
(isStreamOrBlobUrl) {
On 2016/06/27 15:35:58, whywhat wrote:
> On 2016/06/27 at 15:10:47, sof wrote:
> > The logic looks a bit strange now - more readable as
> > 
> >   if (isStreamOrBlobUrl && !source.isMediaStream()) 
> > 
> > perhaps?
> 
> Ok, I guess the overall condition triggering the code below comes down to
> 
> ((source.isMediaStream() || url.protocolIs(mediaSourceBlobProtocol) &&
> !source.isMediaStream() && (!source.isURL() ||
> !isMediaStreamURL(url.getString()))
> 
> We could definitely remove source.isMediaStrem() from the first bracket:
> 
> url.protocolIs(mediaSourceBlobProtocol) && !source.isMediaStream() &&
> (!source.isURL() || !isMediaStreamURL(url.getString()))
> 
> Maybe replace with:
> 
> isMediaStream = source.isMediaStream() || (source.isURL() &&
> isMediaStreamURL(url.getString()));
> isMediaSourceUrl = url.protocolIs(mediaSourceBlobProtocol) && !isMediaStream;
> if (isMediaSourceUrl) {
>   m_mediaSource = ...
>   ...
> }
> 
> WDYT?
> 

That makes it a lot clearer, i.e., check if a media source blob: URL is for a
registered source. And try to use that media source, if so.

I'd just leave the isStreamOrBlobUrl as it is.

> Hm, isStreamOrBlobUrl is used elsewhere :(
> 
> Seems like "source.isURL() && isMediaStreamURL(url.getString())" is important
> for m_mediaSource logic nested below but not for isStreamOrBlobUrl below? Does
> it mean registered media stream URLs are ignored sometimes?

That question I'll leave to the grownups working with media element every day :)

[whywhat, 2016-06-27 18:59:26]

Shuffled the logic for MediaSource in load resource a bit

[whywhat, 2016-06-27 19:00:50]

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1035: if
(isStreamOrBlobUrl) {
On 2016/06/27 at 18:54:06, sof wrote:
> On 2016/06/27 15:35:58, whywhat wrote:
> > On 2016/06/27 at 15:10:47, sof wrote:
> > > The logic looks a bit strange now - more readable as
> > > 
> > >   if (isStreamOrBlobUrl && !source.isMediaStream()) 
> > > 
> > > perhaps?
> > 
> > Ok, I guess the overall condition triggering the code below comes down to
> > 
> > ((source.isMediaStream() || url.protocolIs(mediaSourceBlobProtocol) &&
> > !source.isMediaStream() && (!source.isURL() ||
> > !isMediaStreamURL(url.getString()))
> > 
> > We could definitely remove source.isMediaStrem() from the first bracket:
> > 
> > url.protocolIs(mediaSourceBlobProtocol) && !source.isMediaStream() &&
> > (!source.isURL() || !isMediaStreamURL(url.getString()))
> > 
> > Maybe replace with:
> > 
> > isMediaStream = source.isMediaStream() || (source.isURL() &&
> > isMediaStreamURL(url.getString()));
> > isMediaSourceUrl = url.protocolIs(mediaSourceBlobProtocol) &&
!isMediaStream;
> > if (isMediaSourceUrl) {
> >   m_mediaSource = ...
> >   ...
> > }
> > 
> > WDYT?
> > 
> 
> That makes it a lot clearer, i.e., check if a media source blob: URL is for a
registered source. And try to use that media source, if so.
> 
> I'd just leave the isStreamOrBlobUrl as it is.
> 
> > Hm, isStreamOrBlobUrl is used elsewhere :(
> > 
> > Seems like "source.isURL() && isMediaStreamURL(url.getString())" is
important
> > for m_mediaSource logic nested below but not for isStreamOrBlobUrl below?
Does
> > it mean registered media stream URLs are ignored sometimes?
> 
> That question I'll leave to the grownups working with media element every day
:)

Not sure if it became much more clearer but I took a stab at it. Further
suggestions are welcome.

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

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1036: if
(isStreamOrBlobUrl && !source.isMediaStream() && (!source.isURL() ||
!isMediaStreamURL(url.getString()))) {
Can (at least) be reduced to

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

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

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1040: if
(!m_mediaSource->attachToElement(this)) {
nit: the two ifs could be fused.

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

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1034: // MediaSource is
a blob URL that is not a media stream.
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().

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1035: bool
isStreamOrBlobUrl = source.isMediaStream() ||
url.protocolIs(mediaSourceBlobProtocol);
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.

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1222: if (m_srcObject
|| (!m_currentSrc.isNull() && isMediaStreamURL(m_currentSrc.getString())))
Did this assert somewhere before?

[whywhat, 2016-06-28 16:07:07]

Addressed follow up comments

[whywhat, 2016-06-28 16:09:34]

Description was changed from

==========
[Android, Media] Lock the media element whenever its source changes and it was
unlocked for MediaStream

Keep track whenever the user gesture requirement has been removed due to the
source being a MediaStream (via srcObject or src) and locking the element
again as soon as one of these attributes is changed.

BUG=620319
TEST=manual+a layout test
==========

to

==========
Fixes the issue when the media element could be assigned a MediaStream object
and then a src= URL that would autoplay as the element was unlocked for
MediaStream.

BUG=620319
TEST=manual + a layout test
==========

[whywhat, 2016-06-28 16:09:42]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 16:10:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[whywhat, 2016-06-28 16:13:51]

PTAL

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1034: // MediaSource is
a blob URL that is not a media stream.
On 2016/06/28 at 15:28:51, Philip Jägenstedt wrote:
> 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().

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().

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1035: bool
isStreamOrBlobUrl = source.isMediaStream() ||
url.protocolIs(mediaSourceBlobProtocol);
On 2016/06/28 at 15:28:51, Philip Jägenstedt wrote:
> 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.

Done.

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1036: if
(isStreamOrBlobUrl && !source.isMediaStream() && (!source.isURL() ||
!isMediaStreamURL(url.getString()))) {
On 2016/06/27 at 19:28:48, sof wrote:
> Can (at least) be reduced to
> 
> if (url.protocolIs(mediaSourceBlobProtocol) &&
!isMediaStreamURL(url.getString()))
> 
> ..but perhaps the RHS of (&&) is redundant?

I believe now it's even simpler.

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1040: if
(!m_mediaSource->attachToElement(this)) {
On 2016/06/27 at 19:28:48, sof wrote:
> nit: the two ifs could be fused.

Ditto.

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1222: if (m_srcObject
|| (!m_currentSrc.isNull() && isMediaStreamURL(m_currentSrc.getString())))
On 2016/06/28 at 15:28:51, Philip Jägenstedt wrote:
> Did this assert somewhere before?

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).

[commit-bot: I haz the power, 2016-06-28 17:34:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 17:34:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[whywhat, 2016-06-28 18:03:42]

Fixed media source test

[whywhat, 2016-06-28 18:04:38]

The CQ bit was checked by avayvod@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 18:05:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1034: // MediaSource is
a blob URL that is not a media stream.
On 2016/06/28 at 16:13:50, whywhat wrote:
> On 2016/06/28 at 15:28:51, Philip Jägenstedt wrote:
> > 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().
> 
> 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().

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.

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1040: if
(!m_mediaSource->attachToElement(this)) {
On 2016/06/28 at 16:13:50, whywhat wrote:
> On 2016/06/27 at 19:28:48, sof wrote:
> > nit: the two ifs could be fused.
> 
> Ditto.

Actually fused these diffs :)

[commit-bot: I haz the power, 2016-06-28 19:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 19:46:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2016-06-29 09:30:26]

foolip@chromium.org changed reviewers:
+ wolenetz@chromium.org

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

Adding wolenetz@. Previous review around this was
https://codereview.chromium.org/2049513002/

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2075563003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1034: // MediaSource is
a blob URL that is not a media stream.
On 2016/06/28 18:06:34, whywhat wrote:
> On 2016/06/28 at 16:13:50, whywhat wrote:
> > On 2016/06/28 at 15:28:51, Philip Jägenstedt wrote:
> > > 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().
> > 
> > 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().
> 
> 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.

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.

[foolip, 2016-06-29 10:06:59]

As discussed briefly over chat, lgtm as-is to leave the MSE change as a
follow-up. It would become a one-liner.

[whywhat, 2016-06-29 10:12:50]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2016-06-29 10:12:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2075563003/#ps140001
(title: "Fixed media source test")

[commit-bot: I haz the power, 2016-06-29 10:13:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 11:25:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-29 11:26:34]

Description was changed from

==========
Fixes the issue when the media element could be assigned a MediaStream object
and then a src= URL that would autoplay as the element was unlocked for
MediaStream.

BUG=620319
TEST=manual + a layout test
==========

to

==========
Fixes the issue when the media element could be assigned a MediaStream object
and then a src= URL that would autoplay as the element was unlocked for
MediaStream.

BUG=620319
TEST=manual + a layout test

Committed: https://crrev.com/49e3b076201542b38774c2137e097f52ea15110f
Cr-Commit-Position: refs/heads/master@{#402777}
==========

[commit-bot: I haz the power, 2016-06-29 11:26:36]

Patchset 8 (id:??) landed as
https://crrev.com/49e3b076201542b38774c2137e097f52ea15110f
Cr-Commit-Position: refs/heads/master@{#402777}