Bug 112419 - [GTK][WK2] MiniBrowser custom URI scheme support
Summary: [GTK][WK2] MiniBrowser custom URI scheme support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-15 02:03 PDT by Manuel Rego Casasnovas
Modified: 2013-04-04 06:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.52 KB, patch)
2013-03-15 02:21 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.26 KB, patch)
2013-04-01 03:54 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.66 KB, patch)
2013-04-01 05:04 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (6.59 KB, patch)
2013-04-04 05:04 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2013-03-15 02:03:01 PDT
Add support for "about:foo" URIs in MiniBrowser.
Comment 1 Manuel Rego Casasnovas 2013-03-15 02:21:50 PDT
Created attachment 193260 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2013-03-15 02:23:26 PDT
The patch works properly in the stable branch.

However in trunk when you try to go to "about:minibrowser" (or any other "about:" URI) you get the following message :
URL cannot be shown

I still need to figure out what happens there, but I'd like to gather your feedback about the patch anyway.
Comment 3 Manuel Rego Casasnovas 2013-03-15 02:38:30 PDT
(In reply to comment #2)
> However in trunk when you try to go to "about:minibrowser" (or any other "about:" URI) you get the following message :
> URL cannot be shown
> 
> I still need to figure out what happens there, but I'd like to gather your feedback about the patch anyway.

Maybe the problem in trunk is related with bug #112421 (not sure yet).
Comment 4 Build Bot 2013-03-15 06:04:33 PDT
Comment on attachment 193260 [details]
Patch

Attachment 193260 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17179178

New failing tests:
fast/css/sticky/inline-sticky.html
Comment 5 Carlos Garcia Campos 2013-03-31 03:46:18 PDT
Comment on attachment 193260 [details]
Patch

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.
Comment 6 Manuel Rego Casasnovas 2013-04-01 03:54:03 PDT
Created attachment 195949 [details]
Patch
Comment 7 WebKit Review Bot 2013-04-01 03:55:41 PDT
Attachment 195949 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/main.c']" exit_code: 1
Tools/MiniBrowser/gtk/main.c:213:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/MiniBrowser/gtk/main.c:213:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/main.c:214:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/MiniBrowser/gtk/main.c:214:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/main.c:215:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/MiniBrowser/gtk/main.c:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Carlos Garcia Campos 2013-04-01 04:04:18 PDT
Comment on attachment 195949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195949&action=review

Looks better, setting r- because of the leaks and style issues.

> Tools/MiniBrowser/gtk/BrowserWindow.c:140
> +    gtk_entry_set_text(GTK_ENTRY(window->uriEntry), getExternalURI(webkit_web_view_get_uri(webView)));

This is leaking the uri, you should either use a variable to free the uri after set_text, or cache the uri in the window structure and make getExternalURI return a const char *

> Tools/MiniBrowser/gtk/BrowserWindow.c:301
> +        titleOrURI = getExternalURI(webkit_web_view_get_uri(window->webView));

titleOrURI is const char, getExternalURI returns a new allocated string that you are leaking here.

> Tools/MiniBrowser/gtk/BrowserWindow.c:665
> +        webkit_web_view_load_uri(window->webView, getInternalURI(uri));

Same leak here.

> Tools/MiniBrowser/gtk/main.c:212
> +        contents = g_strdup_printf("<html><body><h1>WebKitGTK+ MiniBrowser</h1><p>About page example.</p><p>Version: %d.%d.%d</p></body></html>",

We don't need to say this is an about page example, just a use a sentence like any other about app. WebKitGTK+ MiniBrowser 2.0.0 The WebKit2 test browser of the GTK+ port. Or something similar.

>>> Tools/MiniBrowser/gtk/main.c:215
>>> +                                   webkit_get_micro_version ());
>> 
>> Extra space before ( in function call  [whitespace/parens] [4]
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Fix all the style issue please.
Comment 9 Manuel Rego Casasnovas 2013-04-01 05:04:57 PDT
Created attachment 195959 [details]
Patch
Comment 10 WebKit Review Bot 2013-04-01 05:07:17 PDT
Attachment 195959 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/gtk/main.c']" exit_code: 1
Tools/MiniBrowser/gtk/main.c:213:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/main.c:214:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/main.c:215:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Manuel Rego Casasnovas 2013-04-04 05:04:22 PDT
Created attachment 196467 [details]
Patch

Fix style issues.
Comment 12 WebKit Review Bot 2013-04-04 06:24:48 PDT
Comment on attachment 196467 [details]
Patch

Clearing flags on attachment: 196467

Committed r147624: <http://trac.webkit.org/changeset/147624>
Comment 13 WebKit Review Bot 2013-04-04 06:24:52 PDT
All reviewed patches have been landed.  Closing bug.