[Webkit-unassigned] [Bug 94316] [GTK] Add webkit_uri_scheme_request_finish_error

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=94316


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #196839|review?                     |review-
               Flag|                            |




--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-04-08 05:44:44 PST ---
(From update of attachment 196839)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.