| Summary: | [GTK] Use WebKitWebContext in WebKitWebView | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||
| Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | gustavo, mrobinson, nayankk, pnormand, xan.lopez | ||||||||
| Priority: | P2 | Keywords: | Gtk | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Bug Depends on: | 67931 | ||||||||||
| Bug Blocks: | 68085, 68371 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Carlos Garcia Campos
2011-09-13 01:18:20 PDT
Created attachment 107151 [details]
Patch
Seems like you missed adding WebKitWebContextPrivate.h? Created attachment 107986 [details]
Updated patch
Add missing WebKitWebContextPrivate.h as pointed out by Nayan.
Comment on attachment 107986 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=107986&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:153 > +/* Private API */ it should be apparent because it's in WebKit coding style. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:154 > +WKContextRef webkitWebContextGetContext(WebKitWebContext* context) Probably better to call it something like: webkitWebContextGetWKContextRef to eliminate ambiguity. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:49 > + webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetContext(webView->priv->context), 0); Might as well do: webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView->priv->context)), 0); > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:113 > + * Creates a new #WebKitWebView with the default context. > + * See also webkit_web_view_new_with_context. > + * Somewhere we need to explain what a web context represents. I think it's sufficient to link to #WebKitWebContext here and ensure the documentation there is good. i.e. Creates a new #WebKitWebView with the default #WebKitWebContext. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:127 > + * @context: the #WebKitWebContext to be used by the #WebKitWebView > + * > + * Creates a new #WebKitWebView with the given context. > + * > + * Returns: The newly created #WebKitWebView widget Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:140 > + * Gets the web context of @web_view. Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-63 > }; > > WK_EXPORT GType > -webkit_web_view_get_type (void); This is why I think we should stop lining up the argument lists here. It's standard in GLib headers, but this kind of churn hurts patch readability and makes "git blame" useless. > Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:40 > + WebKitWebView *view = WEBKIT_WEB_VIEW(webkit_web_view_new()); > + g_object_ref_sink(view); > + g_assert(webkit_web_view_get_context(view) == webkit_web_context_get_default()); > + g_object_unref(view); > +} > + > +int main(int argc, char **argv) > +{ > + g_thread_init(NULL); > + gtk_test_init(&argc, &argv, NULL); > + > + g_test_bug_base("https://bugs.webkit.org/"); > + g_test_add_func("/webkit2/webview/default_context", > + testWebViewDefaultContext); > + I'm loving these unit tests! The asterisks are on the wrong side here though. (In reply to comment #4) > (From update of attachment 107986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107986&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:153 > > +/* Private API */ > > it should be apparent because it's in WebKit coding style. It means the API is public inside webkit, but private for the users. It's still obvious because they are not static methods, but I added just to organize the source file, I can remove it anyway. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:154 > > +WKContextRef webkitWebContextGetContext(WebKitWebContext* context) > > Probably better to call it something like: webkitWebContextGetWKContextRef to eliminate ambiguity. Makes sense. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:49 > > + webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetContext(webView->priv->context), 0); > > Might as well do: > webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), > webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView->priv->context)), 0); I'm not sure I understand what you mean here, you are using a WebView cast for a WebContext. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:113 > > + * Creates a new #WebKitWebView with the default context. > > + * See also webkit_web_view_new_with_context. > > + * > > Somewhere we need to explain what a web context represents. Yes, that would be part of the section for WebKitWebContext > I think it's sufficient to link to #WebKitWebContext here and ensure the documentation there is good. > > i.e. Creates a new #WebKitWebView with the default #WebKitWebContext. Ok. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:127 > > + * @context: the #WebKitWebContext to be used by the #WebKitWebView > > + * > > + * Creates a new #WebKitWebView with the given context. > > + * > > + * Returns: The newly created #WebKitWebView widget > > Ditto. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:140 > > + * Gets the web context of @web_view. > > Ditto. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-63 > > }; > > > > WK_EXPORT GType > > -webkit_web_view_get_type (void); > > This is why I think we should stop lining up the argument lists here. It's standard in GLib headers, but this kind of churn hurts patch readability and makes "git blame" useless. I think source code readability is more important than patches readability, and git blame in a C header file is not that useful, but if everybody else agrees it's fine with me to stop lining up the arguments. > > Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:40 > > + WebKitWebView *view = WEBKIT_WEB_VIEW(webkit_web_view_new()); > > + g_object_ref_sink(view); > > + g_assert(webkit_web_view_get_context(view) == webkit_web_context_get_default()); > > + g_object_unref(view); > > +} > > + > > +int main(int argc, char **argv) > > +{ > > + g_thread_init(NULL); > > + gtk_test_init(&argc, &argv, NULL); > > + > > + g_test_bug_base("https://bugs.webkit.org/"); > > + g_test_add_func("/webkit2/webview/default_context", > > + testWebViewDefaultContext); > > + > > I'm loving these unit tests! The asterisks are on the wrong side here though. Unit tests are C files, not C++. Created attachment 108115 [details]
Updated patch
Updated patch fixing some of the comments, and it also adds WebKitPrivate.h with GParamFlags macros, since this patch is the first one adding properties.
(In reply to comment #5) > > Might as well do: > > webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), > > webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView->priv->context)), 0); > > I'm not sure I understand what you mean here, you are using a WebView cast for a WebContext. Sorry. I mean this: webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), > > webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView)->priv->context), > > I'm loving these unit tests! The asterisks are on the wrong side here though. > Unit tests are C files, not C++. Sorry. I always forget about the silly difference in style for C files. This patch looks good to me. I think it should be r+ with another reviewer looking at it and a few minor tweaks. Comment on attachment 108115 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=108115&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h:35 > +#define WEBKIT_PARAM_READABLE (static_cast<GParamFlags>(G_PARAM_READABLE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB)) > +#define WEBKIT_PARAM_WRITABLE (static_cast<GParamFlags>(G_PARAM_WRITABLE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB)) > +#define WEBKIT_PARAM_READWRITE (static_cast<GParamFlags>(G_PARAM_READWRITE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB)) You have some extra spaces after the macro names. I don't think we typically line these up in WebKit. Comment on attachment 108115 [details]
Updated patch
Looks good to me, API wise!
Comment on attachment 108115 [details]
Updated patch
Philippe has approved this so r+ with the few tweaks I suggested.
(In reply to comment #10) > (From update of attachment 108115 [details]) > Philippe has approved this so r+ with the few tweaks I suggested. Thanks, not that this still depends on bug https://bugs.webkit.org/show_bug.cgi?id=67931, which depends on headers patch. Committed r96136: <http://trac.webkit.org/changeset/96136> |