[webkit-reviews] review denied: [Bug 94316] [GTK] Add webkit_uri_scheme_request_finish_error : [Attachment 196628] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 5 09:39:16 PDT 2013
Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 94316: [GTK] Add webkit_uri_scheme_request_finish_error
https://bugs.webkit.org/show_bug.cgi?id=94316
Attachment 196628: Patch
https://bugs.webkit.org/attachment.cgi?id=196628&action=review
------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=196628&action=review
Looks good in general, but there are some memory leaks and other minor issues.
Thanks!
> Source/WebKit2/ChangeLog:7
> + Need a short description (OOPS!).
> + Need the bug URL (OOPS!).
Remove this, as you already provided those.
> Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:233
> + WebCore::ResourceError resourceError =
WebCore::ResourceError(g_quark_to_string(error->domain), error->code,
priv->uri.data(), String::fromUTF8(error->message));
This can be WebCore::ResourceError
resourceError(g_quark_to_string(error->domain), error->code, priv->uri.data(),
String::fromUTF8(error->message));
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:571
> + * #WebKitURISchemeRequest and calling webkit_uri_scheme_request_finish() or
> + * webkit_uri_scheme_request_finish_error() later when the data of the
request
> + * is available.
webkit_uri_scheme_request_finish() later when the data of the request is
available or webkit_uri_scheme_request_finish_error() in case of error.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:597
> + * g_error_new
(WEBKIT_NETWORK_ERROR,
Don't leak the GError in the example, people will copy-paste and everybody will
end up leaking. Use a different doamin, one invented to avoid confusion thjat
you have to use NETWORK.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:598
> + * 0,
Use an error code to, also invented, but something different that 0.
> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:599
> + *
"<html><body><p>Invalid about:%s
page</p></body></html>",
Don't use html in GError message.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:125
> + Standard,
> + WithPath,
> + Error
You can check the actual scheme instead of having to pass this.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:176
> + webkit_uri_scheme_request_finish_error(request,
g_error_new(WEBKIT_NETWORK_ERROR, 0, "Error"));
Don't leak the GError. If you use the network doain you should use a valid
error code, and 0 isn't an error code valid for the domain. You ca use
g_quark_from_string to create one just for testing.
> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:228
> + test->waitUntilLoadFinished();
> +
g_assert(test->m_loadEvents.contains(LoadTrackingTest::ProvisionalLoadFailed));
Your should also check the actual GError received to make sure domain, code,
and message match.
> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:173
> + GError* gerror =
g_error_new_literal(g_quark_from_string(error.domain().utf8().data()),
error.errorCode(), error.localizedDescription().utf8().data());
gerror -> error
> Source/WebKit2/WebProcess/soup/WebSoupRequestManager.cpp:174
> + g_simple_async_result_take_error(result.get(), gerror);
You coudl even pass the error directly without caching it in a local variable.
> Tools/MiniBrowser/gtk/main.c:222
> + webkit_uri_scheme_request_finish_error(request,
g_error_new(WEBKIT_NETWORK_ERROR, 0, "<html><body><p>Invalid about:%s
page.</p></body></html>", path));
The error is leaked here too, same comments about the domain +error code. Don't
use html in the message, the default handler for error pages already includes
the html tags
More information about the webkit-reviews
mailing list