[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


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