| 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: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Manuel Rego Casasnovas
2013-03-15 02:03:01 PDT
Created attachment 193260 [details]
Patch
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. (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 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 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. Created attachment 195949 [details]
Patch
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 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. Created attachment 195959 [details]
Patch
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.
Created attachment 196467 [details]
Patch
Fix style issues.
Comment on attachment 196467 [details] Patch Clearing flags on attachment: 196467 Committed r147624: <http://trac.webkit.org/changeset/147624> All reviewed patches have been landed. Closing bug. |