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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 9 09:55:08 PDT 2011


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 92777: proposed patch
https://bugs.webkit.org/attachment.cgi?id=92777&action=review

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

Awesome! This looks good to me. Please see my comments below, which comprise
mostly stylistic nits. We need to get tha approval of another GTK+ reviewer for
this.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:-389
> -    if (!redirectResponse.isNull()) {
> -	   // 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());
> -    }
> -    

In this ChangeLog you should describe why you removed this bit in the
FrameLoaderClient::dispatchWillSendRequest method line.

> Source/WebKit/gtk/webkit/webkitnetworkrequest.cpp:174
> +							   NULL,

NULL -> 0 here.

> Source/WebKit/gtk/webkit/webkitnetworkrequest.cpp:306
> +    WebKitNetworkRequestPrivate* priv = request->priv;
> +
> +    return priv->mainURI;

Please just say request->priv->mainURI here. :)

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:332
> +	* the lifetime of the resource, but the contents may change from
> +	* inbetween signal emissions.

"from inbetween" should just be "between" Awesome doc!

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:340
> +	       NULL, NULL,

NULL -> 0 here.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:353
> +	* Emitted when the first byte of data arrives

Period needed here.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:361
> +	       NULL, NULL,

NULL -> 0.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:372
> +	* Emitted when all the data for the resource was loaded

Period necessary.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:380
> +	       NULL, NULL,

NULL -> 0.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:391
> +	* Emitted when all the data for the resource was loaded

Period.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:399
> +	       NULL, NULL,

NULL -> 0.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:411
> +	* Invoked when a resource failed to load

Period.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:419
> +	       NULL, NULL,

NULL -> 0.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1058
> +static char* urlPath(SoupURI* uri)

Please call this convertSoupURIToURLPath or something equally descriptive, to
meet the style guidelines.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1068
> +    gchar* basename = g_path_get_basename(uri->path);
> +    gchar* dirname = g_path_get_basename(g_path_get_dirname(uri->path));
> +    return g_strdup_printf("%s/%s", dirname, basename);

Please just make this one line. Might want to investigate whether it is
possible to use CString here without leaking memory. That would avoid needing
to use GOwnPtr. If not, no worries.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1071
> +static char* urlPath(SoupMessage* soupMessage)

Please make the method name more descriptive, per the style guidelines.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1080
> +    if (soupMessage) {
> +	   SoupURI* requestURI = soup_message_get_uri(soupMessage);
> +	   if (requestURI)
> +	       path = urlPath(requestURI);
> +    }
> +    return path;

Please use early returns here with something like this:

if (!soupMessage)
    return 0;
if (SoupURI* requestURI = soup_message_get_uri(soupMessage))
    return urlPath(requestURI);
return 0;

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1083
> +static char* urlPath(WebKitNetworkRequest* request)

Please make the helper name more descriptive.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1086
> +    SoupMessage* soupMessage = webkit_network_request_get_message(request);
> +    return urlPath(soupMessage);

Please make this one line.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1092
> +    SoupURI* uri = soup_uri_new(webkit_web_resource_get_uri(webResource));
> +    return urlPath(uri);

Ditto.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1171
> +    if (!g_ascii_strncasecmp(errorDomain, "webkit-network-error-quark", 26))


Wouldn't it be better to use g_str_equal here or is there a chance that
error->domain may not be null escaped or have weird case?

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1210
> +    char* description;

You can eliminate this variable entirely.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1218
> +    description = g_strdup_printf("<NSURLResponse %s, http status code %d>",
path.get(), statusCode);
> +    return description;

See above.


More information about the webkit-reviews mailing list