Bug 128813 – [GTK] Minibrowser: Add shortcut to open Web Inspector

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Diego Pino]

To open the Web Inspector it is necessary to right-click on the current page and select "Inspect Element" in the context menu. Safari or MiniBrowser EFL have a shortcut to open the Web Inspector directly. That is more comfortable.

[Diego Pino]

Created attachment 224210 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 224210 [details]
Patch

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.

[Diego Pino]

Created attachment 224254 [details]
Patch

[Martin Robinson]

Comment on attachment 224254 [details]
Patch

Both Firefox and Chromium use F12 as the inspector toggle shortcut.

[Diego Pino]

Comment on attachment 224210 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224210&action=review

>> Tools/ChangeLog:8
>> +        Added shortcut Ctrl+i for toggling Web Inspector.
> 
> Is this shortcut used by some other port? Is it used in GtkLauncher?

Yes, Minibrowser EFL uses Ctrl+i to open the Web Inspector (Tools/MiniBrowser/efl/main.c:444):

else if (!strcmp(ev->key, "i") && ctrlPressed) {
        info("Show Inspector (Ctrl+i) was pressed.");
        ewk_view_inspector_show(ewk_view);
    }

GtkLauncher doesn't implement this shortcut.

>> 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().

Ok

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

Yes, that's correct. Finally I implemented visibility of the window registering to signals "open-window" and "closed" to know when the window has been detached but it's still opened and when the window was closed.

[Diego Pino]

(In reply to comment #4)
> (From update of attachment 224254 [details])
> Both Firefox and Chromium use F12 as the inspector toggle shortcut.

I didn't know about that. Minibrowser EFL uses Ctrl+i and Safari uses Command+Option+i (Ctrl+Alt+i in PCs).

https://developer.apple.com/library/mac/documentation/AppleApplications/Conceptual/Safari_Developer_Guide/KeyboardShortcuts/KeyboardShortcuts.html

I initially thought of implemented as Ctrl+i as it's close to what other WebKit browsers (Minibrowser EFL and Safari) but if other browsers use F12 we can use that too.

[Anders Carlsson]

Oops, I missed the discussion in the bug when I set cq+. Resetting cq flag (hopefully this won't be landed!)

[Diego Pino]

I checked Firefox and it uses 'Ctrl+Shift+I' for opening up its Toolbox.

https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts

Chromium uses 'Ctrl+Shift+I' too.

http://www.shortcutworld.com/en/linux/Chrome.html

Martin, I think you were thinking of Firebug which uses Ctrl+F12.

https://getfirebug.com/enable

So, I think the shortcut should involve the 'Ctrl' and 'I' keys. Should 'Shift' also be included?

[Martin Robinson]

(In reply to comment #8)

> Martin, I think you were thinking of Firebug which uses Ctrl+F12.
> 
> https://getfirebug.com/enable

It seems that F12 and Ctrl+Shift+i both do the same thing. I do not have firebug installed. Using both Ctrl+Shift+i and F12 sounds find to me. That will make our behavior match Firefox and Chrome.

[Diego Pino]

Created attachment 224527 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 224527 [details]
Patch

Clearing flags on attachment: 224527

Committed r164307: <http://trac.webkit.org/changeset/164307>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.