Add srcObject attribute of type MediaStream to HTMLMediaElement.

Add srcObject attribute of type MediaStream to HTMLMediaElement.

Intent to implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk

BUG=387740
Committed: https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}

[Guido Urdaneta, 2016-03-18 12:27:39]

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

[commit-bot: I haz the power, 2016-03-18 12:27:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815033003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815033003/20001

[Guido Urdaneta, 2016-03-18 13:22:33]

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

[commit-bot: I haz the power, 2016-03-18 13:22:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815033003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815033003/80001

[Guido Urdaneta, 2016-03-18 14:08:29]

Description was changed from

==========
Add srcObject attribute of type MediaStream to HTMLMediaElement.

BUG=387740
==========

to

==========
Add srcObject attribute of type MediaStream to HTMLMediaElement.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk

BUG=387740
==========

[Guido Urdaneta, 2016-03-18 14:17:15]

guidou@chromium.org changed reviewers:
+ haraken@chromium.org

[Guido Urdaneta, 2016-03-18 14:17:16]

Hi, PTAL

[commit-bot: I haz the power, 2016-03-18 14:33:22]

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

[commit-bot: I haz the power, 2016-03-18 14:33:23]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-03-18 23:37:39]

haraken@chromium.org changed reviewers:
+ philipj@opera.com

[haraken, 2016-03-18 23:37:40]

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.

2016

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
(right):

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h:30:
PersistentWillBeMember<MediaStream> m_srcObject;

Are you sure that the Persistent handle doesn't cause a leak?

- HTMLMediaElement and HTMLMediaElementMediaStream have the same lifetime.
- HTMLMediaElementMediaStream has a persistent to MediaStream.
- If the MediaStream has a strong reference to the DOM tree that contains the
HTMLMediaElement, there is a cycle.

(This issue will be gone once we remove the WillBe types, which will happen
soon.)

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:9:
] partial interface HTMLMediaElement {

Would you add:

  // TODO(haraken): Per the spec, the srcObject should be defined in
HTMLMediaElement.idl. However, we cannot define srcObject in
HTMLMediaElement.idl because of the dependency restriction from modules/ to
core/. For now we avoid the problem by using a partial interface.

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:10:
attribute MediaStream? srcObject;

The spec says that the type is MediaProvider?. Is it okay to use MediaStream? ?

[Guido Urdaneta, 2016-03-20 12:16:30]

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/03/18 23:37:40, haraken wrote:
> 
> 2016

Done.

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h
(right):

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.h:30:
PersistentWillBeMember<MediaStream> m_srcObject;
On 2016/03/18 23:37:40, haraken wrote:
> 
> Are you sure that the Persistent handle doesn't cause a leak?
> 
> - HTMLMediaElement and HTMLMediaElementMediaStream have the same lifetime.
> - HTMLMediaElementMediaStream has a persistent to MediaStream.
> - If the MediaStream has a strong reference to the DOM tree that contains the
> HTMLMediaElement, there is a cycle.
> 
> (This issue will be gone once we remove the WillBe types, which will happen
> soon.)

I ran a few experiments and observed that the lifecycles of the HTMLMediaElement
and the MediaStream objects were the same with assigment using the srcObject
attribute as with assigment via the src attribute and URL.createObjectURL. Note
that with the src attribute there is no Persistent reference. Thus, I think the
Persistent reference here changes nothing, given how things work today. This was
true with and without Oilpan.

However, that doesn't mean things are perfect.
For example, once a MediaStream is assigned to an element, it is registered in
MediaStreamRegistry, which has a persistent reference to the MediaStream. The
MediaStreams are GC'ed only when the registry is stopped.

There is also some reference from MediaStream to the DOM tree, although I don't
know exactly where it is. This is a brief summary of my experiments:

Experiment 1:
var v = document.createElement('video')
var ms = null
navigator.getUserMedia({video:true}, stream=>{ms=stream}, _=>{})
v = null;
gc(); // video is released
ms = null;
gc(); // mediastream is released

Experiment 2:
var v = document.createElement('video')
navigator.getUserMedia({video:true}, stream=>{v.srcObject=stream}, _=>{})
v.srcObject = null
gc(); //nothing is released
v = null
gc(); //video is released
// Reload page
gc(); //mediastream is released

Experiment 3:
var v = document.createElement('video')
navigator.getUserMedia({video:true}, stream=>{v.srcObject=stream}, _=>{})
v = null
gc(); //nothing is released
// Reload document
gc(); // video and mediastream are released

Like I said, the experiments above work the same for both srcObject and src
assigment, with and without oilpan.

My conclusion is that we should investigate this further.
I don't think this should block this CL, although it might be possible that
after we improve other things this Persistent reference might make a difference.
WDYT?

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:9:
] partial interface HTMLMediaElement {
On 2016/03/18 23:37:40, haraken wrote:
> 
> Would you add:
> 
>   // TODO(haraken): Per the spec, the srcObject should be defined in
> HTMLMediaElement.idl. However, we cannot define srcObject in
> HTMLMediaElement.idl because of the dependency restriction from modules/ to
> core/. For now we avoid the problem by using a partial interface.

Done.

https://codereview.chromium.org/1815033003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:10:
attribute MediaStream? srcObject;
On 2016/03/18 23:37:40, haraken wrote:
> 
> The spec says that the type is MediaProvider?. Is it okay to use MediaStream?
?

Now using MediaProvider, which is typedef'ed to be MediaStream for now.
According to the spec, it can be MediaStream, MediaSource or Blob.
For now we support only MediaStream, but added a TODO to support the other types
mentioned in the spec. 
As I explain in the I2I, supporting MediaStream puts us at the same level of
support of all the other major browsers.

[philipj_slow, 2016-03-21 11:13:13]

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

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
Rather than using an object URL, can you look into something like
Source/platform/MediaProvider which wraps the MediaStream (and later other
types), with the isMediaStream()/getAsMediaStream() pattern that generated union
types have?

This class (maybe it has to be public/platform/WebMediaProvider) would be used
by FrameLoaderClientImpl::createWebMediaPlayer or
RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS instance for the
MediaStream object.

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:29:
AtomicString url(URLMediaStream::createObjectURL(element.getExecutionContext(),
mediaStream));
If this URL is never revoked, won't the mediaStream object simply leak? Getting
rid of this problem is one of the main benefits of srcObject over
createObjectURL, if it can be fixed.

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:12:
] partial interface HTMLMediaElement {
As stated on blink-dev, I think this could be put in modules/srcobject, which
would be allowed to depend on modules/mediastream (now) and modules/mediasource
(later). Can you check if it's just a simple renaming?

[Guido Urdaneta, 2016-03-21 16:34:53]

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

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> Rather than using an object URL, can you look into something like
> Source/platform/MediaProvider which wraps the MediaStream (and later other
> types), with the isMediaStream()/getAsMediaStream() pattern that generated
union
> types have?
> 
> This class (maybe it has to be public/platform/WebMediaProvider) would be used
> by FrameLoaderClientImpl::createWebMediaPlayer or
> RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS instance for
the
> MediaStream object.


Source/platform and public/platform cannot access MediaStream due to dependency
restrictions. Fortunately, this dependency problem has already been solved for
MediaStreams with the MediaStreamDescriptor class in Source/platform.

The main difficulty I see of not using the URL is that all the code from
HTMLMediaElement to the creation of the WebMediaPlayer uses the URL, and it's in
the lowest layer where the MediaStreamDescriptor is obtained from the URL.

I think we would need alternative non-URL versions of at least
HTMLMediaElement::loadResource(), HTMLMediaElement::startPlayerLoad(),
FrameLoaderClientImpl::createWebMediaPlayer(), and
RenderFrameImpl::createMediaPlayer(). At first sight, it looks like a lot of
work. Of course, I'm not familiar with HTMLMediaElement code, so maybe there is
a way to do it without duplicating much code.

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:29:
AtomicString url(URLMediaStream::createObjectURL(element.getExecutionContext(),
mediaStream));
On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> If this URL is never revoked, won't the mediaStream object simply leak?
Getting
> rid of this problem is one of the main benefits of srcObject over
> createObjectURL, if it can be fixed.

What if we do revoke the URL? For example, when srcObject becomes null and
perhaps in other cases?

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl
(right):

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.idl:12:
] partial interface HTMLMediaElement {
On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> As stated on blink-dev, I think this could be put in modules/srcobject, which
> would be allowed to depend on modules/mediastream (now) and
modules/mediasource
> (later). Can you check if it's just a simple renaming?

Moving this to modules/srcobject works fine.
git cl presubmit does not complain.

[philipj_slow, 2016-03-22 16:06:54]

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

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
On 2016/03/21 16:34:53, Guido Urdaneta wrote:
> On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> > Rather than using an object URL, can you look into something like
> > Source/platform/MediaProvider which wraps the MediaStream (and later other
> > types), with the isMediaStream()/getAsMediaStream() pattern that generated
> union
> > types have?
> > 
> > This class (maybe it has to be public/platform/WebMediaProvider) would be
used
> > by FrameLoaderClientImpl::createWebMediaPlayer or
> > RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS instance for
> the
> > MediaStream object.
> 
> 
> Source/platform and public/platform cannot access MediaStream due to
dependency
> restrictions. Fortunately, this dependency problem has already been solved for
> MediaStreams with the MediaStreamDescriptor class in Source/platform.
> 
> The main difficulty I see of not using the URL is that all the code from
> HTMLMediaElement to the creation of the WebMediaPlayer uses the URL, and it's
in
> the lowest layer where the MediaStreamDescriptor is obtained from the URL.
> 
> I think we would need alternative non-URL versions of at least
> HTMLMediaElement::loadResource(), HTMLMediaElement::startPlayerLoad(),
> FrameLoaderClientImpl::createWebMediaPlayer(), and
> RenderFrameImpl::createMediaPlayer(). At first sight, it looks like a lot of
> work. Of course, I'm not familiar with HTMLMediaElement code, so maybe there
is
> a way to do it without duplicating much code.
> 

Just judging by include rules, maybe public/web/WebMediaProvider.h +
Source/web/WebMediaProvider.cpp would work?

As for HTMLMediaElement processing, the nicest thing would be an entry point
that matches
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo...
so that one can be confident that the implementation matches the spec. One idea
would be to let the WebMediaProvider represent both URLs and all kinds of
srcObjects and to use that throughout. Do you think that could work?

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp
(right):

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/HTMLMediaElementMediaStream.cpp:29:
AtomicString url(URLMediaStream::createObjectURL(element.getExecutionContext(),
mediaStream));
On 2016/03/21 16:34:53, Guido Urdaneta wrote:
> On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> > If this URL is never revoked, won't the mediaStream object simply leak?
> Getting
> > rid of this problem is one of the main benefits of srcObject over
> > createObjectURL, if it can be fixed.
> 
> What if we do revoke the URL? For example, when srcObject becomes null and
> perhaps in other cases?

Yes, if avoiding URLs is not possible right now, that'd be an improvement.

[Guido Urdaneta, 2016-03-24 14:38:44]

Hi, PTAL

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

https://codereview.chromium.org/1815033003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:111: void
setSrcObjectURL(const AtomicString&);
On 2016/03/22 16:06:54, philipj_UTC7 wrote:
> On 2016/03/21 16:34:53, Guido Urdaneta wrote:
> > On 2016/03/21 11:13:13, philipj_UTC7 wrote:
> > > Rather than using an object URL, can you look into something like
> > > Source/platform/MediaProvider which wraps the MediaStream (and later other
> > > types), with the isMediaStream()/getAsMediaStream() pattern that generated
> > union
> > > types have?
> > > 
> > > This class (maybe it has to be public/platform/WebMediaProvider) would be
> used
> > > by FrameLoaderClientImpl::createWebMediaPlayer or
> > > RenderFrameImpl::createMediaPlayer to create a WebMediaPlayerMS instance
for
> > the
> > > MediaStream object.
> > 
> > 
> > Source/platform and public/platform cannot access MediaStream due to
> dependency
> > restrictions. Fortunately, this dependency problem has already been solved
for
> > MediaStreams with the MediaStreamDescriptor class in Source/platform.
> > 
> > The main difficulty I see of not using the URL is that all the code from
> > HTMLMediaElement to the creation of the WebMediaPlayer uses the URL, and
it's
> in
> > the lowest layer where the MediaStreamDescriptor is obtained from the URL.
> > 
> > I think we would need alternative non-URL versions of at least
> > HTMLMediaElement::loadResource(), HTMLMediaElement::startPlayerLoad(),
> > FrameLoaderClientImpl::createWebMediaPlayer(), and
> > RenderFrameImpl::createMediaPlayer(). At first sight, it looks like a lot of
> > work. Of course, I'm not familiar with HTMLMediaElement code, so maybe there
> is
> > a way to do it without duplicating much code.
> > 
> 
> Just judging by include rules, maybe public/web/WebMediaProvider.h +
> Source/web/WebMediaProvider.cpp would work?
> 
> As for HTMLMediaElement processing, the nicest thing would be an entry point
> that matches
>
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo...
> so that one can be confident that the implementation matches the spec. One
idea
> would be to let the WebMediaProvider represent both URLs and all kinds of
> srcObjects and to use that throughout. Do you think that could work?

I went for the URL+srcObject approach. I called the class
public/platform/WebMediaElementSource. I did not use WebMediaProvider since the
spec uses that term for the non-URL case. I'm open to naming suggestions though
:)

I couldn't exactly match
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo...
 because that logic seems to be split across HTMLMediaElement and the various
WebMediaPlayers. Intead, I left the current loading logic practically
unmodified. Just ignoring URL checks when it's an srcObject.

Please let me know what you think.

[philipj_slow, 2016-03-24 17:43:33]

I'll be OOO until Tuesday, but maybe you will too :)

[philipj_slow, 2016-03-29 13:28:50]

Can you add some reviewers for everything outside third_party/WebKit/ to get
that started?

[philipj_slow, 2016-03-29 13:43:19]

I haven't looked very closely at HTMLMediaElement yet.

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:360: void
loadResource(ContentType&);
Can loadResource still take the thing to load as an argument? If not, why?

Note that this code is being refactored in
https://codereview.chromium.org/1810513002/ so there will be conflicts for
someone.

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaElementSource.h:33: WebMediaStream
m_mediaStream;
Is it possible to get the original MediaStream back from a WebMediaStream? If it
is, then it should be possible to let WebMediaElementSource be the only thing
that keeps it alive, which would be more like how I hope this will finally look
like, and will also mean that HTMLMediaElementSrcObject doesn't need to be a
supplement since the HTMLMediaElement itself will hold on to the
WebMediaElementSource object.

[Guido Urdaneta, 2016-03-29 13:45:16]

guidou@chromium.org changed reviewers:
+ jochen@chromium.org, tommi@chromium.org, wolenetz@chromium.org

[Guido Urdaneta, 2016-03-29 13:45:17]

tommi@chromium.org: Please review changes in content/renderer/media

jochen@chromium.org: Please review changes in content/public, content/shell and
content/renderer (feel free to review anything else, of course)

wolenetz@chromium.org: Please review changes in media/blink

[Guido Urdaneta, 2016-03-29 14:48:11]

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:360: void
loadResource(ContentType&);
On 2016/03/29 13:43:19, philipj_UTC7 wrote:
> Can loadResource still take the thing to load as an argument? If not, why?

The thing to load is now stored in the m_source member.
When it's an srcObject it has to be stored somewhere because there are
asynchronous tasks between setSrcObject() and loadResource().
 
> Note that this code is being refactored in
> https://codereview.chromium.org/1810513002/ so there will be conflicts for
> someone.

I think rebasing on top of that CL will not be an issue.

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):

https://codereview.chromium.org/1815033003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaElementSource.h:33: WebMediaStream
m_mediaStream;
On 2016/03/29 13:43:19, philipj_UTC7 wrote:
> Is it possible to get the original MediaStream back from a WebMediaStream? If
it
> is, then it should be possible to let WebMediaElementSource be the only thing
> that keeps it alive, which would be more like how I hope this will finally
look
> like, and will also mean that HTMLMediaElementSrcObject doesn't need to be a
> supplement since the HTMLMediaElement itself will hold on to the
> WebMediaElementSource object.

I just noticed it is possible to get the MediaStream from WebMediaStream as a
MediaStreamDescriptorClient. Updated HTMLMediaElementSrcObject to no longer be a
supplement.

[wolenetz, 2016-03-31 19:35:27]

blink/media mostly looking good % nit.

Note, you'll also need to update
content/renderer/media/android/webmediaplayer_android.{h,cc} ("WMPA")

My l*tm is pending WMPA change and a question in HTMLMediaElement.cpp:

https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:271: // Only URL is supported.
nit: please s/is supported/or MSE blob URL are supported/.

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

https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:563: if
(!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) {
On node insertion, if srcObject is used instead of srcAttr, could srcAttr still
be empty here? If so, I suspect a further condition is necessary here on
srcObject to allow load to occur of MediaStream srcObject to occur on node
insertion.

[Guido Urdaneta, 2016-04-01 12:14:57]

wolenetz: note that changes to WMPA were already in the CL. Can you also review
the rest of content/renderer/media?

https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1815033003/diff/280001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:271: // Only URL is supported.
On 2016/03/31 19:35:27, wolenetz wrote:
> nit: please s/is supported/or MSE blob URL are supported/.

Done.

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

https://codereview.chromium.org/1815033003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:563: if
(!getAttribute(srcAttr).isEmpty() && m_networkState == NETWORK_EMPTY) {
On 2016/03/31 19:35:27, wolenetz wrote:
> On node insertion, if srcObject is used instead of srcAttr, could srcAttr
still
> be empty here? If so, I suspect a further condition is necessary here on
> srcObject to allow load to occur of MediaStream srcObject to occur on node
> insertion.

I think you are right. Added extra condition.

[Guido Urdaneta, 2016-04-01 12:15:20]

guidou@chromium.org changed reviewers:
- tommi@chromium.org

[tommi (sloooow) - chröme, 2016-04-01 13:20:39]

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

[tommi (sloooow) - chröme, 2016-04-01 13:20:40]

lgtm for content/renderer/media

https://codereview.chromium.org/1815033003/diff/300001/content/renderer/media...
File content/renderer/media/web_media_element_source_utils.cc (right):

https://codereview.chromium.org/1815033003/diff/300001/content/renderer/media...
content/renderer/media/web_media_element_source_utils.cc:18: if (source.isURL())
nit: {}

[Guido Urdaneta, 2016-04-01 14:10:14]

guidou@chromium.org changed reviewers:
+ xhwang@chromium.org

[Guido Urdaneta, 2016-04-01 14:17:34]

+xhwang
xhwang: can you take a look at content/rendere/media/webmediaplayer_ms.cc lines
134 and 135?
You recently introduced a metric report that includes the MediaStream URL, but
now there might not be a URL for the MediaStream. I opted to report an empty URL
when this happens, but would like your input on this.

[hta - Chromium, 2016-04-01 15:08:14]

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

[hta - Chromium, 2016-04-01 15:08:15]

Added a few comments on layout tests.

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:24:
function playingFileOnce()
Style suggestion: name function playingFileFromSrcAttribute (the critical thing
isn't that it's played only once)

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:43:
video.srcObject=null;
Nit: Spaces around operator

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:46:
function playingFileTwice()
Style suggestion: Name function playing FileAfterSrcObjectRemoved

[Guido Urdaneta, 2016-04-02 18:32:52]

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:24:
function playingFileOnce()
On 2016/04/01 15:08:15, hta - Chromium wrote:
> Style suggestion: name function playingFileFromSrcAttribute (the critical
thing
> isn't that it's played only once)

Done.

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:43:
video.srcObject=null;
On 2016/04/01 15:08:15, hta - Chromium wrote:
> Nit: Spaces around operator

Done.

https://codereview.chromium.org/1815033003/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:46:
function playingFileTwice()
On 2016/04/01 15:08:15, hta - Chromium wrote:
> Style suggestion: Name function playing FileAfterSrcObjectRemoved

Done.

[philipj_slow, 2016-04-06 11:58:42]

Looks like we're on the right track, please see comments. The new module needs
an OWNER, I guess that's you :)

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:5:
<div id="log"></div>
You typically don't need to include this, it'll be inserted automatically.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:14:
var video = document.getElementById("testVideo");
You can skip the id and just use document.querySelector("video"). As little
markup and code as possible in tests is nice, gets right to the important bits
:)

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:15:
var trackevent = "playing";
Just inline "playing" everywhere?

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:21:
video.src = getVideoURI("test");
It'd be more clear if you set src and srcObject at the same time I think. If
you're trying to test something about the timing by doing at separate points,
being more explicit about that would be good, but I don't think that's the case.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:4: <video
id="testVideo" autoplay="autoplay"></video>
Nit: The autoplay attribute doesn't need any specific value, the empty string is
fine, which you'll get by just <video autoplay>.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:6: <p>Test
assigment of a mediastream via the srcObject attribute.</p>
Can you put this inside <title> at the top? This way, when there's only a single
test, that test gets the document.title as its title by default. Also capitalize
MediaStream.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9: <script
src="./w3c-media-utils.js"></script>
Doesn't look like you're using anything from w3c-media-utils.js.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:16:
test.step(() => assert_idl_attribute(video, "srcObject"));
You shouldn't need to wrap any individual steps like this. Just make sure that
each of these functions is appropriately wrapped in test.step_func(). This way
if exceptions are thrown where you're not anticipating them, like say if
navigator.webkitGetUserMedia is removed, it'll still fail the test properly.

You can avoid the startTest() method entirely by just doing async_test(test => {
/* the steps that start at this line */ }, /* no title if you have a <title>
above */).

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:17:
video.addEventListener("playing", playingSrcObject);
Here you'll need test.step_func(playingSrcObject). Similarly elsewhere.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22: stream
=> video.srcObject = stream,
The sequence of events could be a bit clearer if you added the playing event
listener in this callback, because I guess it's setting the srcObject which will
eventually cause the play to happen? An explicit call to play() and removing
autoplay would also be clearer, IMHO.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:23: _ =>
test.step(() => assert_unreached("Did not get mediastream")));
This can be test.unreached_func("Did not get MediaStream");

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:28:
video.removeEventListener("playing", playingSrcObject)
Is this needed? If it is, then using the onplaying attribute and setting it to
null here could be cleaner than saving the test.step_func(playingSrcObject)
value that you'd otherwise have to do.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
This is an oddly named callback. If you don't enjoy coming up with names for
callbacks, you can of course just nest everything. Go down to two spaces indent
and it may still be readable enough.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:33:
video.srcObject=null;
Needs more whitespace.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:40:
test.done();
When this is wrapped instead, you can use test.step_func_done() and omit the
step.done().

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

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if (m_networkState
== NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
Hmm, why this change? This block of code isn't per spec, did something break if
you didn't extend it to srcObject?

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:727: m_source =
srcObject;
Looking at this I'm thinking that it'd be cleaner if this is called just
m_srcObject and that before setting it you
ASSERT(srcObject.isMediaProviderObject()).

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:999: m_source =
WebMediaElementSource(WebURL(mediaURL));
Here, rather than setting m_source/m_srcObject, can you add back loadResource's
first argument and just pass it in here? I guess it can be const
WebMediaPlayerSource& and the WebMediaPlayer doesn't get any ownership, so it
has to extract the wrapped source from it before returning.

This should force the code into being more like the spec, it won't be possible
to keep some extra state in m_source/m_srcObject, which could be tempting even
if you haven't done it here.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1016: m_source =
WebMediaElementSource(WebURL(mediaURL));
Ditto.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1065: bool
isBlobProtocol = m_source.isMediaProviderObject() ||
url.protocolIs(mediaSourceBlobProtocol);
I guess isObjectOrBlobURL or something would be more accurate?

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1110: if
(m_source.isURL()) {
This will have to change with my proposed changes to m_srcObject. Will you be
able to use m_mode?

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp:21:
MediaStream* stream = static_cast<MediaStream*>(descriptor->client());
Can you introduce a free function toMediaStream(MediaStreamDescriptor*) or a
MediaStreamDescriptor::getAsMediaStream() instead so that the static_cast is
together with the code that makes it obviously safe?

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaElementSource.h:14: class
BLINK_PLATFORM_EXPORT WebMediaElementSource {
How about WebMediaPlayerSource as the name? It goes together with
WebMediaPlayer, and it's nice to contain the notion of a media element
(HTMLMediaElement) to inside Blink as much as possible.

[Guido Urdaneta, 2016-04-06 16:34:55]

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:5:
<div id="log"></div>
On 2016/04/06 11:58:41, philipj wrote:
> You typically don't need to include this, it'll be inserted automatically.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:14:
var video = document.getElementById("testVideo");
On 2016/04/06 11:58:41, philipj wrote:
> You can skip the id and just use document.querySelector("video"). As little
> markup and code as possible in tests is nice, gets right to the important bits
> :)

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:15:
var trackevent = "playing";
On 2016/04/06 11:58:41, philipj wrote:
> Just inline "playing" everywhere?

Done. Using onplaying instead.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:21:
video.src = getVideoURI("test");
On 2016/04/06 11:58:41, philipj wrote:
> It'd be more clear if you set src and srcObject at the same time I think. If
> you're trying to test something about the timing by doing at separate points,
> being more explicit about that would be good, but I don't think that's the
case.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:4: <video
id="testVideo" autoplay="autoplay"></video>
On 2016/04/06 11:58:42, philipj wrote:
> Nit: The autoplay attribute doesn't need any specific value, the empty string
is
> fine, which you'll get by just <video autoplay>.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:6: <p>Test
assigment of a mediastream via the srcObject attribute.</p>
On 2016/04/06 11:58:42, philipj wrote:
> Can you put this inside <title> at the top? This way, when there's only a
single
> test, that test gets the document.title as its title by default. Also
capitalize
> MediaStream.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9: <script
src="./w3c-media-utils.js"></script>
On 2016/04/06 11:58:41, philipj wrote:
> Doesn't look like you're using anything from w3c-media-utils.js.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:16:
test.step(() => assert_idl_attribute(video, "srcObject"));
On 2016/04/06 11:58:42, philipj wrote:
> You shouldn't need to wrap any individual steps like this. Just make sure that
> each of these functions is appropriately wrapped in test.step_func(). This way
> if exceptions are thrown where you're not anticipating them, like say if
> navigator.webkitGetUserMedia is removed, it'll still fail the test properly.
> 
> You can avoid the startTest() method entirely by just doing async_test(test =>
{
> /* the steps that start at this line */ }, /* no title if you have a <title>
> above */).

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:17:
video.addEventListener("playing", playingSrcObject);
On 2016/04/06 11:58:41, philipj wrote:
> Here you'll need test.step_func(playingSrcObject). Similarly elsewhere.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22: stream
=> video.srcObject = stream,
On 2016/04/06 11:58:42, philipj wrote:
> The sequence of events could be a bit clearer if you added the playing event
> listener in this callback, because I guess it's setting the srcObject which
will
> eventually cause the play to happen? An explicit call to play() and removing
> autoplay would also be clearer, IMHO.

Done, but kept the autoplay. With an explicit call to play() the test worked,
but I got the following console message: 
"Uncaught (in promise) AbortError: The play() request was interrupted by a new
load request."

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:23: _ =>
test.step(() => assert_unreached("Did not get mediastream")));
On 2016/04/06 11:58:41, philipj wrote:
> This can be test.unreached_func("Did not get MediaStream");

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:28:
video.removeEventListener("playing", playingSrcObject)
On 2016/04/06 11:58:42, philipj wrote:
> Is this needed? If it is, then using the onplaying attribute and setting it to
> null here could be cleaner than saving the test.step_func(playingSrcObject)
> value that you'd otherwise have to do.

Done. Not needed.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
On 2016/04/06 11:58:41, philipj wrote:
> This is an oddly named callback. If you don't enjoy coming up with names for
> callbacks, you can of course just nest everything. Go down to two spaces
indent
> and it may still be readable enough.

Done. What would have been a good name? :)

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:33:
video.srcObject=null;
On 2016/04/06 11:58:42, philipj wrote:
> Needs more whitespace.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:40:
test.done();
On 2016/04/06 11:58:42, philipj wrote:
> When this is wrapped instead, you can use test.step_func_done() and omit the
> step.done().

Done.

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

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if (m_networkState
== NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
On 2016/04/06 11:58:42, philipj wrote:
> Hmm, why this change? This block of code isn't per spec, did something break
if
> you didn't extend it to srcObject?

Nothing broke with or without this change.
wolenetz@ suggested this might be necessary and it made sense to me.
I reverted it for now.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:727: m_source =
srcObject;
On 2016/04/06 11:58:42, philipj wrote:
> Looking at this I'm thinking that it'd be cleaner if this is called just
> m_srcObject and that before setting it you
> ASSERT(srcObject.isMediaProviderObject()).

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:999: m_source =
WebMediaElementSource(WebURL(mediaURL));
On 2016/04/06 11:58:42, philipj wrote:
> Here, rather than setting m_source/m_srcObject, can you add back
loadResource's
> first argument and just pass it in here? I guess it can be const
> WebMediaPlayerSource& and the WebMediaPlayer doesn't get any ownership, so it
> has to extract the wrapped source from it before returning.
> 
> This should force the code into being more like the spec, it won't be possible
> to keep some extra state in m_source/m_srcObject, which could be tempting even
> if you haven't done it here.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1016: m_source =
WebMediaElementSource(WebURL(mediaURL));
On 2016/04/06 11:58:42, philipj wrote:
> Ditto.

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1065: bool
isBlobProtocol = m_source.isMediaProviderObject() ||
url.protocolIs(mediaSourceBlobProtocol);
On 2016/04/06 11:58:42, philipj wrote:
> I guess isObjectOrBlobURL or something would be more accurate?

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1110: if
(m_source.isURL()) {
On 2016/04/06 11:58:42, philipj wrote:
> This will have to change with my proposed changes to m_srcObject. Will you be
> able to use m_mode?

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/srcobject/HTMLMediaElementSrcObject.cpp:21:
MediaStream* stream = static_cast<MediaStream*>(descriptor->client());
On 2016/04/06 11:58:42, philipj wrote:
> Can you introduce a free function toMediaStream(MediaStreamDescriptor*) or a
> MediaStreamDescriptor::getAsMediaStream() instead so that the static_cast is
> together with the code that makes it obviously safe?

Done.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMediaElementSource.h (right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMediaElementSource.h:14: class
BLINK_PLATFORM_EXPORT WebMediaElementSource {
On 2016/04/06 11:58:42, philipj wrote:
> How about WebMediaPlayerSource as the name? It goes together with
> WebMediaPlayer, and it's nice to contain the notion of a media element
> (HTMLMediaElement) to inside Blink as much as possible.

Done.

[philipj_slow, 2016-04-07 15:38:11]

third_party/WebKit/ LGTM % a few questions and nits.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:22: stream
=> video.srcObject = stream,
On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> On 2016/04/06 11:58:42, philipj wrote:
> > The sequence of events could be a bit clearer if you added the playing event
> > listener in this callback, because I guess it's setting the srcObject which
> will
> > eventually cause the play to happen? An explicit call to play() and removing
> > autoplay would also be clearer, IMHO.
> 
> Done, but kept the autoplay. With an explicit call to play() the test worked,
> but I got the following console message: 
> "Uncaught (in promise) AbortError: The play() request was interrupted by a new
> load request."

Do you get a rejected promise even if you call play() after setting srcObject? I
think that should be fixed by a CL mlamouri@ is working on:
https://codereview.chromium.org/1865933002/

Just using autoplay is fine, though.

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:29:
video.addEventListener("emptied", playingNothing)
On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> On 2016/04/06 11:58:41, philipj wrote:
> > This is an oddly named callback. If you don't enjoy coming up with names for
> > callbacks, you can of course just nest everything. Go down to two spaces
> indent
> > and it may still be readable enough.
> 
> Done. What would have been a good name? :)

Maybe "emptied"? Just "playing" for the current function would also work.

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

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if (m_networkState
== NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> On 2016/04/06 11:58:42, philipj wrote:
> > Hmm, why this change? This block of code isn't per spec, did something break
> if
> > you didn't extend it to srcObject?
> 
> Nothing broke with or without this change.
> wolenetz@ suggested this might be necessary and it made sense to me.
> I reverted it for now.

What could now be broken when using srcObject is moving a media element between
documents. Even with the change it wouldn't work per spec, but it would work a
little bit. So if you want to write a test for it and think it makes sense, I'm
OK with that.

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:10:
var test = async_test(test => {
I'd suggest the same kinds of changes here, to contain everything inside the
async_test

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9: var
test = async_test(test => {
This looks a bit unusual, if you just move playingSrcObject into the inner
function you won't need the return value, and you can move the video variable in
as well.

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:28: bool
WebMediaPlayerSource::isEmpty() const
Hmm, this isn't actually used anywhere?

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43: bool
WebMediaPlayerSource::isMediaProviderObject() const
Maybe for the time being, just isMediaStream() will suffice? They do the same
thing now, and in HTMLMediaElement::loadResource there's actually a bit of
redundancy if one knows this.

[jochen (gone - plz use gerrit), 2016-04-08 02:22:35]

everything in content outside of media/ lgtm

[Guido Urdaneta, 2016-04-08 10:58:47]

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

https://codereview.chromium.org/1815033003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:627: if (m_networkState
== NETWORK_EMPTY && (!getAttribute(srcAttr).isEmpty() ||
m_source.isMediaProviderObject())) {
On 2016/04/07 15:38:11, philipj wrote:
> On 2016/04/06 16:34:54, Guido Urdaneta wrote:
> > On 2016/04/06 11:58:42, philipj wrote:
> > > Hmm, why this change? This block of code isn't per spec, did something
break
> > if
> > > you didn't extend it to srcObject?
> > 
> > Nothing broke with or without this change.
> > wolenetz@ suggested this might be necessary and it made sense to me.
> > I reverted it for now.
> 
> What could now be broken when using srcObject is moving a media element
between
> documents. Even with the change it wouldn't work per spec, but it would work a
> little bit. So if you want to write a test for it and think it makes sense,
I'm
> OK with that.

I think it makes sense to add the srcObject check to the condition. However, I
was unable to hit this condition. Whenever I tried moving an element with
nonempty src or srcObject, the networkState was never NETWORK_EMPTY.
Nevertheless, I added a layout test for moving an element with an srcObject to a
new document. It works regardless of this change.

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream-src-file.html:10:
var test = async_test(test => {
On 2016/04/07 15:38:11, philipj wrote:
> I'd suggest the same kinds of changes here, to contain everything inside the
> async_test

Done.

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/video-srcobject-mediastream.html:9: var
test = async_test(test => {
On 2016/04/07 15:38:11, philipj wrote:
> This looks a bit unusual, if you just move playingSrcObject into the inner
> function you won't need the return value, and you can move the video variable
in
> as well.

Done.

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:28: bool
WebMediaPlayerSource::isEmpty() const
On 2016/04/07 15:38:11, philipj wrote:
> Hmm, this isn't actually used anywhere?

Removed. Will add it in the future if we need it.

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43: bool
WebMediaPlayerSource::isMediaProviderObject() const
On 2016/04/07 15:38:11, philipj wrote:
> Maybe for the time being, just isMediaStream() will suffice? They do the same
> thing now, and in HTMLMediaElement::loadResource there's actually a bit of
> redundancy if one knows this.

I think isMediaProviderObject() makes the code easier to follow in terms of the
spec. I don't see much benefit in removing it, since our plan is to support the
other types anyway.

[Guido Urdaneta, 2016-04-08 11:00:15]

wolenetz@: friendly ping for media/

[philipj_slow, 2016-04-08 12:49:31]

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp
(right):

https://codereview.chromium.org/1815033003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebMediaPlayerSource.cpp:43: bool
WebMediaPlayerSource::isMediaProviderObject() const
On 2016/04/08 10:58:47, Guido Urdaneta wrote:
> On 2016/04/07 15:38:11, philipj wrote:
> > Maybe for the time being, just isMediaStream() will suffice? They do the
same
> > thing now, and in HTMLMediaElement::loadResource there's actually a bit of
> > redundancy if one knows this.
> 
> I think isMediaProviderObject() makes the code easier to follow in terms of
the
> spec. I don't see much benefit in removing it, since our plan is to support
the
> other types anyway.

Acknowledged.

[wolenetz, 2016-04-08 23:10:06]

media/ lgtm

[Guido Urdaneta, 2016-04-09 07:12:26]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-04-09 07:12:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
jochen@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1815033003/#ps420001
(title: "philipj's comments")

[commit-bot: I haz the power, 2016-04-09 07:12:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815033003/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815033003/420001

[commit-bot: I haz the power, 2016-04-09 08:32:37]

Description was changed from

==========
Add srcObject attribute of type MediaStream to HTMLMediaElement.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk

BUG=387740
==========

to

==========
Add srcObject attribute of type MediaStream to HTMLMediaElement.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk

BUG=387740

Committed: https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}
==========

[commit-bot: I haz the power, 2016-04-09 08:32:38]

Patchset 22 (id:??) landed as
https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}

[commit-bot: I haz the power, 2016-04-09 08:32:42]

Failed to apply the patch.

[Guido Urdaneta, 2016-04-09 08:40:42]

Description was changed from

==========
Add srcObject attribute of type MediaStream to HTMLMediaElement.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk

BUG=387740

Committed: https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}
==========

to

==========
Add srcObject attribute of type MediaStream to HTMLMediaElement.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/D20_5X-2nCk

BUG=387740

Committed: https://crrev.com/9bfe4e2f473b01d5039dd31c9b04cc8c78ff5c70
Cr-Commit-Position: refs/heads/master@{#386294}
==========

[sof, 2016-04-12 18:40:20]

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

[sof, 2016-04-12 18:40:22]

https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:495: WebMediaPlayerSource
m_srcObject;
This value object contains a WebMediaStream, which in turn contains a
WebPrivatePtr<MediaStreamDescriptor> (a persistent) -- have this been verified
not to hold on to too much? i.e., that there isn't a cycle back to
HTMLMediaElement somehow via that WebPrivatePtr<>.

[Guido Urdaneta, 2016-04-12 21:38:55]

https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1815033003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:495: WebMediaPlayerSource
m_srcObject;
On 2016/04/12 18:40:22, sof wrote:
> This value object contains a WebMediaStream, which in turn contains a
> WebPrivatePtr<MediaStreamDescriptor> (a persistent) -- have this been verified
> not to hold on to too much? i.e., that there isn't a cycle back to
> HTMLMediaElement somehow via that WebPrivatePtr<>.

I don't think there are any cycles, but I will replace the persistent reference
in an upcoming CL to avoid future problems.