[Webkit-unassigned] [Bug 94316] [GTK] Add webkit_uri_scheme_request_finish_error
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 5 09:39:20 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=94316
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #196628|review? |review-
Flag| |
--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-04-05 09:37:31 PST ---
(From update of attachment 196628)
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
--
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