[webkit-reviews] review denied: [Bug 49543] [GTK] Improve FrameLoader signals. Resource loading : [Attachment 92992] updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 49543: [GTK] Improve FrameLoader signals. Resource loading
https://bugs.webkit.org/show_bug.cgi?id=49543

Attachment 92992: updated patch
https://bugs.webkit.org/attachment.cgi?id=92992&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92992&action=review

Thank you very much for updating the patch! I still have a few concerns though.
See below.

> LayoutTests/platform/gtk/Skipped:907
>  fast/loader/recursive-before-unload-crash.html 

What's missing for this one still?

> Source/WebKit/gtk/ChangeLog:23
> +	   Based on patch by Sergio Villar Senin <svillar at igalia.com>

Please just put this in the top line:

2011-05-09  Philippe Normand  <pnormand at igalia.com> and Sergio Villar Senin 
<svillar at igalia.com>

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:389
> -	   // This is a redirect, so we need to update the WebResource's
knowledge
> -	   // of the URI.
> -	   g_free(webResource->priv->uri);
> -	   webResource->priv->uri =
g_strdup(request.url().string().utf8().data());
> -    }
> -    
> +	    // This is a redirect, so we need to update the WebResource's
knowledge
> +	    // of the URI.
> +	    g_free(webResource->priv->uri);
> +	    webResource->priv->uri =
g_strdup(request.url().string().utf8().data());
> +	}
> +

This change was probably accidental?

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1178
> +    // Notify listeners

You can just omit this comment, in my opinion. :)

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1195
> +    // Notify listeners

Ditto.

> Source/WebKit/gtk/webkit/webkitnetworkrequest.cpp:169
> +    /**
> +	* WebKitNetworkRequest:main-uri:
> +	*
> +	* The #ResourceRequest that backs the request.
> +	*
> +	* Since: 1.5.1
> +	*/

I'm not sure I understand the difference between the URI and MainURI.  The
comment above doesn't seem to explain it either. Why is this property
necessary? I don't see a similar one on the Mac port's API surface.

> Source/WebKit/gtk/webkit/webkitnetworkrequest.cpp:300
> +const gchar*
> +webkit_network_request_get_main_uri(WebKitNetworkRequest* request)

Should just be one line here.

> Source/WebKit/gtk/webkit/webkitwebresource.cpp:138
> +	       NULL, NULL,

Should turn these NULLs into 0.

> Source/WebKit/gtk/webkit/webkitwebresource.cpp:156
> +	       NULL, NULL,

Ditto.

> Source/WebKit/gtk/webkit/webkitwebresource.cpp:173
> +	       NULL, NULL,

Ditto.

> Source/WebKit/gtk/webkit/webkitwebresource.cpp:190
> +	       NULL, NULL,

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2766
> +	       NULL, NULL,

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2787
> +	       NULL, NULL,

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2808
> +	       NULL, NULL,

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2830
> +	       NULL, NULL,

Ditto.

> Tools/ChangeLog:10
> +	   Based on patch by Sergio Villar Senin <svillar at igalia.com>

Please move this one up as well.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:70
> +extern WebKitWebResource* webkit_web_view_get_main_resource(WebKitWebView*);


It's unfortuante that we are still using private APIs here. :( Hopefully a
later patch can push whatever is needed into the public API. See below where
I've suggested an alternative approach though.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1062
> +    GOwnPtr<gchar> urlPath(g_strdup_printf("%s/%s",
g_path_get_basename(g_path_get_dirname(uri->path)),
g_path_get_basename(uri->path)));

This is a memory leak because g_path_get_basename returns a newly allocated
string.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1082
> +    return
convertSoupURIToURLPath(soup_uri_new(webkit_web_resource_get_uri(webResource)))
;

Doesn't this leak the SoupURI?

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1090
> +    return CString(g_path_get_basename(uriString));

This is a memory leak, because the g_path_get_basename returns a newly
allocated string.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1107
> +    if (webResource == webkit_web_view_get_main_resource(webView)

Couldn't you use the public API here by doing: webkit_web_frame_get_data_source
 and then webkit_web_data_source_get_main_resource?