Bug 65616

Summary: [GTK] Reorganize header files
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, mrobinson, nayankk, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 65178    
Attachments:
Description Flags
Patch
mrobinson: review-
Updated patch
none
New patch to use <webkit/webkitgtk.h>
none
New patch mrobinson: review+

Description Carlos Garcia Campos 2011-08-03 06:21:17 PDT
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.
Comment 1 Carlos Garcia Campos 2011-08-03 06:32:10 PDT
Created attachment 102777 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-03 06:34:56 PDT
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.
Comment 3 Martin Robinson 2011-08-03 06:42:54 PDT
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."
Comment 4 Martin Robinson 2011-08-03 06:46:04 PDT
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.
Comment 5 Carlos Garcia Campos 2011-08-03 07:46:35 PDT
(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.
Comment 6 Martin Robinson 2011-08-03 07:59:01 PDT
(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.
Comment 7 Carlos Garcia Campos 2011-08-03 08:09:02 PDT
Created attachment 102786 [details]
Updated patch
Comment 8 Alejandro G. Castro 2011-09-12 01:53:02 PDT
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.
Comment 9 Martin Robinson 2011-09-14 08:56:16 PDT
Xan or Gustavo, any thoughts on this?  I think we should get some more buy-in from the reviwers before doing this.
Comment 10 Gustavo Noronha (kov) 2011-09-19 05:05:17 PDT
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.
Comment 11 Philippe Normand 2011-09-22 00:20:13 PDT
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.
Comment 12 Xan Lopez 2011-09-22 05:26:13 PDT
(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>
Comment 13 Carlos Garcia Campos 2011-09-23 05:55:05 PDT
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.
Comment 14 WebKit Review Bot 2011-09-23 05:57:09 PDT
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.
Comment 15 Carlos Garcia Campos 2011-09-27 00:46:34 PDT
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>.
Comment 16 Martin Robinson 2011-09-27 00:53:43 PDT
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
Comment 17 Carlos Garcia Campos 2011-09-27 02:35:04 PDT
Committed r96087: <http://trac.webkit.org/changeset/96087>