Bug 91301 – create different WebKit::WebMediaPlayer based on URL

• Component: Media
• Status: RESOLVED FIXED

In Chromium, it's better to create a different WebKit::WebMediaPlayer based on URL, since media stream has different characteristics than regular media playback. Therefore, the URL is needed when frame client's createMediaPlayer() is called.

A prototype patch can be found at http://codereview.chromium.org/10382048/

Created attachment 152382 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

The patch is uploaded for review first. It's pending on Chromium patch http://codereview.chromium.org/10537091/. Due to API change, this patch will be landed after Chromium patch is landed.

[WebKit Review Bot]

Comment on attachment 152382 [details]
Patch

Attachment 152382 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13237316

[Eric Carlson]

Comment on attachment 152382 [details]
Patch

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

> Tools/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Oops!

[Darin Fisher (:fishd, Google)]

Comment on attachment 152382 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:91
> +    virtual WebMediaPlayer* createMediaPlayer(const WebURL&, WebFrame*, WebMediaPlayerClient*) { return 0; }

nit: WebFrameClient methods always take a WebFrame pointer as their first parameter.

[Andrew Scherkus]

Comment on attachment 152382 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:3
> +        create different WebKit::WebMediaPlayer based on URL

This describes the intent but not the patch contents

This should be more along the lines of "Pass URL to WebFrameClient::createMediaPlayer()" with the additional information below describing why we want to do this (i.e., "Permits creation of different WebMediaPlayer implementations based on the URL.")

> Source/WebKit/chromium/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please fill out

[Andrew Scherkus]

Whoops my other comments didn't get published!

Nits aside -- I think the approach is OK.

As pointed out in http://codereview.chromium.org/10537091/ an alternative would involve relying on the media engine registrar + supportsType() by registering multiple media engines (i.e., the existing one and another one for media stream URLs)

If we wanted to go that path I think we'd have to plumb the registrar mechanism through the Chromium WebKit API (i.e., ask Chromium to register multiple engines). In the end I think we'd end up with an even more indirect way of creating WebMediaPlayer implementations so I'm not sure it'd be worth it.

Created attachment 158431 [details]
Patch

Please take another look. Need LGTM from this patch in order to land chromium patch http://codereview.chromium.org/10537091/. Then this patch can be landed.

[WebKit Review Bot]

Comment on attachment 158431 [details]
Patch

Attachment 158431 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13504348

[Peter Beverloo (cr-android ews)]

Comment on attachment 158431 [details]
Patch

Attachment 158431 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13491998

Created attachment 159795 [details]
Patch

ping. With chromium patch being landed, this patch is ready to go.

[Adam Barth]

Comment on attachment 159795 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:48
> -static PassOwnPtr<WebMediaPlayer> createWebMediaPlayer(WebMediaPlayerClient* client, Frame* frame)
> +static PassOwnPtr<WebMediaPlayer> createWebMediaPlayer(WebMediaPlayerClient* client, const String& url, Frame* frame)

It seems like this function should take a KURL rather than a String.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:324
> +    m_webMediaPlayer = createWebMediaPlayer(this, m_url, frame);

So, m_url is a String?  Why not a KURL?

Created attachment 160022 [details]
Patch

(In reply to comment #15)
> (From update of attachment 159795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159795&action=review
> 
> > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:48
> > -static PassOwnPtr<WebMediaPlayer> createWebMediaPlayer(WebMediaPlayerClient* client, Frame* frame)
> > +static PassOwnPtr<WebMediaPlayer> createWebMediaPlayer(WebMediaPlayerClient* client, const String& url, Frame* frame)
> 
> It seems like this function should take a KURL rather than a String.
> 
> > Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:324
> > +    m_webMediaPlayer = createWebMediaPlayer(this, m_url, frame);
> 
> So, m_url is a String?  Why not a KURL?

Changed to use KURL in createWebMediaPlayer() in new patch. Not sure why m_url is a String, instead of KURL.

[Adam Barth]

Comment on attachment 160022 [details]
Patch

Thanks.  A good followup patch would be to see if we can change m_url to be a KURL.

[WebKit Review Bot]

Comment on attachment 160022 [details]
Patch

Clearing flags on attachment: 160022

Committed r126463: <http://trac.webkit.org/changeset/126463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.