[webkit-reviews] review denied: [Bug 94316] [GTK] Add webkit_uri_scheme_request_finish_error : [Attachment 196839] Patch

[bugzilla-daemon at webkit.org]

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 196839: Patch
https://bugs.webkit.org/attachment.cgi?id=196839&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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