Bug 153135 - [EFL][GTK][WK2] Fix UIProcess build with GStreamer and without VIDEO
Summary: [EFL][GTK][WK2] Fix UIProcess build with GStreamer and without VIDEO
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 153134
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-15 10:40 PST by Olivier Blin
Modified: 2016-01-18 07:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.43 KB, patch)
2016-01-15 10:47 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2016-01-15 15:55 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (5.50 KB, patch)
2016-01-18 02:09 PST, Olivier Blin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Blin 2016-01-15 10:40:49 PST
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;
                  ^
Comment 1 Olivier Blin 2016-01-15 10:41:21 PST
See also bug 153134 for a related fix in WebCore.
Comment 2 Olivier Blin 2016-01-15 10:47:01 PST
Created attachment 269068 [details]
Patch
Comment 3 Michael Catanzaro 2016-01-15 11:44:13 PST
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?
Comment 4 Olivier Blin 2016-01-15 15:55:16 PST
Created attachment 269114 [details]
Patch
Comment 5 Olivier Blin 2016-01-15 15:57:02 PST
(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 6 Michael Catanzaro 2016-01-15 18:41:00 PST
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?
Comment 7 Carlos Garcia Campos 2016-01-16 01:42:23 PST
HTMLMediaElement si the one starting the requestInstallMissingPlugins IIRC, and HTMLMediaElement is only compiled if ENABLE(VIDEO).
Comment 8 Olivier Blin 2016-01-16 03:28:03 PST
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?
Comment 9 Michael Catanzaro 2016-01-16 07:58:51 PST
(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.
Comment 10 Olivier Blin 2016-01-18 02:09:21 PST
Created attachment 269213 [details]
Patch
Comment 11 Olivier Blin 2016-01-18 02:12:16 PST
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 12 WebKit Commit Bot 2016-01-18 07:17:43 PST
Comment on attachment 269213 [details]
Patch

Clearing flags on attachment: 269213

Committed r195231: <http://trac.webkit.org/changeset/195231>
Comment 13 WebKit Commit Bot 2016-01-18 07:17:46 PST
All reviewed patches have been landed.  Closing bug.