[webkit-reviews] review granted: [Bug 70594] [GTK] Add helper function to set the loader client in WebKitWebView : [Attachment 111934] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 70594: [GTK] Add helper function to set the loader client in WebKitWebView
https://bugs.webkit.org/show_bug.cgi?id=70594

Attachment 111934: Patch
https://bugs.webkit.org/attachment.cgi?id=111934&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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-instanti
ation
> 
> 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?