Created attachment 148423 [details] Patch

Attachment 148423 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/MediaPlayer.h:206: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/HTMLMediaElement.h:131: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 148427 [details] Patch

Comment on attachment 148427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148427&action=review > Source/WebCore/ChangeLog:6 > + In the case of two media players where one is used for MediaStream, it needs to know the URL to check if it's > + a MediaStream generated blob, to return if it's supported or not. I don't understand this. Please elaborate. > Source/WebCore/ChangeLog:13 > + No new tests since there's no change on code behavior. If there is no change in behavior, why is the code change necessary?

This is related with bug 87514 [GStreamer] Adding Farstream backend to WebRTC implementation

Comment on attachment 148427 [details] Patch Attachment 148427 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13002018

Created attachment 148543 [details] Patch

(In reply to comment #4) > (From update of attachment 148427 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148427&action=review > > > Source/WebCore/ChangeLog:6 > > + In the case of two media players where one is used for MediaStream, it needs to know the URL to check if it's > > + a MediaStream generated blob, to return if it's supported or not. > > I don't understand this. Please elaborate. When the user calls webkitGetUserMedia it will create a blob specific for that stream. On GTK we've created an StreamMediaPlayer to be used for the propose of playing rtp streams created by the mediastream call, however we're having issues on how to identify if we should use this player or the gstreamer one, since the MediaPlayerPrivateInterface::supportsType(type,codec) doesn't have access to the media's url. With this patch the new player will be able to identify if the media is a mediastream blob created by us (using MediaStreamRegistry::registry().lookupMediaStreamDescriptor(url)), and then return MediaPlayer::IsSupported; While the players that don't implement MediaStream or those using the same player could just ignore that. > > > Source/WebCore/ChangeLog:13 > > + No new tests since there's no change on code behavior. > > If there is no change in behavior, why is the code change necessary? Since basically the current ports will just ignore the parameter, I initially thought that no tests would be necessary. But since it modifies the MediaPlayer API I'll check which tests need to be changed.

Comment on attachment 148543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148543&action=review > Source/WebCore/ChangeLog:11 > + adding url to MediaPlayer::supportsType > + > + In the case of two media players where one is used for MediaStream, it needs to know the URL to check if it's > + a MediaStream generated blob, to return if it's supported or not. > + > + Add url to supportsType > + https://bugs.webkit.org/show_bug.cgi?id=89514 > + > + Reviewed by NOBODY (OOPS!). The format is wrong. Usually it's Bug title link Reviewed by ... Explanation of the change.

Created attachment 148780 [details] Patch

Eric: if I've changed only the privateinterfaces API and its usage, I believe the current media tests will cover the modification, since the current players won't use the new parameter. The new parameter will be use when [1] is accepted Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp [1] http://pastebin.com/vHZA2K9g

Comment on attachment 148780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148780&action=review > Source/WebCore/ChangeLog:12 > + When a blob is created as the address for a Media Stream, the MediaEngine > + will ask it's players if they support that media. However, a player built > + for MediaStream needs to know to URL to decide if it's or it's not supported. Nit: "if it's or it's not supported" is slightly awkward, I suggest something like "if it is supported or not" > Source/WebCore/platform/graphics/MediaPlayer.h:426 > +typedef MediaPlayer::SupportsType (*MediaEngineSupportsType)(const String& type, const String& codecs, const String& keySystem, const String& url); Why not pass the url as a KURL rather than as a string? > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731 > +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationCF::supportsType(const String& type, const String& codecs, const String& url) > { > + UNUSED_PARAM(url); > + The "UNUSED_PARAM()" will be unnecessary if you leave out the parameter name, e.g.. const String& codecs, const String&) > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:678 > +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationObjC::supportsType(const String& type, const String& codecs, const String& url) > { > + UNUSED_PARAM(url); > + Ditto here and throughout the rest of the patch.

Created attachment 149005 [details] Patch

(In reply to comment #12) > (From update of attachment 148780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148780&action=review > > > Source/WebCore/ChangeLog:12 > > + When a blob is created as the address for a Media Stream, the MediaEngine > > + will ask it's players if they support that media. However, a player built > > + for MediaStream needs to know to URL to decide if it's or it's not supported. > > Nit: "if it's or it's not supported" is slightly awkward, I suggest something like "if it is supported or not" Agreed > > > Source/WebCore/platform/graphics/MediaPlayer.h:426 > > +typedef MediaPlayer::SupportsType (*MediaEngineSupportsType)(const String& type, const String& codecs, const String& keySystem, const String& url); > > Why not pass the url as a KURL rather than as a string? I was using String because it was enough and all the other parameters were Strings. But I agree what a KURL is semantic better and might be needed/wanted in the future. Fixed > > > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731 > > +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationCF::supportsType(const String& type, const String& codecs, const String& url) > > { > > + UNUSED_PARAM(url); > > + > > The "UNUSED_PARAM()" will be unnecessary if you leave out the parameter name, e.g.. const String& codecs, const String&) Right. Fixed for all code.

Created attachment 149008 [details] Patch

In that case I had to change m_url to be a KURL. I could construct a KURL for the supportsType calls, but changing m_url to KURL make more sense.

Comment on attachment 149008 [details] Patch Attachment 149008 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13072037

Created attachment 149037 [details] Patch

Comment on attachment 149037 [details] Patch Attachment 149037 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13038418

Created attachment 149044 [details] Patch

Comment on attachment 149044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149044&action=review This looks great modulo the two minor issues noted. Thanks! > Source/WebCore/ChangeLog:15 > + for MediaStream needs to know to URL to decide if it's supported or not. > + > + > + * dom/DOMImplementation.cpp: Minor nit: extra blank line here. > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731 > + UNUSED_PARAM(url); > + Oops, it looks like you forgot to change this one.

Created attachment 149051 [details] Patch

(In reply to comment #21) > (From update of attachment 149044 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149044&action=review > > This looks great modulo the two minor issues noted. Thanks! > > > Source/WebCore/ChangeLog:15 > > + for MediaStream needs to know to URL to decide if it's supported or not. > > + > > + > > + * dom/DOMImplementation.cpp: > > Minor nit: extra blank line here. > > > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:731 > > + UNUSED_PARAM(url); > > + > > Oops, it looks like you forgot to change this one. Thanks for the review! Can you add it to the queue?

Comment on attachment 149051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149051&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:730 > +MediaPlayer::SupportsType MediaPlayerPrivateAVFoundationCF::supportsType(const String& type, const String& codecs, const KURL& url) > { > // Only return "IsSupported" if there is no codecs parameter for now as there is no way to ask if it supports an This will fail to compile: "const KURL& url" -> "const KURL&"

Created attachment 149056 [details] Patch

Comment on attachment 149056 [details] Patch @Danilo, I am adding this to the commit queue because you asked in a comment, but in the future you can just set the cq? flag to signal to the reviewer that you would like the patch to be added to the commit queue when it gets an r+. Thanks!

Comment on attachment 149056 [details] Patch Clearing flags on attachment: 149056 Committed r121053: <http://trac.webkit.org/changeset/121053>

All reviewed patches have been landed. Closing bug.