[Webkit-unassigned] [Bug 67990] [GTK] Use WebKitWebContext in WebKitWebView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67990





--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-09-20 23:41:38 PST ---
(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++.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.