Bug 57820 – [GTK] Export an API similar to WebKit1

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Currently the minibrowser is using the common C API, we should build an API, similar to WebKit1 API or the same if possible, on top of the common C API.

[Carlos Garcia Campos]

Created attachment 88190 [details]
Patch

Move gtk API from UIProcess/gtk to UIProcess/API/gtk. WebView and WebViewWidget have been merged into WebKitWebView, the public view widget that provides the same API than WebKit1. PageClient implementation has been moved to PageClientImpl. WKView and WKAPICastGtk have been removed since they are no longer needed.

[Carlos Garcia Campos]

The patch applies on top of patch attached to bug #54230

[Martin Robinson]

Comment on attachment 88190 [details]
Patch

We want to support both the WebKit2-style and WebKit1-style API. Thus, the WebKit1 API should sit fully on top of the WebKit2 API. The WebKit1-style widget needs it's own Clients, so I'd like to keep it separate from the WebKit2-widget, which should be mostly private.

[Carlos Garcia Campos]

Created attachment 88406 [details]
Patch to add WebKitWebView

This patch adds WebKitWebView on top of WebKitWebViewBase (see patch attached to bug #57944). It also enables GtkLauncher build for webkit2. There are some runtime warnings when running on webkit2, but they are harmless.

[Carlos Garcia Campos]

Created attachment 89759 [details]
Updated patch

Patch updated to current git master.

[Carlos Garcia Campos]

Created attachment 90909 [details]
Updated patch

New patch, the only difference with previous one is that this one includes a webkit.h file for compatibility with webkit1 gtk API. GtkLauncher still uses a macro, but only for missing features, so it will be removed soon and the code will be exactly the same for webkit1 and webkit2.

[Martin Robinson]

Comment on attachment 90909 [details]
Updated patch

Going to remove the review flag from this one, since the changes to the dependency will almost certainly mean this needs a new patch.

[Carlos Garcia Campos]

Created attachment 91669 [details]
New patch updated to current git master

Instead of using the same GtkLauncher, it builds GtkLauncher2 so that when bug #59695 is fixed there will be two different binaries.

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/gtk/webkit/webkit.h:26:  #ifndef header guard has wrong style, please use: WTF_webkit_h  [build/header_guard] [5]
Source/WebKit2/UIProcess/API/gtk/webkit/webkit.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:61:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:72:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:75:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:78:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:40:  webkit_web_view_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:45:  webkit_web_view_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:55:  webkit_web_view_load_uri is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:64:  webkit_web_view_go_back is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:72:  webkit_web_view_go_forward is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 18 in 12 files


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

[Martin Robinson]

Comment on attachment 91669 [details]
New patch updated to current git master

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:16
> +/*
> + * Copyright (C) 2011 Igalia S.L.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS

Instead of duplicating this header, would it be possible to reuse the one from Webkit1?

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 91669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91669&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:16
> > +/*
> > + * Copyright (C) 2011 Igalia S.L.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> > + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
> 
> Instead of duplicating this header, would it be possible to reuse the one from Webkit1?

Yes, sure.

[Carlos Garcia Campos]

Created attachment 92059 [details]
Updated patch to current git master

[Martin Robinson]

Comment on attachment 92059 [details]
Updated patch to current git master

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:21
> +#include "WebKitWebView.h"

Why not just put the WebKit1 header in the include path instead of completely duplicating the header?

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:69:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:72:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:75:  The parameter name "webView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/webkit/webkit.h:20:  #ifndef header guard has wrong style, please use: WTF_webkit_h  [build/header_guard] [5]
Source/WebKit2/UIProcess/API/gtk/webkit/webkit.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:34:  webkit_web_view_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:39:  webkit_web_view_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:49:  webkit_web_view_load_uri is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:58:  webkit_web_view_go_back is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:66:  webkit_web_view_go_forward is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 18 in 10 files


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

[Carlos Garcia Campos]

(In reply to comment #13)
> (From update of attachment 92059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92059&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:21
> > +#include "WebKitWebView.h"
> 
> Why not just put the WebKit1 header in the include path instead of completely duplicating the header?

Because headers might be different in the future. The existing API in webkit1 will be kept in webkit2 to ensure the compatibility, but we might want to add new methods to the API that don't make sense in webkit1. For example, when we add a wrapper for WKContext, we could add webkit_web_view_new_with_context() to the webkit2 API.

[Martin Robinson]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 92059 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92059&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:21
> > > +#include "WebKitWebView.h"
> > 
> > Why not just put the WebKit1 header in the include path instead of completely duplicating the header?
> 
> Because headers might be different in the future. The existing API in webkit1 will be kept in webkit2 to ensure the compatibility, but we might want to add new methods to the API that don't make sense in webkit1. For example, when we add a wrapper for WKContext, we could add webkit_web_view_new_with_context() to the webkit2 API.

For now, I think it would be better to explore sharing as much of the headers as possible. If we need to deviate from the WebKit1 header we could simply have a wrapper include.

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > (From update of attachment 92059 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=92059&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:21
> > > > +#include "WebKitWebView.h"
> > > 
> > > Why not just put the WebKit1 header in the include path instead of completely duplicating the header?
> > 
> > Because headers might be different in the future. The existing API in webkit1 will be kept in webkit2 to ensure the compatibility, but we might want to add new methods to the API that don't make sense in webkit1. For example, when we add a wrapper for WKContext, we could add webkit_web_view_new_with_context() to the webkit2 API.
> 
> For now, I think it would be better to explore sharing as much of the headers as possible. If we need to deviate from the WebKit1 header we could simply have a wrapper include.

How can we share a header if they are not exactly the same? both apis are supposed to be parallel installable even if the apis are similar. If we add an #ifdef WEBKIT2 to all headers, clients will have to pass that macro somehow when building.

[Xan Lopez]

(In reply to comment #16)
> 
> For now, I think it would be better to explore sharing as much of the headers as possible. If we need to deviate from the WebKit1 header we could simply have a wrapper include.

I agree sharing as much code as possible makes sense, but I think having exactly the same header for both is not possible even to begin with (among others for the reasons Carlos has explained). So IMHO some form of shared header that both wk1 and wk2 include could be the way to go.

[Carlos Garcia Campos]

Created attachment 96727 [details]
Patch rebased to current git master

As suggested by xan, I've moved the common declarations to a common header file that is part of webkit1, but included from webkit2 api.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:53:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/webkit/webkit.h:20:  #ifndef header guard has wrong style, please use: WTF_webkit_h  [build/header_guard] [5]
Source/WebKit2/UIProcess/API/gtk/webkit/webkit.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:34:  webkit_web_view_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:39:  webkit_web_view_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:49:  webkit_web_view_load_uri is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:58:  webkit_web_view_go_back is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:66:  webkit_web_view_go_forward is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 15 in 14 files


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

[Martin Robinson]

Comment on attachment 96727 [details]
Patch rebased to current git master

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:44
> +GtkWidget* webkit_web_view_new(void)

I think we should avoid the void for empty parameter lists in C++ code, if possible.

[Martin Robinson]

Look good!

[Carlos Garcia Campos]

Committed r88631: <http://trac.webkit.org/changeset/88631>