[webkit-reviews] review denied: [Bug 112419] [GTK][WK2] MiniBrowser custom URI scheme support : [Attachment 195949] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 1 04:06:06 PDT 2013


Carlos Garcia Campos <cgarcia at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 112419: [GTK][WK2] MiniBrowser custom URI scheme support
https://bugs.webkit.org/show_bug.cgi?id=112419

Attachment 195949: Patch
https://bugs.webkit.org/attachment.cgi?id=195949&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.


More information about the webkit-reviews mailing list