Bug 128813 - [GTK] Minibrowser: Add shortcut to open Web Inspector
Summary: [GTK] Minibrowser: Add shortcut to open Web Inspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-14 06:32 PST by Diego Pino
Modified: 2014-02-18 12:42 PST (History)
5 users (show)

See Also:


Attachments
Patch (2.27 KB, patch)
2014-02-14 06:44 PST, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (3.96 KB, patch)
2014-02-14 14:19 PST, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (4.12 KB, patch)
2014-02-18 11:28 PST, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Pino 2014-02-14 06:32:35 PST
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.
Comment 1 Diego Pino 2014-02-14 06:44:19 PST
Created attachment 224210 [details]
Patch
Comment 2 Sergio Villar Senin 2014-02-14 08:22:34 PST
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.
Comment 3 Diego Pino 2014-02-14 14:19:27 PST
Created attachment 224254 [details]
Patch
Comment 4 Martin Robinson 2014-02-14 14:22:11 PST
Comment on attachment 224254 [details]
Patch

Both Firefox and Chromium use F12 as the inspector toggle shortcut.
Comment 5 Diego Pino 2014-02-14 14:25:15 PST
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.
Comment 6 Diego Pino 2014-02-14 14:30:45 PST
(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.
Comment 7 Anders Carlsson 2014-02-15 09:04:09 PST
Oops, I missed the discussion in the bug when I set cq+. Resetting cq flag (hopefully this won't be landed!)
Comment 8 Diego Pino 2014-02-18 03:37:36 PST
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?
Comment 9 Martin Robinson 2014-02-18 07:34:03 PST
(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.
Comment 10 Diego Pino 2014-02-18 11:28:42 PST
Created attachment 224527 [details]
Patch
Comment 11 WebKit Commit Bot 2014-02-18 12:42:09 PST
Comment on attachment 224527 [details]
Patch

Clearing flags on attachment: 224527

Committed r164307: <http://trac.webkit.org/changeset/164307>
Comment 12 WebKit Commit Bot 2014-02-18 12:42:11 PST
All reviewed patches have been landed.  Closing bug.