Bug 153135 – [EFL][GTK][WK2] Fix UIProcess build with GStreamer and without VIDEO

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Olivier Blin]

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;
                  ^

[Olivier Blin]

See also bug 153134 for a related fix in WebCore.

[Olivier Blin]

Created attachment 269068 [details]
Patch

[Michael Catanzaro]

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?

[Olivier Blin]

Created attachment 269114 [details]
Patch

[Olivier Blin]

(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.

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

HTMLMediaElement si the one starting the requestInstallMissingPlugins IIRC, and HTMLMediaElement is only compiled if ENABLE(VIDEO).

[Olivier Blin]

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?

[Michael Catanzaro]

(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.

[Olivier Blin]

Created attachment 269213 [details]
Patch

[Olivier Blin]

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.

[WebKit Commit Bot]

Comment on attachment 269213 [details]
Patch

Clearing flags on attachment: 269213

Committed r195231: <http://trac.webkit.org/changeset/195231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.