Bug 191147 – Fix build with VIDEO and WEB_AUDIO disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Claudio Saavedra]

Fix build with VIDEO and WEB_AUDIO disabled

[Claudio Saavedra]

Created attachment 353590 [details]
Patch

[Adrian Perez]

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.

[Claudio Saavedra]

Created attachment 353591 [details]
Patch

[Claudio Saavedra]

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.

[Adrian Perez]

Also, please add it in the backports list for 2.22.x:

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

Thanks!

[Philippe Normand]

Comment on attachment 353591 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 353591 [details]
Patch

Clearing flags on attachment: 353591

Committed r237677: <https://trac.webkit.org/changeset/237677>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45726710>

[Adrian Perez]

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.

[Philippe Normand]

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?

[Philippe Normand]

Ok, nevermind... The issue is in wpe's minibrowser. Forget previous comment :)

[Adrian Perez]

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 =)

[Adrian Perez]

Created attachment 353599 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353599 [details]
Patch for landing

Clearing flags on attachment: 353599

Committed r237680: <https://trac.webkit.org/changeset/237680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

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.

[Philippe Normand]

https://trac.webkit.org/r237742