Bug 94316 - [GTK] Add webkit_uri_scheme_request_finish_error
Summary: [GTK] Add webkit_uri_scheme_request_finish_error
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 114347
  Show dependency treegraph
 
Reported: 2012-08-17 02:03 PDT by Jesse van den Kieboom
Modified: 2013-05-06 14:19 PDT (History)
12 users (show)

See Also:


Attachments
Patch (19.40 KB, patch)
2013-04-05 07:01 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (22.96 KB, patch)
2013-04-08 01:27 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (25.80 KB, patch)
2013-04-08 15:31 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (28.28 KB, patch)
2013-04-10 02:52 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse van den Kieboom 2012-08-17 02:03:55 PDT
It would be nice to have an additional method on WebKitURISchemeRequest to signal errors in requesting a resource. For example, it would be nice to be able to signal a 404, or something similar.
Comment 1 Manuel Rego Casasnovas 2013-04-05 07:01:12 PDT
Created attachment 196628 [details]
Patch
Comment 2 WebKit Review Bot 2013-04-05 07:03:13 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Carlos Garcia Campos 2013-04-05 09:37:31 PDT
Comment on attachment 196628 [details]
Patch

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
Comment 4 Manuel Rego Casasnovas 2013-04-08 01:27:34 PDT
Created attachment 196839 [details]
Patch

While adding the check for the error domain I realized that the error always have G_IO_ERROR domain, this was because of the implementation of ResourceError::httpError(). I've did a change to keep the domain of the error, but maybe that's not right.
Comment 5 Carlos Garcia Campos 2013-04-08 05:44:44 PDT
Comment on attachment 196839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196839&action=review

Looks good, but there are still some issues to fix.

> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:64
> -        return genericIOError(error, request);
> +        return ResourceError(g_quark_to_string(error->domain), error->code, failingURI(request),
> +            String::fromUTF8(error->message));

This leaves generioIOError unused, I think. We could rename it to genericGError, for example, and use the GError + failingURI to build a ResourceError. Martin, what do you think?

> Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:224
> + * Finish a #WebKitURISchemeRequest with a #GError that you could monitor using
> + * the #WebKitWebView::load-failed signal.

This is a bit confusing, sounds like you can monitor the GError or something like that. There's nothing special about this that requires to be mentioned, it's like any other resource, if it fails to load, the normal signals will be emitted. I think it's better to not say anything about the load-failed signal.

> Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:225
> + */

Add Since: 2.2 tag here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:599
> + *         GError *error = g_error_new (g_quark_from_string("example-domain"), 10, "Invalid about:%s page.", path);
> + *         webkit_uri_scheme_request_finish_error (request, error);
> + *         g_error_free (error);
> + *         return;

GError *error;

error = g_error_new (g_quark_from_string("example-domain"), 10, "Invalid about:%s page.", path);
webkit_uri_scheme_request_finish_error (request, error);
....

Code examples are in C, so let's make this looks a bit more like C.

Also the GError it's not usually built that way, a predefined quark + error code is used, so you could do something like:

error = g_error_new (ABOUT_HANDLER_ERROR, ABOUT_HANDLER_ERROR_INVALID, "Invalid about:%s page.", path);

> Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp:69
> +    if (test->m_error)
> +        g_error_free(test->m_error);
> +    test->m_error = g_error_copy(error);

Use GOwnPtr<GError> and also clear it always before starting a new load, otherwise a successful load after a failed one will have an invalid error set.

> Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp:109
> +    if (m_error)
> +        g_error_free(m_error);

You don't need this if you use a GOwnPtr

> Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.h:62
> +    GError* m_error;

GOwnPtr<GError> m_error;

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:159
> +        if (scheme == "echo") {

use g_strcmp0 to compare strings

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:163
> +            GError* error = g_error_new_literal(g_quark_from_string(errorDomain), errorCode, errorMessage);

Use GOwnPtr here too

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:168
> +        } else
> +            if (!handler.reply.isNull())

else if (!handler.reply.isNull()). This elase after a return looks weird, why don't you move this check before the error one? The error request should always be done without any reply, since we know it's going to fail.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:216
> +    test->registerURISchemeHandler("error", kBarHTML, strlen(kBarHTML), "text/html");

Don't pass a reply since we are going to fail.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:225
> +    g_assert(test->m_error);
> +    g_assert_cmpstr(g_quark_to_string(test->m_error->domain), ==, errorDomain);
> +    g_assert_cmpint(test->m_error->code, ==, errorCode);
> +    g_assert_cmpstr(test->m_error->message, ==, errorMessage);

Use g_assert_error() instead
Comment 6 Martin Robinson 2013-04-08 08:48:25 PDT
Comment on attachment 196839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196839&action=review

>> Source/WebCore/platform/network/soup/ResourceErrorSoup.cpp:64
>> +            String::fromUTF8(error->message));
> 
> This leaves generioIOError unused, I think. We could rename it to genericGError, for example, and use the GError + failingURI to build a ResourceError. Martin, what do you think?

Sounds good to me.
Comment 7 Manuel Rego Casasnovas 2013-04-08 15:31:52 PDT
Created attachment 196962 [details]
Patch
Comment 8 Carlos Garcia Campos 2013-04-10 01:48:06 PDT
Comment on attachment 196962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196962&action=review

Looks good to me. This is new API so you need another review from the API point of view, and also an owner should approve this.

> Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp:68
> +    test->m_error.clear();
> +    test->m_error.set(g_error_copy(error));

Don't need to clear before set.

> Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp:159
> +    m_error.clear();

This should be done in all other load methods.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:158
> +        if (equalIgnoringCase(scheme, "error")) {

Where does this came from? Why not using g_str_equal or g_strcmp0?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:161
> +            error.clear();

This is done automatically.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:225
> +    g_assert_cmpstr(test->m_error.get()->message, ==, errorMessage);

I think you can do test->m_error->message directly.

> Tools/MiniBrowser/gtk/main.c:223
> +        GError *error = g_error_new(g_quark_from_string("minibrowser"), 10, "Invalid about:%s page.", path);

This 10 looks arbitrary. You could define a global quark and error code.
Comment 9 Manuel Rego Casasnovas 2013-04-10 02:52:27 PDT
Created attachment 197231 [details]
Patch
Comment 10 Martin Robinson 2013-05-06 09:42:47 PDT
API looks good to me. This just needs owner review.
Comment 11 Manuel Rego Casasnovas 2013-05-06 12:11:31 PDT
Two GTK+ reviewers have approved the new API. Adding owners to CC.
Comment 12 WebKit Commit Bot 2013-05-06 14:19:53 PDT
Comment on attachment 197231 [details]
Patch

Clearing flags on attachment: 197231

Committed r149642: <http://trac.webkit.org/changeset/149642>
Comment 13 WebKit Commit Bot 2013-05-06 14:19:56 PDT
All reviewed patches have been landed.  Closing bug.