Bug 121943 – [MediaStream API] HTMLMediaElement should be able to use MediaStream as source

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Eric Carlson]

partial interface HTMLMediaElement
 {
                attribute MediaStream? srcObject;
};




<rdar://problem/15022712>

[Nick Diego Yamane (diegoyam)]

Created attachment 217891 [details]
Patch

[Thiago de Barros Lacerda]

Comment on attachment 217891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:324
> +#if ENABLE(MEDIA_STREAM)

Shouldn't this be guarded with ENABLE(VIDEO) too?

> Source/WebCore/html/HTMLMediaElement.cpp:753
> +#if ENABLE(MEDIA_STREAM)

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:147
> +#if ENABLE(MEDIA_STREAM)

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:808
> +#if ENABLE(MEDIA_STREAM)

Ditto.

[Eric Carlson]

Comment on attachment 217891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:324
>> +#if ENABLE(MEDIA_STREAM)
> 
> Shouldn't this be guarded with ENABLE(VIDEO) too?

Everything in this file is already guarded with ENABLE(VIDEO)

>> Source/WebCore/html/HTMLMediaElement.h:147
>> +#if ENABLE(MEDIA_STREAM)
> 
> Ditto.

Ditto.

[Nick Diego Yamane (diegoyam)]

Created attachment 217896 [details]
Added Xcode stuff

[Sam Weinig]

Comment on attachment 217891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:38
> +    ASSERT(mediaElement);

If this can never be null, can you change the function to take a HTMLMediaElement&.

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:40
> +    isNull = !mediaStream;

This is probably not your doing, but passing null out of band here seems wasteful, since we have an inband signal already (the value of the pointer).

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:46
> +    ASSERT(mediaElement);

If this can never be null, can you change the function to take a HTMLMediaElement&.

> Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.h:40
> +    static PassRefPtr<MediaStream> srcObject(HTMLMediaElement*, bool& isNull);

This should not return a PassRefPtr. Just good old MediaStream*.

> Source/WebCore/html/HTMLMediaElement.h:148
> +    PassRefPtr<MediaStream> srcObject() const { return m_mediaStreamSrcObject; }

This should not return a PassRefPtr. Just good old MediaStream*.

[kov's GTK+ EWS bot]

Comment on attachment 217891 [details]
Patch

Attachment 217891 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/37468030

[Nick Diego Yamane (diegoyam)]

(In reply to comment #5)
> (From update of attachment 217891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217891&action=review
> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:38
> > +    ASSERT(mediaElement);
> 
> If this can never be null, can you change the function to take a HTMLMediaElement&.

I suspect that's not possible, because this function is called by the auto-generated binding code, which in this case expects to pass a HTMLMediaElement* as the first parameter.

> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:40
> > +    isNull = !mediaStream;
> 
> This is probably not your doing, but passing null out of band here seems wasteful, since we have an inband signal already (the value of the pointer).
> 

'isNull' parameter is also parte of the interface expected by auto-generated code. I'm not sure if it's necessary to set it according to the pointer value...

> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.cpp:46
> > +    ASSERT(mediaElement);
> 
> If this can never be null, can you change the function to take a HTMLMediaElement&.
> 
> > Source/WebCore/Modules/mediastream/HTMLMediaElementMediaStream.h:40
> > +    static PassRefPtr<MediaStream> srcObject(HTMLMediaElement*, bool& isNull);
> 
> This should not return a PassRefPtr. Just good old MediaStream*.
> 
Ok, done.

> > Source/WebCore/html/HTMLMediaElement.h:148
> > +    PassRefPtr<MediaStream> srcObject() const { return m_mediaStreamSrcObject; }
> 
> This should not return a PassRefPtr. Just good old MediaStream*.

ok

[Nick Diego Yamane (diegoyam)]

Created attachment 217914 [details]
Proposed Patch

Fixing GTK build issues

[WebKit Commit Bot]

Comment on attachment 217914 [details]
Proposed Patch

Clearing flags on attachment: 217914

Committed r159797: <http://trac.webkit.org/changeset/159797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.