Bug 110328 - Web Inspector: Add keyboard shortcut for switching to last used dock configuration
Summary: Web Inspector: Add keyboard shortcut for switching to last used dock configur...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: EasyFix, InRadar
Depends on:
Blocks:
 
Reported: 2013-02-20 04:58 PST by Masataka Yakura
Modified: 2016-09-03 17:29 PDT (History)
15 users (show)

See Also:


Attachments
Patch (8.05 KB, patch)
2016-09-01 14:31 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2016-09-02 14:24 PDT, Devin Rousso
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (1.73 MB, application/zip)
2016-09-02 15:10 PDT, Build Bot
no flags Details
Patch (8.46 KB, patch)
2016-09-03 16:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Masataka Yakura 2013-02-20 04:58:16 PST
It would be so nice if there's a keyboard shortcut since I switch dock side pretty often while developing.
Comment 1 Brian Burg 2014-08-03 19:06:59 PDT
Fixing component
Comment 2 Radar WebKit Bug Importer 2014-08-03 19:07:15 PDT
<rdar://problem/17898381>
Comment 3 Devin Rousso 2016-09-01 14:31:25 PDT
Created attachment 287680 [details]
Patch
Comment 4 BJ Burg 2016-09-02 11:26:39 PDT
Comment on attachment 287680 [details]
Patch

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

Thanks for switching to the enums. But please change it to the simpler "toggleDockSide" strategy, and clarify what happen when not docked in the ChangeLog.

> Source/WebInspectorUI/ChangeLog:7
> +

Does it affect docking at all if the inspector is not attached?

> Source/WebInspectorUI/UserInterface/Base/Main.js:319
> +    this._switchToPreviousDockSideKeyboardShortcut = new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.CommandOrControl | WebInspector.KeyboardShortcut.Modifier.Shift, "D", this._switchToPreviousDockSide.bind(this));

There are only two dock sides, so this should be called toggleDockSideKeyboardShortcut. And similar for the other parts of the patch. I don't think we even need to keep track of previous dock side, just switch it if docked already.
Comment 5 Devin Rousso 2016-09-02 14:19:43 PDT
Comment on attachment 287680 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:7
>> +
> 
> Does it affect docking at all if the inspector is not attached?

Yes.  I will rename this bug so that it better clarifies what this patch does.  The idea is that pressing Cmd+Shift+D will toggle the dock "configuration" to whatever was previously used.  This means that if WebInspector is docked right, and you decide to switch to docked bottom, pressing Cmd+Shift+D will toggle between right and bottom.  This also applies to undocked (e.g. if you are docked right and switch to undocked, Cmd+Shift+D will toggle between right and undocked).

>> Source/WebInspectorUI/UserInterface/Base/Main.js:319
>> +    this._switchToPreviousDockSideKeyboardShortcut = new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.CommandOrControl | WebInspector.KeyboardShortcut.Modifier.Shift, "D", this._switchToPreviousDockSide.bind(this));
> 
> There are only two dock sides, so this should be called toggleDockSideKeyboardShortcut. And similar for the other parts of the patch. I don't think we even need to keep track of previous dock side, just switch it if docked already.

See above.  I will rename this to _togglePreviousDockConfigurationKeyboardShortcut.
Comment 6 Devin Rousso 2016-09-02 14:24:57 PDT
Created attachment 287814 [details]
Patch
Comment 7 Build Bot 2016-09-02 15:10:36 PDT
Comment on attachment 287814 [details]
Patch

Attachment 287814 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1996442

New failing tests:
fast/dom/HTMLFormElement/document-deactivation-callback-crash.html
Comment 8 Build Bot 2016-09-02 15:10:40 PDT
Created attachment 287823 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 BJ Burg 2016-09-03 14:32:04 PDT
Comment on attachment 287814 [details]
Patch

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

r=me. I found a typo.

> Source/WebInspectorUI/ChangeLog:25
> +         - Left

I think this should say 'Bottom'

> Source/WebInspectorUI/ChangeLog:26
> +         - Right

At some point we may change this to "trailing" if we support RTL, but don't worry for now.
Comment 10 Devin Rousso 2016-09-03 16:58:18 PDT
Created attachment 287875 [details]
Patch
Comment 11 WebKit Commit Bot 2016-09-03 17:29:35 PDT
Comment on attachment 287875 [details]
Patch

Clearing flags on attachment: 287875

Committed r205413: <http://trac.webkit.org/changeset/205413>
Comment 12 WebKit Commit Bot 2016-09-03 17:29:40 PDT
All reviewed patches have been landed.  Closing bug.