[Webkit-unassigned] [Bug 49543] [GTK] Improve FrameLoader signals. Resource loading
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 10 12:28:41 PDT 2011
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.
More information about the webkit-unassigned
mailing list