[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