Bug 84133

Summary:

[GTK] Add API to register custom URI schemes to WebKit2 GTK+ API

Product:

WebKit

Reporter:

Carlos Garcia Campos <cgarcia>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

danw, gustavo, mrobinson, philn, svillar, webkit.review.bot, xan.lopez

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

84130, 85880, 88087

Bug Blocks:

Attachments:

Description	Flags
Patch	mrobinson: review-, gustavo: commit-queue-
Updated patch	none
Updated patch	none
Updated patch	none
New patch	mrobinson: review+

Carlos Garcia Campos

Reported 2012-04-17 01:57:58 PDT

Allow users to register custom URI schemes, like about: pages.

Attachments
Patch (34.54 KB, patch) 2012-04-17 02:14 PDT, Carlos Garcia Campos	mrobinson: review- gustavo: commit-queue-	Details Formatted Diff Diff
Updated patch (36.95 KB, patch) 2012-05-07 02:34 PDT, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
Updated patch (36.93 KB, patch) 2012-05-08 06:53 PDT, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
Updated patch (37.19 KB, patch) 2012-05-10 04:24 PDT, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
New patch (43.43 KB, patch) 2012-06-01 07:01 PDT, Carlos Garcia Campos	mrobinson: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2012-04-17 02:14:30 PDT

Created attachment 137495 [details] Patch

WebKit Review Bot

Comment 2 2012-04-17 02:18:10 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

Gustavo Noronha (kov)

Comment 3 2012-04-17 02:27:12 PDT

Comment on attachment 137495 [details] Patch Attachment 137495 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12417539

Carlos Garcia Campos

Comment 4 2012-04-17 02:29:39 PDT

(In reply to comment #3) > (From update of attachment 137495 [details]) > Attachment 137495 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/12417539 It fails to build because it depends on bug #84130

Martin Robinson

Comment 5 2012-04-26 08:48:54 PDT

Comment on attachment 137495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137495&action=review Looks good! The only large change I think you should make is to nix webkit_uri_scheme_request_get_path and webkit_uri_scheme_request_get_scheme, as their functionality is really duplicated by Soup. In my mind, we should either be passing a URI object in our API or all agree that it's fine to use Soup to parse URIs manually. Adding these helper methods to one class makes me ask "Why aren't these everywhere that we return URIs?" If we added them everywhere we might as well pass SoupURI. So you see my issue... > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:83 > +/** > + * webkit_uri_scheme_request_get_scheme: > + * @request: a #WebKitURISchemeRequest > + * > + * Get the URI scheme of @request > + * > + * Returns: the URI scheme of @request > + */ > +const char* webkit_uri_scheme_request_get_scheme(WebKitURISchemeRequest* request) > +{ > + g_return_val_if_fail(WEBKIT_IS_URI_SCHEME_REQUEST(request), 0); > + > + if (!request->priv->soupURI) > + request->priv->soupURI.set(soup_uri_new(request->priv->uri.data())); > + return request->priv->soupURI->scheme; > +} It seems a little weird that this class should be responsible for parsing URI's when other bits of the Gnome platform can do it. Perhaps you should omit this method and webkit_uri_scheme_request_get_path and simply rely on Soup to do it in callers. > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:127 > +/** > + * webkit_uri_scheme_request_finish: > + * @request: a #WebKitURISchemeRequest > + * @data: the contents of the request > + * @data_length: the length of the data, may be -1 if @data is a %NULL-terminated string > + * @mime_type: the content type of the data > + * > + * Finish a #WebKitURISchemeRequest by setting the contents of the request and its mime type. > + */ > +void webkit_uri_scheme_request_finish(WebKitURISchemeRequest* request, gconstpointer data, gssize dataLength, const gchar* mimeType) > +{ It would be really nice if this API would support passing data in chunks. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:268 > + * Register @scheme in @context, so that when an URI request with @scheme is made in the Let me prefix this comment by stating outright that I think English is a dumb language for a variety of reasons. Notwithstanding, in cases where the 'U' is pronounced like 'you' it's actually better to use 'a' instead of 'an.' So your sentence would now include "a URI" instead of "an URI." > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:284 > + * path = webkit_uri_scheme_request_get_path (request); As stated above, I think it makes sense for Soup to parse the URI directly here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:292 > + * contents = g_strdup_printf ("<html><body><p>Invalid about:%s page</p></body></html>", path); Oh, gtkdoc! > Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h:-32 > -G_BEGIN_DECLS Thanks for cleaning these up as you go along! > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:33 > +<SUBSECTION URI Scheme> Extreme nit: I think you have an extra space after SUBSECTION. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:36 > +static const char* kBarHtml = "<html><body>Bar</body></html>"; > +static const char* kEchoHtmlFormat = "<html><body>%s</body></html>"; It probably makes sense to make these static within the class. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:52 > + URISchemeTest* test = static_cast<URISchemeTest*>(userData); > + test->m_uriSchemeRequest = request; > + test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(request)); > + if (!g_strcmp0(webkit_uri_scheme_request_get_scheme(request), "foo")) > + webkit_uri_scheme_request_finish(request, reinterpret_cast<gconstpointer>(kBarHtml), -1, "text/html"); > + else if (!g_strcmp0(webkit_uri_scheme_request_get_scheme(request), "echo")) { > + GOwnPtr<char> echoHtml(g_strdup_printf(kEchoHtmlFormat, webkit_uri_scheme_request_get_path(request))); > + webkit_uri_scheme_request_finish(request, reinterpret_cast<gconstpointer>(echoHtml.get()), -1, "text/html"); > + } As stated above, I think it makes sense to use Soup to parse the URI directly here.

Carlos Garcia Campos

Comment 6 2012-04-26 09:11:08 PDT

(In reply to comment #5) > (From update of attachment 137495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137495&action=review > > Looks good! The only large change I think you should make is to nix webkit_uri_scheme_request_get_path and webkit_uri_scheme_request_get_scheme, as their functionality is really duplicated by Soup. Thanks for reviewing! > In my mind, we should either be passing a URI object in our API or all agree that it's fine to use Soup to parse URIs manually. Adding these helper methods to one class makes me ask "Why aren't these everywhere that we return URIs?" If we added them everywhere we might as well pass SoupURI. So you see my issue... In this particular case, it's pretty common to get the path and the scheme if you register more than one scheme with the same callback. > > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:83 > > +/** > > + * webkit_uri_scheme_request_get_scheme: > > + * @request: a #WebKitURISchemeRequest > > + * > > + * Get the URI scheme of @request > > + * > > + * Returns: the URI scheme of @request > > + */ > > +const char* webkit_uri_scheme_request_get_scheme(WebKitURISchemeRequest* request) > > +{ > > + g_return_val_if_fail(WEBKIT_IS_URI_SCHEME_REQUEST(request), 0); > > + > > + if (!request->priv->soupURI) > > + request->priv->soupURI.set(soup_uri_new(request->priv->uri.data())); > > + return request->priv->soupURI->scheme; > > +} > > It seems a little weird that this class should be responsible for parsing URI's when other bits of the Gnome platform can do it. Perhaps you should omit this method and webkit_uri_scheme_request_get_path and simply rely on Soup to do it in callers. This is just a convenient method. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:268 > > + * Register @scheme in @context, so that when an URI request with @scheme is made in the > > Let me prefix this comment by stating outright that I think English is a dumb language for a variety of reasons. Notwithstanding, in cases where the 'U' is pronounced like 'you' it's actually better to use 'a' instead of 'an.' So your sentence would now include "a URI" instead of "an URI." Ok. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:284 > > + * path = webkit_uri_scheme_request_get_path (request); > > As stated above, I think it makes sense for Soup to parse the URI directly here. I think this makes the api a lot simpler, because the user doesn't need to create a soup uri, get the path and free the uri. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:292 > > + * contents = g_strdup_printf ("<html><body><p>Invalid about:%s page</p></body></html>", path); > > Oh, gtkdoc! it looks nice in the generated HTML :-) > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContextPrivate.h:-32 > > -G_BEGIN_DECLS > > Thanks for cleaning these up as you go along! np > > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:33 > > +<SUBSECTION URI Scheme> > > Extreme nit: I think you have an extra space after SUBSECTION. Ah, ok, will remove it. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:36 > > +static const char* kBarHtml = "<html><body>Bar</body></html>"; > > +static const char* kEchoHtmlFormat = "<html><body>%s</body></html>"; > > It probably makes sense to make these static within the class. I tried it, but I don't know how to do that in C++, it failed to compile all the time. > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:52 > > + URISchemeTest* test = static_cast<URISchemeTest*>(userData); > > + test->m_uriSchemeRequest = request; > > + test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(request)); > > + if (!g_strcmp0(webkit_uri_scheme_request_get_scheme(request), "foo")) > > + webkit_uri_scheme_request_finish(request, reinterpret_cast<gconstpointer>(kBarHtml), -1, "text/html"); > > + else if (!g_strcmp0(webkit_uri_scheme_request_get_scheme(request), "echo")) { > > + GOwnPtr<char> echoHtml(g_strdup_printf(kEchoHtmlFormat, webkit_uri_scheme_request_get_path(request))); > > + webkit_uri_scheme_request_finish(request, reinterpret_cast<gconstpointer>(echoHtml.get()), -1, "text/html"); > > + } > > As stated above, I think it makes sense to use Soup to parse the URI directly here. I think it makes the API more convenient to use, it saves some code to the user.

Carlos Garcia Campos

Comment 7 2012-04-26 09:14:48 PDT

Please, note that this depends on #84130 which is still pending review

Martin Robinson

Comment 8 2012-04-26 11:17:51 PDT

Comment on attachment 137495 [details] Patch Carlos and I chatted about this a bit and agreed that it makes more sense for the API to accept a GInputStream, to open up the possibility for doing things asynchronously. For now we'll make the API layer fetch the entire GInputStream contents to make the IPC traffic simpler, but later we'll do things in chunks and implement a custom GInputStream on the Web Process side.

Carlos Garcia Campos

Comment 9 2012-05-07 02:34:28 PDT

Created attachment 140497 [details] Updated patch Patch updated to receive a GInputStream instead of a void* with all the data.

Carlos Garcia Campos

Comment 10 2012-05-08 06:53:07 PDT

Created attachment 140709 [details] Updated patch This patch is now based on bug #85880 that adds support for sending data in chunks to the web process

Carlos Garcia Campos

Comment 11 2012-05-10 04:24:06 PDT

Created attachment 141144 [details] Updated patch Updated to the changes made in bug #85880

Martin Robinson

Comment 12 2012-05-30 13:52:04 PDT

Comment on attachment 141144 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=141144&action=review This looks pretty good to me, though I'm not certain of a few bits of this code. For instance, this does not seem to support empty responses. Perhaps Dan can say how important that is. I'll hold off on my review until then, but I noticed a few other small things... > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:128 > + GRefPtr<WebKitURISchemeRequest> request = adoptGRef(schemeRequest); I think this deserves a small comment explaining that a reference was added during the call to g_input_stream_read_async. > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:131 > + if (bytesRead == -1) > + return; You might want to put a FIXME here about passing the error to the WebProcess side. > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:137 > + WKSoupRequestManagerDidHandleURIRequest(priv->wkRequestManager.get(), wkData.get(), priv->streamLength, wkMimeType.get(), priv->requestID); Here you are passing zero to the WebProcess for streams that do not have predetermined length. How do you differentiate between this situation and empty responses? > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:139 > + } else if (bytesRead || (!bytesRead && !priv->streamLength)) > + WKSoupRequestManagerDidReceiveURIRequestData(priv->wkRequestManager.get(), wkData.get(), priv->requestID); Is it important to call WKSoupRequestManagerDidReceiveURIRequestData when priv->streamLength == 0 (undetermined stream length) case because otherwise the WebProcess won't know that the request is complete? I think this merits a small comment explaining the situation. > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:162 > + g_return_if_fail(streamLength == -1 || streamLength > 0); So it's not possible to send an empty reply? > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:52 > + { > + } > + Nit: I think the curly braces are one indentation level too far to the right. > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:36 > +static const char* kBarHtml = "<html><body>Bar</body></html>"; > +static const char* kEchoHtmlFormat = "<html><body>%s</body></html>"; Nit: Html -> HTML > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:53 > + g_memory_input_stream_add_data(G_MEMORY_INPUT_STREAM(inputStream.get()), reinterpret_cast<gconstpointer>(kBarHtml), inputStreamLength, 0); Do you actually need to castkBarHTML and echoHTML here? I tried compiling this locally with g++ and had no errors: #include <cstdio> void foo(const void* in) { printf("%p\n", in); } int main() { const char* thing = "blahblah"; foo(thing); } > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:113 > + test->loadURI("foo:blank"); > + test->waitUntilLoadFinished(); > + test->checkMainResourceData(); > + > + test->loadURI("echo:hello world"); > + test->waitUntilLoadFinished(); > + test->checkMainResourceData(); It's a bit odd that the fixture handles registering the handler and then the test handles loading the URL. Perhaps structure it like this: test->addURLHandler("foo", kBarHTML); test->waitUntilLoadFinished(); g_assert_cmpstr(test->mainResourceData(), ==, kBarHTML); I think that will make your fixture slightly smaller and more general in this case.

Martin Robinson

Comment 13 2012-05-30 13:52:27 PDT

CCing Dan so he can give some insight about empty replies.

Dan Winship

Comment 14 2012-05-31 03:30:02 PDT

(In reply to comment #13) > CCing Dan so he can give some insight about empty replies. it definitely seems like there might be use cases where you'd want to be able to return a 0-length response. Also, should webkit_web_context_register_uri_scheme() integrate with SchemeRegistry in some way?

Carlos Garcia Campos

Comment 15 2012-05-31 09:57:22 PDT

(In reply to comment #12) > (From update of attachment 141144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141144&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:137 > > + WKSoupRequestManagerDidHandleURIRequest(priv->wkRequestManager.get(), wkData.get(), priv->streamLength, wkMimeType.get(), priv->requestID); > > Here you are passing zero to the WebProcess for streams that do not have predetermined length. How do you differentiate between this situation and empty responses? No, the input stream returns a 0 byte read when it has finished, if there's nothing to read yet, the callback is not emitted. The case of zero-length request is hanlded here indeed. > > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:139 > > + } else if (bytesRead || (!bytesRead && !priv->streamLength)) > > + WKSoupRequestManagerDidReceiveURIRequestData(priv->wkRequestManager.get(), wkData.get(), priv->requestID); > > Is it important to call WKSoupRequestManagerDidReceiveURIRequestData when priv->streamLength == 0 (undetermined stream length) case because otherwise the WebProcess won't know that the request is complete? I think this merits a small comment explaining the situation. WKSoupRequestManagerDidHandleURIRequest handles the case of a zero-length request. > > Source/WebKit2/UIProcess/API/gtk/WebKitURISchemeRequest.cpp:162 > > + g_return_if_fail(streamLength == -1 || streamLength > 0); > > So it's not possible to send an empty reply? hmm, we could change that to be streamLength == -1 || streamLength >= 0. > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:52 > > + { > > + } > > + > > Nit: I think the curly braces are one indentation level too far to the right. emacs . . . :-) > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:36 > > +static const char* kBarHtml = "<html><body>Bar</body></html>"; > > +static const char* kEchoHtmlFormat = "<html><body>%s</body></html>"; > > Nit: Html -> HTML > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:53 > > + g_memory_input_stream_add_data(G_MEMORY_INPUT_STREAM(inputStream.get()), reinterpret_cast<gconstpointer>(kBarHtml), inputStreamLength, 0); > > Do you actually need to castkBarHTML and echoHTML here? I tried compiling this locally with g++ and had no errors: I don't know, I'll try again.

Carlos Garcia Campos

Comment 16 2012-05-31 10:16:27 PDT

(In reply to comment #14) > (In reply to comment #13) > > CCing Dan so he can give some insight about empty replies. > > it definitely seems like there might be use cases where you'd want to be able to return a 0-length response. > > > Also, should webkit_web_context_register_uri_scheme() integrate with SchemeRegistry in some way? WebKit2 only has messages to set url schemes as emptyDocument and Secure for now. You might want to register a scheme as whatever without using a custom handler no? So maybe it's better to have separate methods for those uses cases, what do you think?

Carlos Garcia Campos

Comment 17 2012-05-31 10:44:49 PDT

(In reply to comment #12) > (From update of attachment 141144 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141144&action=review > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:53 > > + g_memory_input_stream_add_data(G_MEMORY_INPUT_STREAM(inputStream.get()), reinterpret_cast<gconstpointer>(kBarHtml), inputStreamLength, 0); > > Do you actually need to castkBarHTML and echoHTML here? I tried compiling this locally with g++ and had no errors: You are right, the cast is not needed there.

Carlos Garcia Campos

Comment 18 2012-06-01 07:01:34 PDT

Created attachment 145296 [details] New patch Addresses all the review comments and adds test cases for errors loading the resource request and zero-length replies.

Carlos Garcia Campos

Comment 19 2012-06-01 07:02:36 PDT

Depends on https://bugs.webkit.org/show_bug.cgi?id=88087

Carlos Garcia Campos

Comment 20 2012-06-07 03:16:36 PDT

Committed r119700: <http://trac.webkit.org/changeset/119700>

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