Bug 94316

Summary:

[GTK] Add webkit_uri_scheme_request_finish_error

Product:

WebKit

Reporter:

Jesse van den Kieboom <jessevdk>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, benjamin, cgarcia, commit-queue, danw, gustavo, gyuyoung.kim, mrobinson, rakuco, rego, spena, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

114347

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Jesse van den Kieboom

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Manuel Rego Casasnovas

Comment 1 2013-04-05 07:01:12 PDT

Created attachment 196628 [details] Patch

WebKit Review Bot

Comment 2 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

Carlos Garcia Campos

Comment 3 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

Manuel Rego Casasnovas

Comment 4 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.

Carlos Garcia Campos

Comment 5 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

Martin Robinson

Comment 6 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.

Manuel Rego Casasnovas

Comment 7 2013-04-08 15:31:52 PDT

Created attachment 196962 [details] Patch

Carlos Garcia Campos

Comment 8 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.

Manuel Rego Casasnovas

Comment 9 2013-04-10 02:52:27 PDT

Created attachment 197231 [details] Patch

Martin Robinson

Comment 10 2013-05-06 09:42:47 PDT

API looks good to me. This just needs owner review.

Manuel Rego Casasnovas

Comment 11 2013-05-06 12:11:31 PDT

Two GTK+ reviewers have approved the new API. Adding owners to CC.

WebKit Commit Bot

Comment 12 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>

WebKit Commit Bot

Comment 13 2013-05-06 14:19:56 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.