Bug 69506 - [GTK] Do not use C API in GTK+ API public headers
Summary: [GTK] Do not use C API in GTK+ API public headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 68434 69509
  Show dependency treegraph
 
Reported: 2011-10-06 01:02 PDT by Carlos Garcia Campos
Modified: 2011-10-06 09:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.33 KB, patch)
2011-10-06 01:05 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch to ignore WEBKIT_API in docs (10.10 KB, patch)
2011-10-06 08:56 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-10-06 01:02:34 PDT
We only use WK_EXPORT macro, but if the C API is eventually removed we will have to change all public headers, so I think it's better to just add our own macro like WebKit1 does
Comment 1 Carlos Garcia Campos 2011-10-06 01:05:55 PDT
Created attachment 109932 [details]
Patch
Comment 2 Martin Robinson 2011-10-06 07:59:59 PDT
Comment on attachment 109932 [details]
Patch

At this point I think it's best to avoid the code duplication. Even if the C API disappears it will be a while from now and we have time to fix the headers.
Comment 3 Carlos Garcia Campos 2011-10-06 08:02:47 PDT
We are wrapping the C API, it shouldn't be exposed from the GTK+ API.
Comment 4 Carlos Garcia Campos 2011-10-06 08:56:15 PDT
Created attachment 109964 [details]
Updated patch to ignore WEBKIT_API in docs

I still think we shoulnd't expose the C API at all in the public headers. The fact that we are wrapping the C API is just an implementation detail. We might decide not to distribute the C API in the future and use it a private library.
Comment 5 Martin Robinson 2011-10-06 09:17:53 PDT
Comment on attachment 109932 [details]
Patch

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

Okay...I think I'm convinced.

> Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h:45
> +#include <glib.h>
> +
> +#ifdef G_OS_WIN32
> +#    ifdef BUILDING_WEBKIT
> +#        define WEBKIT_API __declspec(dllexport)
> +#    else
> +#        define WEBKIT_API __declspec(dllimport)
> +#    endif
> +#    define WEBKIT_OBSOLETE_API WEBKIT_API
> +#else
> +#    define WEBKIT_API __attribute__((visibility("default")))
> +#    define WEBKIT_OBSOLETE_API WEBKIT_API __attribute__((deprecated))
> +#endif

You should not indent preprocessor defines.
Comment 6 Martin Robinson 2011-10-06 09:20:35 PDT
Comment on attachment 109964 [details]
Updated patch to ignore WEBKIT_API in docs

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

> Source/WebKit2/UIProcess/API/gtk/WebKitError.h:28
>  #include <glib.h>

You can remove this glib.h include now.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:27
>  #include <glib-object.h>

Ditto, I think.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.h:27
>  #include <glib-object.h>

Etc.
Comment 7 Carlos Garcia Campos 2011-10-06 09:56:36 PDT
Committed r96821: <http://trac.webkit.org/changeset/96821>