Bug 84133 - [GTK] Add API to register custom URI schemes to WebKit2 GTK+ API
Summary: [GTK] Add API to register custom URI schemes to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 84130 85880 88087
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 01:57 PDT by Carlos Garcia Campos
Modified: 2012-06-07 03:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (34.54 KB, patch)
2012-04-17 02:14 PDT, Carlos Garcia Campos
mrobinson: review-
gns: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-04-17 01:57:58 PDT
Allow users to register custom URI schemes, like about: pages.
Comment 1 Carlos Garcia Campos 2012-04-17 02:14:30 PDT
Created attachment 137495 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Gustavo Noronha (kov) 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
Comment 4 Carlos Garcia Campos 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
Comment 5 Martin Robinson 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.
Comment 6 Carlos Garcia Campos 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 ("&lt;html&gt;&lt;body&gt;&lt;p&gt;Invalid about:%s page&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;", 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.
Comment 7 Carlos Garcia Campos 2012-04-26 09:14:48 PDT
Please, note that this depends on #84130 which is still pending review
Comment 8 Martin Robinson 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.
Comment 9 Carlos Garcia Campos 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.
Comment 10 Carlos Garcia Campos 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
Comment 11 Carlos Garcia Campos 2012-05-10 04:24:06 PDT
Created attachment 141144 [details]
Updated patch

Updated to the changes made in bug #85880
Comment 12 Martin Robinson 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.
Comment 13 Martin Robinson 2012-05-30 13:52:27 PDT
CCing Dan so he can give some insight about empty replies.
Comment 14 Dan Winship 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?
Comment 15 Carlos Garcia Campos 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.
Comment 16 Carlos Garcia Campos 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?
Comment 17 Carlos Garcia Campos 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.
Comment 18 Carlos Garcia Campos 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.
Comment 19 Carlos Garcia Campos 2012-06-01 07:02:36 PDT
Depends on https://bugs.webkit.org/show_bug.cgi?id=88087
Comment 20 Carlos Garcia Campos 2012-06-07 03:16:36 PDT
Committed r119700: <http://trac.webkit.org/changeset/119700>