DevTools: Implement extensions-based shortcut bindings

DevTools: Implement extensions-based shortcut bindings

R=eustas, pfeldman, vsevik
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170156

[pfeldman, 2014-02-21 16:29:16]

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
File Source/devtools/front_end/KeyboardShortcut.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:107:
WebInspector.KeyboardShortcut.KeyBindings = {
Lets not copy these definitions.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:316:
WebInspector.KeyboardShortcut.loadBindings = function()
What is this loading?

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
Source/devtools/front_end/inspector.js:814:
WebInspector.SkipAllPausesAndReloadActionDelegate = function()
This does not belong to the inspector.js

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
Source/devtools/front_end/inspector.js:835:
WebInspector.DebugReloadActionDelegate = function()
ditto

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
File Source/devtools/front_end/modules.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:38: bindings: [
What about mac shortcuts? F5 should do what Cmd+R does.

shodtcuts: {
    mac: "Cmd+r",
    nonmac: "F5,Ctrl+R"
}

Then you use modifiers on the event to choose between the options (reload
with(out) cache, breakpoints, etc.)

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:208: shortcut: "CtrlOrMeta+O"
nonmac:"ctrl+o",
mac:"cmd+o"

[apavlov, 2014-02-24 09:57:56]

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
File Source/devtools/front_end/KeyboardShortcut.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:107:
WebInspector.KeyboardShortcut.KeyBindings = {
On 2014/02/21 16:29:16, pfeldman wrote:
> Lets not copy these definitions.

OK. But do we want to support "?" or replace it by "Shift+/" (and a few other
keys in a similar manner)? Should I perhaps invest some time into implementing
the new DOM4 KeyboardEvent fields? (garykac@ never responded to my queries about
the state of the feature)

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:316:
WebInspector.KeyboardShortcut.loadBindings = function()
On 2014/02/21 16:29:16, pfeldman wrote:
> What is this loading?

It is called from inspector.js once modules have been registered, in order to
create the eventdescriptor->handler map.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
Source/devtools/front_end/inspector.js:814:
WebInspector.SkipAllPausesAndReloadActionDelegate = function()
On 2014/02/21 16:29:16, pfeldman wrote:
> This does not belong to the inspector.js

I was unsure about these two, since they don't seem belong to ResourceTreeModel
either. What would be the best location to place these into?

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
File Source/devtools/front_end/modules.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:38: bindings: [
On 2014/02/21 16:29:16, pfeldman wrote:
> What about mac shortcuts? F5 should do what Cmd+R does.

Not quite. As per the current code, CtrlOrCmd+R reloads the page skipping all
breakpoints. See the "CtrlOrMeta+R" shortcut and its action below.

> shodtcuts: {
>     mac: "Cmd+r",
>     nonmac: "F5,Ctrl+R"
> }

This approach does not seem flexible enough, since we may need alternative
shortcuts for Windows (panel history traversal in InspectorView), that's why I
made the shortcuts per-platform (platforms can be delimited by commas in the
list, btw)

> Then you use modifiers on the event to choose between the options (reload
> with(out) cache, breakpoints, etc.)

Right, but the Ctrl+R and F5 behaviors seem different (breakpoint skipping)
enough to have distinct actions for these shortcuts.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:208: shortcut: "CtrlOrMeta+O"
On 2014/02/21 16:29:16, pfeldman wrote:
> nonmac:"ctrl+o",
> mac:"cmd+o"

CtrlOrMeta is a shortcut to this :) Fine with separating in a strict manner.

[pfeldman, 2014-03-13 05:24:02]

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
File Source/devtools/front_end/KeyboardShortcut.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:107:
WebInspector.KeyboardShortcut.KeyBindings = {
I don't really see how these correlate. I was just opposed to duping the maps.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:316:
WebInspector.KeyboardShortcut.loadBindings = function()
It should be called differently then - loadBindings is about loading from
settings, while you are registering actions from extensions.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
File Source/devtools/front_end/modules.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:38: bindings: [
Ctrl+R vs F5 behaving differently is just an oversight.
As for platforms, you scheme would be fine (thought too verbose to my taste), if
it was not using logical expressions ("!mac").

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:208: shortcut: "CtrlOrMeta+O"
> CtrlOrMeta is a shortcut to this :) Fine with separating in a strict manner.

It is not clear what this means and it would be hard for user to configure it.
You should keep in mind that we'd want them configurable.

[apavlov, 2014-03-17 13:40:59]

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
File Source/devtools/front_end/KeyboardShortcut.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/Ke...
Source/devtools/front_end/KeyboardShortcut.js:107:
WebInspector.KeyboardShortcut.KeyBindings = {
On 2014/03/13 05:24:02, pfeldman wrote:
> I don't really see how these correlate. I was just opposed to duping the maps.

Okay, I'll build up the required map automatically using the Keys map (since I
need to quickly retrieve the keycode corresponding to any character than can
appear in a shortcut descriptor).

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
File Source/devtools/front_end/inspector.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/in...
Source/devtools/front_end/inspector.js:835:
WebInspector.DebugReloadActionDelegate = function()
On 2014/02/21 16:29:16, pfeldman wrote:
> ditto

Agreed on leaving these here for now offline.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
File Source/devtools/front_end/modules.js (right):

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:38: bindings: [
On 2014/03/13 05:24:02, pfeldman wrote:
> Ctrl+R vs F5 behaving differently is just an oversight.
> As for platforms, you scheme would be fine (thought too verbose to my taste),
if
> it was not using logical expressions ("!mac").

Unified the behavior and removed logical expressions, so that the platforms now
should be listed explicitly.

https://codereview.chromium.org/170273003/diff/1/Source/devtools/front_end/mo...
Source/devtools/front_end/modules.js:208: shortcut: "CtrlOrMeta+O"
On 2014/03/13 05:24:02, pfeldman wrote:
> > CtrlOrMeta is a shortcut to this :) Fine with separating in a strict manner.
> 
> It is not clear what this means and it would be hard for user to configure it.
> You should keep in mind that we'd want them configurable.

Yep, split these into "mac" and "win,linux" configurations.

[pfeldman, 2014-03-24 16:54:22]

lgtm

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/ConsoleModel.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/ConsoleModel.js:430:
WebInspector.ConsoleModel.ShowConsoleActionDelegate = function()
This should be a part of the console module.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/KeyboardShortcut.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:59: Backspace: { code: 8, name:
"\u21a4", shortcut: "backspace" },
Just use the key for this.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:299:
WebInspector.KeyboardShortcut._onkeypress = function(event)
_onKeyPress

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:325: isMatch |= (platforms[i] ===
currentPlatform);
There is no logical |=

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:337: for (var i = 0; i <
bindings.length; ++i) {
bindings &&

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:350:
document.addEventListener("keypress", WebInspector.KeyboardShortcut._onkeypress,
true);
Move this above.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/Main.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/Main.js:55: configuration = ["core", "sources",
"timeline", "profiles", "console", "codemirror"];
main?

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/Main.js:710: return false;
inline

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/Main.js:750: if (window.DEBUG) {
Reload should always work.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/ModuleManager.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/ModuleManager.js:424: * @param {!KeyboardEvent=}
keyEvent
Event

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/SettingsScreen.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/SettingsScreen.js:736:
WebInspector.settingsController.showSettingsScreen(!!keyEvent ?
WebInspector.SettingsScreen.Tabs.General : undefined);
How can there be no keyEvent?

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/SourcesPanel.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/SourcesPanel.js:1172:
WebInspector.SourcesPanel.EditorAction = function()
Poor merge

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/modules.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/modules.js:41: shortcut: "F5"
Space-separated?

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/modules.js:78: className:
"WebInspector.Main.SkipAllPausesAndHardReloadActionDelegate"
WebInspector.Main.HardReloadActionDelegate

[apavlov, 2014-03-25 10:03:44]

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/ConsoleModel.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/ConsoleModel.js:430:
WebInspector.ConsoleModel.ShowConsoleActionDelegate = function()
On 2014/03/24 16:54:22, pfeldman wrote:
> This should be a part of the console module.

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/KeyboardShortcut.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:59: Backspace: { code: 8, name:
"\u21a4", shortcut: "backspace" },
On 2014/03/24 16:54:22, pfeldman wrote:
> Just use the key for this.

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:299:
WebInspector.KeyboardShortcut._onkeypress = function(event)
On 2014/03/24 16:54:22, pfeldman wrote:
> _onKeyPress

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:325: isMatch |= (platforms[i] ===
currentPlatform);
On 2014/03/24 16:54:22, pfeldman wrote:
> There is no logical |=

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:337: for (var i = 0; i <
bindings.length; ++i) {
On 2014/03/24 16:54:22, pfeldman wrote:
> bindings && 

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/KeyboardShortcut.js:350:
document.addEventListener("keypress", WebInspector.KeyboardShortcut._onkeypress,
true);
On 2014/03/24 16:54:22, pfeldman wrote:
> Move this above.

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/Main.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/Main.js:55: configuration = ["core", "sources",
"timeline", "profiles", "console", "codemirror"];
On 2014/03/24 16:54:22, pfeldman wrote:
> main?

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/Main.js:710: return false;
On 2014/03/24 16:54:22, pfeldman wrote:
> inline

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/Main.js:750: if (window.DEBUG) {
On 2014/03/24 16:54:22, pfeldman wrote:
> Reload should always work.

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/ModuleManager.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/ModuleManager.js:424: * @param {!KeyboardEvent=}
keyEvent
On 2014/03/24 16:54:22, pfeldman wrote:
> Event

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/SettingsScreen.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/SettingsScreen.js:736:
WebInspector.settingsController.showSettingsScreen(!!keyEvent ?
WebInspector.SettingsScreen.Tabs.General : undefined);
On 2014/03/24 16:54:22, pfeldman wrote:
> How can there be no keyEvent?

That was a provisional version to handle both keyboard and mouse click
invocation.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/SourcesPanel.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/SourcesPanel.js:1172:
WebInspector.SourcesPanel.EditorAction = function()
On 2014/03/24 16:54:22, pfeldman wrote:
> Poor merge

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
File Source/devtools/front_end/modules.js (right):

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/modules.js:41: shortcut: "F5"
On 2014/03/24 16:54:22, pfeldman wrote:
> Space-separated?

Done.

https://chromiumcodereview.appspot.com/170273003/diff/220001/Source/devtools/...
Source/devtools/front_end/modules.js:78: className:
"WebInspector.Main.SkipAllPausesAndHardReloadActionDelegate"
On 2014/03/24 16:54:22, pfeldman wrote:
> WebInspector.Main.HardReloadActionDelegate

Done.

[apavlov, 2014-03-25 10:04:23]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-03-25 10:04:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/170273003/240001

[commit-bot: I haz the power, 2014-03-25 10:33:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 10:33:16]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[apavlov, 2014-03-27 08:50:50]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-03-27 08:50:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/170273003/270001

[commit-bot: I haz the power, 2014-03-27 09:49:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 09:49:37]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[apavlov, 2014-03-27 09:51:09]

The CQ bit was checked by apavlov@chromium.org

[commit-bot: I haz the power, 2014-03-27 09:51:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/170273003/270001

[commit-bot: I haz the power, 2014-03-27 09:52:00]

Change committed as 170156