[Webkit-unassigned] [Bug 84133] [GTK] Add API to register custom URI schemes to WebKit2 GTK+ API

Thu Apr 26 09:11:08 PDT 2012

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

--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-04-26 09:11:08 PST ---
(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.

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