GStreamer builds fail when WebAudio is enabled but VIDEO disabled. In file included from WebKit/Source/WTF/wtf/FastMalloc.h:26:0, from WebKit/Source/WebKit2/config.h:44, from WebKit/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:29: WebKit/Source/WTF/wtf/StdLibExtras.h: In instantiation of ‘typename std::_Unique_if<T>::_Single_object std::make_unique(Args&& ...) [with T = WebKit::PageClientImpl; Args = {_GtkWidget*&}; typename std::_Unique_if<T>::_Single_object = std::unique_ptr<WebKit::PageClientImpl>]’: WebKit/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:493:67: required from here WebKit/Source/WTF/wtf/StdLibExtras.h:311:60: error: invalid new-expression of abstract class type ‘WebKit::PageClientImpl’ return unique_ptr<T>(new T(std::forward<Args>(args)...)); ^ In file included from WebKit/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:39:0: WebKit/Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h:45:7: note: because the following virtual functions are pure within ‘WebKit::PageClientImpl’: class PageClientImpl : public PageClient ^ In file included from WebKit/Source/WebKit2/UIProcess/API/gtk/PageClientImpl.h:33:0, from WebKit/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:39: WebKit/Source/WebKit2/UIProcess/PageClient.h:356:18: note: virtual bool WebKit::PageClient::decicePolicyForInstallMissingMediaPluginsPermissionRequest&) virtual bool decicePolicyForInstallMissingMediaPluginsPermissionRequest(InstallMissingMediaPluginsPermissionRequest&) = 0; ^
See also bug 153134 for a related fix in WebCore.
Created attachment 269068 [details] Patch
Comment on attachment 269068 [details] Patch Thanks for your patch. I agree these #ifdefs are incorrect, but I think the right fix is to run the permission request even when video is not enabled. For instance, say you navigate to a page that attempts to play MP3 media; the browser ought to be notified if the appropriate GStreamer plugin is not available. Carlos, why did you decide to use ENABLE(VIDEO) to guard this code?
Created attachment 269114 [details] Patch
(In reply to comment #3) > Comment on attachment 269068 [details] > Patch > > Thanks for your patch. I agree these #ifdefs are incorrect, but I think the > right fix is to run the permission request even when video is not enabled. > For instance, say you navigate to a page that attempts to play MP3 media; > the browser ought to be notified if the appropriate GStreamer plugin is not > available. This makes sense, I have updated the patch to do so. Build tested with and without video enabled.
Comment on attachment 269114 [details] Patch Hm, I see there's a significant amount of code in WebKitInstallMissingMediaPluginsPermissionRequest.cpp that is still guarded by ENABLE(VIDEO), so even though this compiles, this feature will surely be broken. Also, the permission request is never emitted due to the ENABLE(VIDEO) guard in WebKitWebView.cpp. This code was added in r188121. At the time, I noticed the ENABLE_VIDEO guards but assumed that ENABLE_WEB_AUDIO depended on ENABLE_VIDEO; that is not correct. Carlos, what do you think about this?
HTMLMediaElement si the one starting the requestInstallMissingPlugins IIRC, and HTMLMediaElement is only compiled if ENABLE(VIDEO).
There does not seem to be any code in WebKitWebAudioSourceGStreamer to detect missing codecs, so it is worth enabling this for WebAudio as well? If not, should I just resubmit the initial patch with a fixed ChangeLog?
(In reply to comment #8) > There does not seem to be any code in WebKitWebAudioSourceGStreamer to > detect missing codecs, so it is worth enabling this for WebAudio as well? > > If not, should I just resubmit the initial patch with a fixed ChangeLog? Yeah, looks like my initial review was wrong. #if ENABLE(VIDEO) it is, then! Thanks again for upstreaming this fix.
Created attachment 269213 [details] Patch
Rolled back to the initial (In reply to comment #9) > (In reply to comment #8) > > If not, should I just resubmit the initial patch with a fixed ChangeLog? > > Yeah, looks like my initial review was wrong. #if ENABLE(VIDEO) it is, then! > Thanks again for upstreaming this fix. I overlooked this as well when writing the patch, thank you both for your comments and review. I have attached the initial version back, with an updated ChangeLog.
Comment on attachment 269213 [details] Patch Clearing flags on attachment: 269213 Committed r195231: <http://trac.webkit.org/changeset/195231>
All reviewed patches have been landed. Closing bug.