RESOLVED FIXED 112419
[GTK][WK2] MiniBrowser custom URI scheme support 
https://bugs.webkit.org/show_bug.cgi?id=112419
Summary [GTK][WK2] MiniBrowser custom URI scheme support
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
DetailsFormatted DiffDiff
Patch (6.26 KB, patch)
2013-04-01 03:54 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (6.66 KB, patch)
2013-04-01 05:04 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (6.59 KB, patch)
2013-04-04 05:04 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
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.