Bug 70594 – [GTK] Add helper function to set the loader client in WebKitWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

It's set in two places, the constructed method and the public method set_loader_client(). In the constructor we don't need to check that view is actually a WebView and loader client is a LoaderClient, and that current client is not the same.

[Carlos Garcia Campos]

Created attachment 111934 [details]
Patch

[Martin Robinson]

Comment on attachment 111934 [details]
Patch

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

> Source/WebKit2/ChangeLog:5
> +        [GTK] Add helper function to set the loader client in WebKitWebView
> +        https://bugs.webkit.org/show_bug.cgi?id=70594
> +

These checks are incredibly cheap. Is there some other problem?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:68
> -
> -    webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
> +    WebPageProxy* page = webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
>  

It seems like instead of doing this manually we should be chaining up to the parent constructor. It's mentioned here that you should always chain up to the parent constructor: http://developer.gnome.org/gobject/stable/chapter-gobject.html#gobject-instantiation

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 111934 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111934&action=review
> 
> > Source/WebKit2/ChangeLog:5
> > +        [GTK] Add helper function to set the loader client in WebKitWebView
> > +        https://bugs.webkit.org/show_bug.cgi?id=70594
> > +
> 
> These checks are incredibly cheap. Is there some other problem?

g_return macros are not so cheap.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:68
> > -
> > -    webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
> > +    WebPageProxy* page = webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetWKContext(priv->context), 0);
> >  
> 
> It seems like instead of doing this manually we should be chaining up to the parent constructor. It's mentioned here that you should always chain up to the parent constructor: http://developer.gnome.org/gobject/stable/chapter-gobject.html#gobject-instantiation

This method is constructed, not constructor.

[Martin Robinson]

Comment on attachment 111934 [details]
Patch

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

Okay. Looks fine, but I have a minor change requested below. Thank for cleaning this up.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:60
> +    webView->priv->loaderClient = loaderClient;
> +    webkitWebLoaderClientAttachLoaderClientToPage(loaderClient, wkPage);

Instead of passing the page as an argument, I think it'd be better to get it here with:

WebPageProxy* page = webkitWebViewBaseGetPage(WEBKIT_WEB_VIEW_BASE(webView));

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:68
>>>  
>> 
>> It seems like instead of doing this manually we should be chaining up to the parent constructor. It's mentioned here that you should always chain up to the parent constructor: http://developer.gnome.org/gobject/stable/chapter-gobject.html#gobject-instantiation
> 
> This method is constructed, not constructor.

Still the docs suggest chaining up: "the constructed function is called by g_object_new() as the final step of the object creation process. At the point of the call, all construction properties have been set on the object. The purpose of this call is to allow for object initialisation steps that can only be performed after construction properties have been set. constructed implementors should chain up to the constructed call of their parent class to allow it to complete its initialisation."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:439
> -void webkitWebViewBaseCreateWebPage(WebKitWebViewBase* webkitWebViewBase, WKContextRef context, WKPageGroupRef pageGroup)
> +WebPageProxy* webkitWebViewBaseCreateWebPage(WebKitWebViewBase* webkitWebViewBase, WKContextRef context, WKPageGroupRef pageGroup)
>  {
>      WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;
>  

I dislike this change because we ignore the return value in WebKitWebViewBase. Do you mind just getting the Page in WebKitWebViewSetLoader client?

[Carlos Garcia Campos]

Committed r98241: <http://trac.webkit.org/changeset/98241>