Bug 191147 - Fix build with VIDEO and WEB_AUDIO disabled
Summary: Fix build with VIDEO and WEB_AUDIO disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-01 02:44 PDT by Claudio Saavedra
Modified: 2018-11-02 10:46 PDT (History)
4 users (show)

See Also:


Attachments
Patch (9.57 KB, patch)
2018-11-01 02:57 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (9.44 KB, patch)
2018-11-01 03:29 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch for landing (1.57 KB, patch)
2018-11-01 07:54 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2018-11-01 02:44:39 PDT
Fix build with VIDEO and WEB_AUDIO disabled
Comment 1 Claudio Saavedra 2018-11-01 02:57:25 PDT
Created attachment 353590 [details]
Patch
Comment 2 Adrian Perez 2018-11-01 03:23:41 PDT
Comment on attachment 353590 [details]
Patch

Patch LGTM (informally reviewing).

Like you mention, building with all the multimedia support disabled is not
officially supported, but we try to make it work on a best-effort basis
because it can be hugely beneficial for embedded devices which do not
need media playback: not only WebKit is slightly smaller, but all the
disk space taken by GStreamer and friends is saved as well -- last time
I checked this amounted to twenty-something megabytes of disk space
savings for WPE.
Comment 3 Claudio Saavedra 2018-11-01 03:29:03 PDT
Created attachment 353591 [details]
Patch
Comment 4 Claudio Saavedra 2018-11-01 03:30:14 PDT
Patch was broken (mistakenly using VIDEO_TRACK instead of VIDEO in a few places). This one should work, let's see what the EWS bots say.
Comment 5 Adrian Perez 2018-11-01 03:33:06 PDT
Also, please add it in the backports list for 2.22.x:

   https://trac.webkit.org/wiki/WebKitGTK/2.22.x

Thanks!
Comment 6 Philippe Normand 2018-11-01 04:01:44 PDT
Comment on attachment 353591 [details]
Patch

Thanks!
Comment 7 WebKit Commit Bot 2018-11-01 04:27:19 PDT
Comment on attachment 353591 [details]
Patch

Clearing flags on attachment: 353591

Committed r237677: <https://trac.webkit.org/changeset/237677>
Comment 8 WebKit Commit Bot 2018-11-01 04:27:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-11-01 04:28:27 PDT
<rdar://problem/45726710>
Comment 10 Adrian Perez 2018-11-01 06:54:55 PDT
Building WPE with MiniBrowser enabled and VIDEO/WEB_AUDIO disabled
results in a build failure. The CMake invocation used was:

  % cmake -DPORT=WPE -DCMAKE_BUILD_TYPE=Release \
     -DENABLE_VIDEO=OFF -DENABLE_WEB_AUDIO=OFF -DENABLE_MINIBROWSER=ON

The build fails in the MiniBrowser code with the following error:

[100%] Building CXX object Tools/MiniBrowser/wpe/CMakeFiles/MiniBrowser.dir/main.cpp.o
.../WebKit/Tools/MiniBrowser/wpe/main.cpp:30:10: fatal error: gst/gst.h: No such file or directory
 #include <gst/gst.h>
          ^~~~~~~~~~~
compilation terminated.
Comment 11 Philippe Normand 2018-11-01 07:49:49 PDT
Comment on attachment 353591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353591&action=review

> Tools/MiniBrowser/gtk/main.c:32
> +#if ENABLE_WEB_AUDIO || ENABLE_VIDEO

Hah. This doesn't check the macro values. Can you follow-up, Claudio?
Comment 12 Philippe Normand 2018-11-01 07:50:42 PDT
Ok, nevermind... The issue is in wpe's minibrowser. Forget previous comment :)
Comment 13 Adrian Perez 2018-11-01 07:52:10 PDT
I have a fix already for the WPE MiniBrowser, it's just a
few lines so I will commit it as unreviewed build fix in
a moment =)
Comment 14 Adrian Perez 2018-11-01 07:54:24 PDT
Created attachment 353599 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2018-11-01 08:19:22 PDT
Comment on attachment 353599 [details]
Patch for landing

Clearing flags on attachment: 353599

Committed r237680: <https://trac.webkit.org/changeset/237680>
Comment 16 WebKit Commit Bot 2018-11-01 08:19:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Philippe Normand 2018-11-02 10:38:14 PDT
Comment on attachment 353591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353591&action=review

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2154
> +#if ENABLE(VIDEO)
>  void InspectorDOMAgent::addEventListenersToNode(Node& node)

Not sure how this couldn't break for you? InspectorInstrumentation::addEventListenersToNodeImpl() calls this method. My !video !web-audio build broke because of that, with a link error. I'll land another follow-up, the ENABLE(VIDEO) should be moved inside the method I think.
Comment 18 Philippe Normand 2018-11-02 10:46:40 PDT
https://trac.webkit.org/r237742