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

Thu Apr 26 08:48:54 PDT 2012

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

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #137495|review?                     |review+
               Flag|                            |

--- Comment #5 from Martin Robinson <mrobinson at webkit.org>  2012-04-26 08:48:54 PST ---
(From update of attachment 137495)
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.

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