Summary: | create different WebKit::WebMediaPlayer based on URL | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | wjia | ||||||||||
Component: | Media | Assignee: | wjia | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, peter+ews, scherkus, tkent, tkent+wkapi, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
wjia
2012-07-13 17:20:56 PDT
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. |