[webkit-reviews] review denied: [Bug 128813] [GTK] Minibrowser: Add shortcut to open Web Inspector : [Attachment 224210] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 14 08:25:19 PST 2014
Sergio Villar Senin <svillar at igalia.com> has denied Diego Pino
<dpino at igalia.com>'s request for review:
Bug 128813: [GTK] Minibrowser: Add shortcut to open Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=128813
Attachment 224210: Patch
https://bugs.webkit.org/attachment.cgi?id=224210&action=review
------- Additional Comments from Sergio Villar Senin <svillar at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224210&action=review
There are a couple of minor nits, but this is a r- because the logic is not
correct.
> Tools/ChangeLog:8
> + Added shortcut Ctrl+i for toggling Web Inspector.
Is this shortcut used by some other port? Is it used in GtkLauncher?
> Tools/MiniBrowser/gtk/BrowserWindow.c:503
> +static gboolean toggleWebInspector(GObject *gObject, gpointer user_data)
Don't use camel case for attributes in this file. Also I guess you can use here
BrowserWindow directly instead of GObject as you're using G_CALLBACK().
> Tools/MiniBrowser/gtk/BrowserWindow.c:511
> + height = webkit_web_inspector_get_attached_height(inspectorWindow);
If you're going to use the variable just once, better do not define it and
directly use the return value from the function.
In any case I don't think the logic is correct. If the inspector is shown in a
dettached mode, the height will be 0 so you won't close it.
More information about the webkit-reviews
mailing list