Bug 112419

Summary:

[GTK][WK2] MiniBrowser custom URI scheme support

Product:

WebKit

Reporter:

Manuel Rego Casasnovas <rego>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cgarcia, mrobinson, rniwa, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Manuel Rego Casasnovas

Reported 2013-03-15 02:03:01 PDT

Add support for "about:foo" URIs in MiniBrowser.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Manuel Rego Casasnovas

Comment 1 2013-03-15 02:21:50 PDT

Created attachment 193260 [details] Patch

Manuel Rego Casasnovas

Comment 2 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.

Manuel Rego Casasnovas

Comment 3 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).

Build Bot

Comment 4 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

Carlos Garcia Campos

Comment 5 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.

Manuel Rego Casasnovas

Comment 6 2013-04-01 03:54:03 PDT

Created attachment 195949 [details] Patch

WebKit Review Bot

Comment 7 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.

Carlos Garcia Campos

Comment 8 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.

Manuel Rego Casasnovas

Comment 9 2013-04-01 05:04:57 PDT

Created attachment 195959 [details] Patch

WebKit Review Bot

Comment 10 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.

Manuel Rego Casasnovas

Comment 11 2013-04-04 05:04:22 PDT

Created attachment 196467 [details] Patch Fix style issues.

WebKit Review Bot

Comment 12 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>

WebKit Review Bot

Comment 13 2013-04-04 06:24:52 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.