[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 #92992|review?                     |review-
               Flag|                            |




--- Comment #22 from Martin Robinson <mrobinson at webkit.org>  2011-05-10 12:28:41 PST ---
(From update of attachment 92992)
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?

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