| Summary: | Add a Setting to disable QTKit media engine. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||
| Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | dglazkov, eric.carlson, feature-media-reviews, gtk-ews, mrowe, ojan.autocc, webkit-ews, webkit.review.bot, xan.lopez | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jer Noble
2013-01-15 18:40:03 PST
Created attachment 182922 [details]
Patch
Comment on attachment 182922 [details] Patch Attachment 182922 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15901333 Comment on attachment 182922 [details] Patch Attachment 182922 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15908264 Created attachment 182924 [details]
Patch
Added #if guards to protect ports without QTKit support.
Comment on attachment 182924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182924&action=review > Source/WebCore/platform/graphics/MediaPlayer.cpp:229 > +#if PLATFORM(MAC) || (PLATFORM(QT) && USE(QTKIT)) > + if (Settings::isQTKitEnabled()) > + MediaPlayerPrivateQTKit::registerMediaEngine(addMediaEngine); > +#endif > + Having the check here means it won't really be possible to have a control in an application's UI, because the user can't know if the media engines have already been registered or not (eg. loading a document WebCore doesn't handle natively has the side effect of registering all media engines to see if it is possible to create a media document). I think it would be more useful to always register the QTKit media engine, but then not use it when the pref is set by checking engineDescription() in bestMediaEngineForTypeAndCodecs(). (In reply to comment #6) > (From update of attachment 182924 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182924&action=review > > > Source/WebCore/platform/graphics/MediaPlayer.cpp:229 > > +#if PLATFORM(MAC) || (PLATFORM(QT) && USE(QTKIT)) > > + if (Settings::isQTKitEnabled()) > > + MediaPlayerPrivateQTKit::registerMediaEngine(addMediaEngine); > > +#endif > > + > > Having the check here means it won't really be possible to have a control in an application's UI, because the user can't know if the media engines have already been registered or not (eg. loading a document WebCore doesn't handle natively has the side effect of registering all media engines to see if it is possible to create a media document). I think it would be more useful to always register the QTKit media engine, but then not use it when the pref is set by checking engineDescription() in bestMediaEngineForTypeAndCodecs(). We don't have access to engineDescription() at that point, as bestMediaEngineForTypeAndCodecs() just uses the MediaEngine factory methods, not an instance of MediaPlayerPrivate. But even if it were available, that seems fragile (i.e., comparing strings returned from engineDescription()). What if we recreate the list of media engine factories whenever one of those preferences change? Comment on attachment 182924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182924&action=review >>> Source/WebCore/platform/graphics/MediaPlayer.cpp:229 >>> + >> >> Having the check here means it won't really be possible to have a control in an application's UI, because the user can't know if the media engines have already been registered or not (eg. loading a document WebCore doesn't handle natively has the side effect of registering all media engines to see if it is possible to create a media document). I think it would be more useful to always register the QTKit media engine, but then not use it when the pref is set by checking engineDescription() in bestMediaEngineForTypeAndCodecs(). > > We don't have access to engineDescription() at that point, as bestMediaEngineForTypeAndCodecs() just uses the MediaEngine factory methods, not an instance of MediaPlayerPrivate. But even if it were available, that seems fragile (i.e., comparing strings returned from engineDescription()). > > What if we recreate the list of media engine factories whenever one of those preferences change? Fair enough. Having to set the pref, quit, and relaunch is a pain but not the end of the world. Created attachment 183009 [details]
Patch
Notwithstanding ericc's r+, I've modified the patch to make the setting 'live'.
Comment on attachment 183009 [details]
Patch
Nice solution, thanks!
Comment on attachment 183009 [details] Patch Attachment 183009 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15903533 Comment on attachment 183009 [details] Patch Attachment 183009 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15912425 Comment on attachment 183009 [details] Patch Attachment 183009 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15925158 Committed r139899: <http://trac.webkit.org/changeset/139899> |