[webkit-reviews] review denied: [Bug 112419] [GTK][WK2] MiniBrowser custom URI scheme support : [Attachment 193260] Patch

Sun Mar 31 03:48:06 PDT 2013

Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 112419: [GTK][WK2] MiniBrowser custom URI scheme support
https://bugs.webkit.org/show_bug.cgi?id=112419

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=193260&action=review

Looks good in general, but there are a few minor issues.

> Tools/MiniBrowser/gtk/BrowserWindow.c:74
> +static const char* getInternalURI(const char* uri)

This is a C file, so the * are placed correctly:

static const char *getInternalURI(const char *uri)

> Tools/MiniBrowser/gtk/BrowserWindow.c:76
> +    // Internally we use minibrowser-about: as about: prefix is ignored by
WebKit

Comments should finish with a period.

> Tools/MiniBrowser/gtk/BrowserWindow.c:78
> +	   return g_strconcat(miniBrowserAboutScheme, uri + strlen ("about"),
NULL);

The function returns a const char *, but this is retuning a new allocated
string. The function should return char * and duplicated the uri whe it's not
an about one. If you want to avoid the unnecessary string duplication when it's
not an about URI, you can move the check out of the function and only call it
when you know it's an about URI or you can cache the current uri in the browser
window struct.

> Tools/MiniBrowser/gtk/BrowserWindow.c:89
> +static const char* getExternalURI(const char* uri)
> +{
> +    // From the user point of view we support about: prefix
> +    if (g_str_has_prefix(uri, miniBrowserAboutScheme))
> +	   return g_strconcat("about", uri + strlen(miniBrowserAboutScheme),
NULL);
> +
> +    return uri;

Ditto everything.

> Tools/MiniBrowser/gtk/BrowserWindow.c:102
> +	   contents = g_strdup_printf("<html><body><p>WebKitGTK+ MiniBrowser
about page</p></body></html>");

Maybe we can make this a bit more useful and include more information like the
webkitgtk version number, for example.

> Tools/MiniBrowser/gtk/BrowserWindow.c:617
> +   
webkit_web_context_register_uri_scheme(webkit_web_view_get_context(window->webV
iew), miniBrowserAboutScheme, aboutURISchemeRequestCallback, 0, 0);

You don't want do this for every browser window, but only once for the default
web context. You can use NULL instead of 0 here.