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
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org 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.
Comment on attachment 152382 [details] Patch Attachment 152382 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13237316
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!
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.
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
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.
Comment on attachment 158431 [details] Patch Attachment 158431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13504348
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.
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.
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.
Comment on attachment 160022 [details] Patch Clearing flags on attachment: 160022 Committed r126463: <http://trac.webkit.org/changeset/126463>
All reviewed patches have been landed. Closing bug.