[Webkit-unassigned] [Bug 112419] [GTK][WK2] MiniBrowser custom URI scheme support

Sun Mar 31 03:48:07 PDT 2013

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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #193260|review?                     |review-
               Flag|                            |

--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-03-31 03:46:18 PST ---
(From update of attachment 193260)
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->webView), 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.

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