Bug 178100

Summary: [WPE] Header cairo.h is used in GLib API headers but cannot be found
Product: WebKit Reporter: Adrian Perez <aperez>
Component: WPE WebKitAssignee: Adrian Perez <aperez>
Status: RESOLVED WONTFIX    
Severity: Normal CC: berto, bugs-noreply, buildbot, cgarcia, commit-queue, gns, mcatanzaro, webkit-bug-importer, zan
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176475
https://bugs.webkit.org/show_bug.cgi?id=178104
Bug Depends on:    
Bug Blocks: 178125    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Adrian Perez 2017-10-09 16:52:31 PDT
When a program tries to use the GLib-based API for WPE (see
bug #178090 for a patch to make it installable), trying to
use the <wpe/webkit.h> header will result in the compilation
error shown below, due to “cairo.h” not being in the headers
search path.

---

In file included from /home/aperez/devel/wpe/buildroot/output/host/arm-buildroot-linux-gnueabihf/sysroot/usr/include/wpe-0.1/WPE/wpe/webkit.h:46:0,
                 from /home/aperez/devel/wpe/buildroot/output/build/dinghy-custom/dinghy.c:8:
/home/aperez/devel/wpe/buildroot/output/host/arm-buildroot-linux-gnueabihf/sysroot/usr/include/wpe-0.1/WPE/wpe/WebKitFaviconDatabase.h:27:19: fatal error: cairo.h: No such file or directory
 #include <cairo.h>
                   ^
Comment 1 Adrian Perez 2017-10-09 16:54:01 PDT
I think we can fix this by adding “cairo” to the “Requires.private”
field of the .pc generated to be used by pkg-config. I'll try that
out and submit a patch.
Comment 2 Adrian Perez 2017-10-09 17:00:14 PDT
Created attachment 323245 [details]
Patch
Comment 3 Zan Dobersek 2017-10-10 04:37:02 PDT
Can we instead just use a forward declaration of cairo_surface_t, and drop the cairo.h include?
Comment 4 Adrian Perez 2017-10-10 04:48:54 PDT
(In reply to Zan Dobersek from comment #3)
> Can we instead just use a forward declaration of cairo_surface_t, and drop
> the cairo.h include?

Very likely! I'll give that a try and update the patch accordingly
once I am sure it works.  :-)
Comment 5 Adrian Perez 2017-10-10 05:21:54 PDT
Created attachment 323299 [details]
Patch
Comment 6 Adrian Perez 2017-10-10 05:24:52 PDT
(In reply to Adrian Perez from comment #4)
> (In reply to Zan Dobersek from comment #3)
> > Can we instead just use a forward declaration of cairo_surface_t, and drop
> > the cairo.h include?
> 
> Very likely! I'll give that a try and update the patch accordingly
> once I am sure it works.  :-)

Worked perfectly, the new version of the patch adds only the
forward declaration and removes the #include \o/
Comment 7 Adrian Perez 2017-10-10 05:26:26 PDT
Ouch, we'll have to add “#include <cairo.h>” to the .cpp files which
make use of “WebKitFaviconDatabase.h”, as Good Guy EWS™ has found out.
Comment 8 Adrian Perez 2017-10-10 11:34:17 PDT
Created attachment 323326 [details]
Patch
Comment 9 Build Bot 2017-10-10 11:35:19 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 10 Zan Dobersek 2017-10-10 11:42:18 PDT
Comment on attachment 323326 [details]
Patch

Thanks, I appreciate the change.
Comment 11 WebKit Commit Bot 2017-10-10 12:09:54 PDT
Comment on attachment 323326 [details]
Patch

Clearing flags on attachment: 323326

Committed r223136: <http://trac.webkit.org/changeset/223136>
Comment 12 WebKit Commit Bot 2017-10-10 12:09:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Michael Catanzaro 2017-10-10 12:39:08 PDT
But you need to update the GTK header as well. We have to keep these headers completely in sync. Only exception should be symbols that do not exist in one port or the other.
Comment 14 Michael Catanzaro 2017-10-10 12:41:10 PDT
Reverted r223136 for reason:

Forgot to update GTK API header

Committed r223138: <http://trac.webkit.org/changeset/223138>
Comment 15 Michael Catanzaro 2017-10-10 12:42:45 PDT
(This was probably overkill for a rollout, but it's easier for backporting to land one fixed patch instead of an incremental fixup.)
Comment 16 Adrian Perez 2017-10-10 12:52:38 PDT
(In reply to Michael Catanzaro from comment #13)
> But you need to update the GTK header as well. We have to keep these headers
> completely in sync. Only exception should be symbols that do not exist in
> one port or the other.

I'll prepare a new version of the patch that covers both WPE and GTK+.
Comment 17 Adrian Perez 2017-10-10 13:08:37 PDT
Created attachment 323336 [details]
Patch
Comment 18 Michael Catanzaro 2017-10-10 13:48:22 PDT
You shouldn't have to #include a new header to use any types from our API. Zan's instincts to avoid unneeded forward declarations are good usually, but not in public headers IMO.

Thanks to gtk-doc for catching this problem on the GTK EWS. :P
Comment 19 Adrian Perez 2017-10-10 13:54:13 PDT
Created attachment 323347 [details]
Patch
Comment 20 Michael Catanzaro 2017-10-10 14:26:44 PDT
Comment on attachment 323347 [details]
Patch

Perfect!
Comment 21 WebKit Commit Bot 2017-10-10 14:32:56 PDT
Comment on attachment 323347 [details]
Patch

Clearing flags on attachment: 323347

Committed r223146: <http://trac.webkit.org/changeset/223146>
Comment 22 WebKit Commit Bot 2017-10-10 14:32:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Michael Catanzaro 2017-10-11 05:42:11 PDT
Reverted r223146 for reason:

Better to not expose cairo in the WPE API

Committed r223174: <http://trac.webkit.org/changeset/223174>
Comment 24 Michael Catanzaro 2017-10-11 05:42:34 PDT
We'll address this in bug #178157 instead.