[Webkit-unassigned] [Bug 49543] [GTK] Improve FrameLoader signals. Resource loading

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49543


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #92777|review?                     |review-
               Flag|                            |




--- Comment #19 from Martin Robinson <mrobinson at webkit.org>  2011-05-09 09:55:09 PST ---
(From update of attachment 92777)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.