Summary: | [GTK] Export an API similar to WebKit1 | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | eric, mrobinson, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Bug Depends on: | 57944 | ||||||||||||||||||
Bug Blocks: | 57835 | ||||||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2011-04-05 01:41:48 PDT
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.
The patch applies on top of patch attached to bug #54230 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.
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. Created attachment 89759 [details]
Updated patch
Patch updated to current git master.
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.
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.
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. 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.
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? (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. Created attachment 92059 [details]
Updated patch to current git master
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? 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.
(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. (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. (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. (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. 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.
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.
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. Look good! Committed r88631: <http://trac.webkit.org/changeset/88631> |