Bug 110328

Summary: Web Inspector: Add keyboard shortcut for switching to last used dock configuration
Product: WebKit Reporter: Masataka Yakura <myakura.web>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bburg, commit-queue, hi, joepeck, keishi, loislo, pfeldman, pmuellr, syoichi, timothy, vsevik, web-inspector-bugs, webkit-bug-importer, yurys
Priority: P2 Keywords: EasyFix, InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
bburg: review+, bburg: commit-queue-
Archive of layout-test-results from ews113 for mac-yosemite
none
Patch none

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.