Created attachment 126320 [details] Patch

Comment on attachment 126320 [details] Patch OK... webm isnt' a compile-time option for gstreamer?

Did you file a bug with gstreamer to fix their advertising? Can we remove this once they fix such?

(In reply to comment #3) > Did you file a bug with gstreamer to fix their advertising? Can we remove this once they fix such? Thanks for the review! Philippe, who typically handles the gstreamer integration, has asked upstream about fixing this. I guess it's kicked off an interesting discussion, but I don't have the details. One thing preventing us from removing the work-around is that we still want to work with older gstreamer versions. At some point, when our dependency jumps (likely when we drop support for pre-0.11 GStreamer) we can remove this work-around.

Comment on attachment 126320 [details] Patch It would be nice if you would explain that in the code. :) Ideally by linking to some gstreamer bug url (so that later hackers can know when to remove this).

(In reply to comment #2) > (From update of attachment 126320 [details]) > OK... webm isnt' a compile-time option for gstreamer? WebM support is shipped in separate plugins but the core of gstreamer provides what they call typefinders, able to detect the media container of a given input byte stream. (In reply to comment #3) > Did you file a bug with gstreamer to fix their advertising? Can we remove this once they fix such? Well the caps infrastructure used in gstreamer is different from the mime-types used in this code. In the future they might consider advertizing audio/webm but for the time being we need this workaround in webkit.

You all do what you need to do. :) My concerns are: 1. Making sure that your'e not inadverntaantly advertising support when you don't hav eit. 2. Adding code which will be obsolute (or worse, wrong) at a later date, w/o any comments in the code as to when to remove it. :) But those are general concerns. This is a pretty simple patch, and you all are both quite experianced at this webkit thing. I trust you do do the right things here (whatever those are).

(In reply to comment #5) > (From update of attachment 126320 [details]) > It would be nice if you would explain that in the code. :) Ideally by linking to some gstreamer bug url (so that later hackers can know when to remove this). Good point. Philippe, is the following comment accurate? // There isn't a one-to-one correspondance of caps to supported mime types in GStreamer. // Thus, we need to manually map from caps to supported mime-types here. At some point in // the future, GStreamer may map caps to mime-types directly and then we can remove this code. I didn't link to the the bug URL, because I'm not sure if this is considered a bug in upstream yet. Perhaps Philippe can help me out here... I also notice that the video/x-h264 cap doesn't add a video/x-h264 mime-type and neither does the video/x-theora add a video/x-theora mime type. Is that on purpose?

(In reply to comment #7) > 1. Making sure that your'e not inadverntaantly advertising support when you don't hav eit. At least in this case, it seems that the audio/x-vorbis cap implies audio/webm mime type support. > 2. Adding code which will be obsolute (or worse, wrong) at a later date, w/o any comments in the code as to when to remove it. :) I definitely think it makes sense to add a comment here explaining the situation.

(In reply to comment #8) > (In reply to comment #5) > > (From update of attachment 126320 [details] [details]) > > It would be nice if you would explain that in the code. :) Ideally by linking to some gstreamer bug url (so that later hackers can know when to remove this). > > Good point. Philippe, is the following comment accurate? > > // There isn't a one-to-one correspondance of caps to supported mime types in GStreamer. > // Thus, we need to manually map from caps to supported mime-types here. At some point in > // the future, GStreamer may map caps to mime-types directly and then we can remove this code. > It's ok apart from the last sentence. Here's my take :) At some point in the future, GStreamer may reduce the differences between some caps and some of the mime-types mentionned in this code. > I didn't link to the the bug URL, because I'm not sure if this is considered a bug in upstream yet. Perhaps Philippe can help me out here... > It's not considered a bug yet. I think if I open one now it will probably be quickly closed as INVALID :) But don't worry, I'm aware of this issue and eager to simplify this mimeTypeCache() function as soon as we can. > I also notice that the video/x-h264 cap doesn't add a video/x-h264 mime-type and neither does the video/x-theora add a video/x-theora mime type. Is that on purpose? Yes, I don't think it's necessary. For instance the media/W3C/video/canPlayType tests use video/mp4 and video/ogg.

Created attachment 126635 [details] Patch for landing

Comment on attachment 126635 [details] Patch for landing Rejecting attachment 126635 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11506144

Created attachment 126636 [details] Patch for landing, for real

Comment on attachment 126636 [details] Patch for landing, for real Clearing flags on attachment: 126636 Committed r107482: <http://trac.webkit.org/changeset/107482>

Comment on attachment 126320 [details] Patch Clearing review bit.