Bug 65616 – [GTK] Reorganize header files

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

[Carlos Garcia Campos]

Header files are currently installed under $prefix/include/webkit-<api-version> directory, but directiory name should be webkitgtk-<api-version> since we renamed the library.

[Carlos Garcia Campos]

Created attachment 102777 [details]
Patch

[WebKit Review Bot]

Attachment 102777 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwindow.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testatk.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebdatasource.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdownload.c"
Source/WebKit/gtk/webkitgtk.h:21:  #ifndef header guard has wrong style, please use: webkitgtk_h  [build/header_guard] [5]
Source/WebKit/gtk/webkitgtk.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomnode.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testnetworkrequest.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testatkroles.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testapplicationcache.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebbackforwardlist.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testcopyandpaste.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebsettings.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdomwindow.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testhittestresult.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testloading.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebhistoryitem.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebresource.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testnetworkresponse.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebframe.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebplugindatabase.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testglobals.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdocument.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testkeyevents.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testhttpbackend.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testmimehandling.c"
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 102777 [details]
Patch

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

This patch should probably also add webkitgtk.h to the list of headers skipped by the style bot.

> GNUmakefile.am:44
> +libwebkitgtkincludedir := $(prefix)/include/webkitgtk-@WEBKITGTK_API_VERSION@

libwebkitgtk_include_dir?

> Source/WebKit/gtk/GNUmakefile.am:34
> +# Global header
> +libwebkitgtkinclude_HEADERS += \
> +	$(srcdir)/Source/WebKit/gtk/webkitgtk.h
> +

Is 32libwebkitgtkinclude_HEADERS a primary? I don't see it referenced anywhere else. What is the significance of the string "libwebkitgtkinclude". I think this should be next to the other list of headers.

> Source/WebKit/gtk/webkit/webkit.h:21
> +#warning "<webkit/webkit.h> is deprecated, use <webkitgtk.h> instead."

Maybe say  "Including <webkit/webkit.h> is deprecated, include <webkitgtk.h> directly instead."

[Martin Robinson]

Just want to say that I support this change. It does two things mainly:

* Headers are installed to $PREFIX/include/webkitgtk-3.0.
* WebKitGTK+ embedders now include "webkitgtk.h" in the same style as GLib.

This change should be backward compatible, since -I flags are abstracted with pkg-config and it preserves webkitgtk.h.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 102777 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102777&action=review
> 
> This patch should probably also add webkitgtk.h to the list of headers skipped by the style bot.
> 
> > GNUmakefile.am:44
> > +libwebkitgtkincludedir := $(prefix)/include/webkitgtk-@WEBKITGTK_API_VERSION@
> 
> libwebkitgtk_include_dir?

I'm not sure you can use _ before dir, I think the pattern is foodir, foot_DATA, foo_HEADER, etc. like pkgconfigdir, typelibsdir, ...

> > Source/WebKit/gtk/GNUmakefile.am:34
> > +# Global header
> > +libwebkitgtkinclude_HEADERS += \
> > +	$(srcdir)/Source/WebKit/gtk/webkitgtk.h
> > +
> 
> Is 32libwebkitgtkinclude_HEADERS a primary? I don't see it referenced anywhere else. What is the significance of the string "libwebkitgtkinclude". I think this should be next to the other list of headers.

Yes, it's initialized in /GNUmakefile.am. For the name I followed the same pattern used by glib and gtk (glibincludedir, libgtkincludedir)

> > Source/WebKit/gtk/webkit/webkit.h:21
> > +#warning "<webkit/webkit.h> is deprecated, use <webkitgtk.h> instead."
> 
> Maybe say  "Including <webkit/webkit.h> is deprecated, include <webkitgtk.h> directly instead."

Ok.

[Martin Robinson]

(In reply to comment #5)

> I'm not sure you can use _ before dir, I think the pattern is foodir, foot_DATA, foo_HEADER, etc. like pkgconfigdir, typelibsdir, ...

Ah, I see. 

> Yes, it's initialized in /GNUmakefile.am. For the name I followed the same pattern used by glib and gtk (glibincludedir, libgtkincludedir)

This works together with libwebkitincludedir.

We just need to get some buy-in from Xan and Gustavo for me to r+ this patch.

[Carlos Garcia Campos]

Created attachment 102786 [details]
Updated patch

[Alejandro G. Castro]

Comment on attachment 102786 [details]
Updated patch

The change looks good to me, we should try to add this for the 1.6 this week.

[Martin Robinson]

Xan or Gustavo, any thoughts on this?  I think we should get some more buy-in from the reviwers before doing this.

[Gustavo Noronha (kov)]

I have nothing against it, not sure about using #include <webkitgtk.h> only, though. I never understood why all other libraries use a directory but maybe there's a good reason? In any case, if Xan does not oppose it, I'm pro.

[Philippe Normand]

Comment on attachment 102786 [details]
Updated patch

LGTM but I'd rather wait on Xan :) This change should probably mentionned in the next release NEWS.

[Xan Lopez]

(In reply to comment #10)
> I have nothing against it, not sure about using #include <webkitgtk.h> only, though. I never understood why all other libraries use a directory but maybe there's a good reason? In any case, if Xan does not oppose it, I'm pro.

AFAIK <foo/foo.h> vs. <foo.h> is just a matter of style for the most part. IMHO <foo/foo.h> indicates better the namespacing, but that's about it. The GNOME platform strongly prefers <foo/foo.h>, I can only think of glib-object.h and cairo.h being "different", and the glib case is really old, with all the new libraries following the new pattern (like gio/gio.h).

So my preference would be to try to do something like <webkit/webkitgtk.h> and only if it's really troublesome go for <webkitgtk.h>

[Carlos Garcia Campos]

Created attachment 108467 [details]
New patch to use <webkit/webkitgtk.h>

I also prefer the pattern foo/foo.h, so I've changed this patch and the webkit2 one to use webkit/webkitgtk.h and webkit2/webkit2gtk.h.

[WebKit Review Bot]

Attachment 108467 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwindow.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testatk.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebdatasource.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdownload.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomnode.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testnetworkrequest.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testatkroles.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testapplicationcache.c"
Source/WebKit/gtk/webkit/webkitgtk.h:21:  #ifndef header guard has wrong style, please use: webkitgtk_h  [build/header_guard] [5]
Source/WebKit/gtk/webkit/webkitgtk.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebbackforwardlist.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testcopyandpaste.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebsettings.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdomwindow.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testhittestresult.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testloading.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebhistoryitem.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebresource.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testnetworkresponse.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebframe.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebplugindatabase.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testglobals.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdocument.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testkeyevents.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testhttpbackend.c"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testmimehandling.c"
Total errors found: 2 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 108803 [details]
New patch

After discussing this again yesterday we decided to try to use webkit2/webkit2.h for webkit2, so that we don't need to change wk1 header. This patch just installs the headers under $prefix/webkigtk-<api-version>.

[Martin Robinson]

Comment on attachment 108803 [details]
New patch

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

Seems reasonable to me!

> GNUmakefile.am:44
> +libwebkitgtkincludedir := $(prefix)/include/webkitgtk-@WEBKITGTK_API_VERSION@

Please call this libwebkitgtk_include_dir if possible

[Carlos Garcia Campos]

Committed r96087: <http://trac.webkit.org/changeset/96087>