[webkit-reviews] review granted: [Bug 84133] [GTK] Add API to register custom URI schemes to WebKit2 GTK+ API : [Attachment 137495] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 26 08:48:53 PDT 2012


Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 84133: [GTK] Add API to register custom URI schemes to WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=84133

Attachment 137495: Patch
https://bugs.webkit.org/attachment.cgi?id=137495&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.


More information about the webkit-reviews mailing list