Bug 67990 - [GTK] Use WebKitWebContext in WebKitWebView
Summary: [GTK] Use WebKitWebContext in WebKitWebView
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: 67931
Blocks: 68085 68371
  Show dependency treegraph
 
Reported: 2011-09-13 01:18 PDT by Carlos Garcia Campos
Modified: 2011-09-27 11:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.77 KB, patch)
2011-09-13 01:23 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (13.33 KB, patch)
2011-09-20 05:26 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch (15.62 KB, patch)
2011-09-21 00:46 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-09-13 01:18:20 PDT
Instead of using WKContextGetSharedProcessContext(), it should use webkit_web_context_get_default() and add API to create a new view with a context and return the current context.
Comment 1 Carlos Garcia Campos 2011-09-13 01:23:54 PDT
Created attachment 107151 [details]
Patch
Comment 2 Nayan Kumar K 2011-09-19 05:45:35 PDT
Seems like you missed adding WebKitWebContextPrivate.h?
Comment 3 Carlos Garcia Campos 2011-09-20 05:26:48 PDT
Created attachment 107986 [details]
Updated patch

Add missing WebKitWebContextPrivate.h as pointed out by Nayan.
Comment 4 Martin Robinson 2011-09-20 15:24:37 PDT
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.
Comment 5 Carlos Garcia Campos 2011-09-20 23:41:38 PDT
(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++.
Comment 6 Carlos Garcia Campos 2011-09-21 00:46:51 PDT
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.
Comment 7 Martin Robinson 2011-09-21 09:01:36 PDT
(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 8 Martin Robinson 2011-09-21 09:02:19 PDT
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 9 Philippe Normand 2011-09-21 09:07:34 PDT
Comment on attachment 108115 [details]
Updated patch

Looks good to me, API wise!
Comment 10 Martin Robinson 2011-09-21 09:08:31 PDT
Comment on attachment 108115 [details]
Updated patch

Philippe has approved this so r+ with the few tweaks I suggested.
Comment 11 Carlos Garcia Campos 2011-09-21 09:13:54 PDT
(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.
Comment 12 Carlos Garcia Campos 2011-09-27 11:32:08 PDT
Committed r96136: <http://trac.webkit.org/changeset/96136>