Devtools UI: Show multiple shortcuts, show more shortcuts

Devtools UI: Show multiple shortcuts, show more shortcuts

This change allows multiple shortcuts for the same action to be shown
in the tooltip, allowing the user to understand different shortcut options
available.

Toolbar buttons can also have defined shortcuts, which record
in timeline and profiles use to show shortcuts.

Instead of showing a symbol for Esc on Mac, this change now shows Esc for the
escape key.

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

[samli, 2015-08-07 04:33:38]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-08-07 04:33:39]

PTAL.

Screenshot multiple shortcuts:
http://i.imgur.com/YqAgzFV.png

Mac escape icon to "Esc" requested by Paul.

[pfeldman, 2015-08-07 19:54:13]

https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/console/ConsoleView.js (right):

https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/console/ConsoleView.js:63:
this._clearConsoleButton.setShortcut([WebInspector.KeyboardShortcut.makeDescriptor("l",
WebInspector.KeyboardShortcut.Modifiers.Ctrl)]);
We should instead fetch the shortcut by the action id. See
WebInspector.shortcutRegistry.shortcutTitleForAction(actionId);

[paulirish, 2015-08-11 01:11:18]

I like the multiple shortcuts, especially for step-wise debugging. lgtm from my
side.

[samli, 2015-08-11 01:59:44]

samli@chromium.org changed reviewers:
+ paulirish@chromium.org

[samli, 2015-08-11 01:59:45]

https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
File Source/devtools/front_end/console/ConsoleView.js (right):

https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
Source/devtools/front_end/console/ConsoleView.js:63:
this._clearConsoleButton.setShortcut([WebInspector.KeyboardShortcut.makeDescriptor("l",
WebInspector.KeyboardShortcut.Modifiers.Ctrl)]);
On 2015/08/07 at 19:54:13, pfeldman wrote:
> We should instead fetch the shortcut by the action id. See
WebInspector.shortcutRegistry.shortcutTitleForAction(actionId);

There is no action id for clear console at the moment. Should I add one and call
WebInspector.ConsolePanel._view._requestClearMessages()?

[pfeldman, 2015-08-11 04:10:18]

On 2015/08/11 01:59:45, samli wrote:
>
https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
> File Source/devtools/front_end/console/ConsoleView.js (right):
> 
>
https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
> Source/devtools/front_end/console/ConsoleView.js:63:
>
this._clearConsoleButton.setShortcut([WebInspector.KeyboardShortcut.makeDescriptor("l",
> WebInspector.KeyboardShortcut.Modifiers.Ctrl)]);
> On 2015/08/07 at 19:54:13, pfeldman wrote:
> > We should instead fetch the shortcut by the action id. See
> WebInspector.shortcutRegistry.shortcutTitleForAction(actionId);
> 
> There is no action id for clear console at the moment. Should I add one and
call
> WebInspector.ConsolePanel._view._requestClearMessages()?

Yes, but you should make that method static and move it to ConsoleModel.js.

[samli, 2015-08-11 05:27:18]

On 2015/08/11 at 04:10:18, pfeldman wrote:
> On 2015/08/11 01:59:45, samli wrote:
> >
https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
> > File Source/devtools/front_end/console/ConsoleView.js (right):
> > 
> >
https://codereview.chromium.org/1273363002/diff/1/Source/devtools/front_end/c...
> > Source/devtools/front_end/console/ConsoleView.js:63:
> >
this._clearConsoleButton.setShortcut([WebInspector.KeyboardShortcut.makeDescriptor("l",
> > WebInspector.KeyboardShortcut.Modifiers.Ctrl)]);
> > On 2015/08/07 at 19:54:13, pfeldman wrote:
> > > We should instead fetch the shortcut by the action id. See
> > WebInspector.shortcutRegistry.shortcutTitleForAction(actionId);
> > 
> > There is no action id for clear console at the moment. Should I add one and
call
> > WebInspector.ConsolePanel._view._requestClearMessages()?
> 
> Yes, but you should make that method static and move it to ConsoleModel.js.

Done.

[samli, 2015-08-11 05:27:28]

PTAL.

[pfeldman, 2015-08-11 05:34:57]

https://codereview.chromium.org/1273363002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1273363002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/Toolbar.js:415: this._shortcutKeys = keys;
We should have action ids for everything with the shortcut, especially the ones
we surface in the toolbar. Lets not add it and migrate record buttons, etc to
the new schema over time.

[samli, 2015-08-11 07:17:01]

Regresses shortcut start/stop. Not sure why yet, since it calls the same
function. For some reason, suspendState event happens twice using keyboard stop
recording.

https://codereview.chromium.org/1273363002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1273363002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/Toolbar.js:415: this._shortcutKeys = keys;
On 2015/08/11 at 05:34:56, pfeldman wrote:
> We should have action ids for everything with the shortcut, especially the
ones we surface in the toolbar. Lets not add it and migrate record buttons, etc
to the new schema over time.

Done.

[samli, 2015-08-11 07:32:50]

On 2015/08/11 at 07:17:01, samli wrote:
> Regresses shortcut start/stop. Not sure why yet, since it calls the same
function. For some reason, suspendState event happens twice using keyboard stop
recording.
Fixed. The panels were conflicting so now I check which panel is open.

> 
>
https://codereview.chromium.org/1273363002/diff/20001/Source/devtools/front_e...
> File Source/devtools/front_end/ui/Toolbar.js (right):
> 
>
https://codereview.chromium.org/1273363002/diff/20001/Source/devtools/front_e...
> Source/devtools/front_end/ui/Toolbar.js:415: this._shortcutKeys = keys;
> On 2015/08/11 at 05:34:56, pfeldman wrote:
> > We should have action ids for everything with the shortcut, especially the
ones we surface in the toolbar. Lets not add it and migrate record buttons, etc
to the new schema over time.
> 
> Done.

[samli, 2015-08-17 00:31:30]

ping pfeldman

[pfeldman, 2015-08-17 19:22:50]

Suggesting a little refactoring there. Let me know if you are comfortable
performing it.

https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/console/ConsoleView.js (right):

https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/console/ConsoleView.js:62:
this._clearConsoleButton.setAction("console.clear");
I think you should build it from the action and if you are doing so, the title
needs to go from the action:

this._clearConsoleButton = toolbar.addAction("console.clear") or similar.

https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/console/ConsoleView.js:582:
contextMenu.appendAction(WebInspector.UIString("Clear console"),
"console.clear");
Like we do to context menus in here.

https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
File Source/devtools/front_end/console/module.json (right):

https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
Source/devtools/front_end/console/module.json:34: "actionId": "console.clear",
So our actions should be named (have titles and maybe glyph names). Then we can
render them in context menus and toolbars uniformly.

[samli, 2015-08-18 03:26:24]

On 2015/08/17 at 19:22:50, pfeldman wrote:
> Suggesting a little refactoring there. Let me know if you are comfortable
performing it.
> 
>
https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
> File Source/devtools/front_end/console/ConsoleView.js (right):
> 
>
https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
> Source/devtools/front_end/console/ConsoleView.js:62:
this._clearConsoleButton.setAction("console.clear");
> I think you should build it from the action and if you are doing so, the title
needs to go from the action:
> 
> this._clearConsoleButton = toolbar.addAction("console.clear") or similar.
> 
>
https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
> Source/devtools/front_end/console/ConsoleView.js:582:
contextMenu.appendAction(WebInspector.UIString("Clear console"),
"console.clear");
> Like we do to context menus in here.
> 
>
https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
> File Source/devtools/front_end/console/module.json (right):
> 
>
https://codereview.chromium.org/1273363002/diff/60001/Source/devtools/front_e...
> Source/devtools/front_end/console/module.json:34: "actionId": "console.clear",
> So our actions should be named (have titles and maybe glyph names). Then we
can render them in context menus and toolbars uniformly.

Done. PTAL.

[pfeldman, 2015-08-18 05:08:31]

lgtm, but please move context menu to registry-provided titles!

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/ui/ActionRegistry.js (right):

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/ui/ActionRegistry.js:74: getActionDescriptor:
function(actionId)
s/get//

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/ui/ContextMenu.js (right):

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/ui/ContextMenu.js:159: appendAction:
function(actionId, label)
Lets remove the label where possible and migrate to .json version. To the extent
of removing the label param here if possible.

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/ui/Toolbar.js:674: var action =
WebInspector.actionRegistry.getActionDescriptor(actionId);
I would expose actionTitle and actionIcon by id in the registry.

[samli, 2015-08-19 05:27:16]

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/ui/ActionRegistry.js (right):

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/ui/ActionRegistry.js:74: getActionDescriptor:
function(actionId)
On 2015/08/18 at 05:08:31, pfeldman wrote:
> s/get//

Ack.

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/ui/ContextMenu.js (right):

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/ui/ContextMenu.js:159: appendAction:
function(actionId, label)
On 2015/08/18 at 05:08:31, pfeldman wrote:
> Lets remove the label where possible and migrate to .json version. To the
extent of removing the label param here if possible.

Done. Can't remove fully since Main.js:845 overrides the label depending on a
value.

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
File Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1273363002/diff/80001/Source/devtools/front_e...
Source/devtools/front_end/ui/Toolbar.js:674: var action =
WebInspector.actionRegistry.getActionDescriptor(actionId);
On 2015/08/18 at 05:08:31, pfeldman wrote:
> I would expose actionTitle and actionIcon by id in the registry.

Done.

[samli, 2015-08-19 05:27:18]

The CQ bit was checked by samli@chromium.org

[samli, 2015-08-19 05:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulirish@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1273363002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-19 05:27:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273363002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273363002/100001

[commit-bot: I haz the power, 2015-08-19 07:10:42]

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

[commit-bot: I haz the power, 2015-08-19 07:10:43]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[samli, 2015-08-21 02:41:12]

https://codereview.chromium.org/1273363002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/1273363002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/timeline/TimelinePanel.js:1819: if
(WebInspector.inspectorView.currentPanel().name !== "timeline")
Not sure how to better handle this. Couldn't find anything in context that would
help.

In the tests, the timeline record button is clicked while the elements panel is
open. There was no action before, so this triggered directly.
This check is necessary because the keyboard shortcut triggers actions in both
timeline/profiles.

[pfeldman, 2015-08-21 18:21:10]

https://codereview.chromium.org/1273363002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/1273363002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/timeline/TimelinePanel.js:1819: if
(WebInspector.inspectorView.currentPanel().name !== "timeline")
On 2015/08/21 02:41:12, samli wrote:
> Not sure how to better handle this. Couldn't find anything in context that
would
> help.
> 
> In the tests, the timeline record button is clicked while the elements panel
is
> open. There was no action before, so this triggered directly.
> This check is necessary because the keyboard shortcut triggers actions in both
> timeline/profiles.

For example, we could add timeline panel's instance by
WebInspector.TimelinePanel flavor in wasShown and remove it in willHide. We then
require this flavor to be non-empty via specifying the context in the
descriptor.

[samli, 2015-08-28 01:24:28]

https://codereview.chromium.org/1273363002/diff/100001/Source/devtools/front_...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/1273363002/diff/100001/Source/devtools/front_...
Source/devtools/front_end/timeline/TimelinePanel.js:1819: if
(WebInspector.inspectorView.currentPanel().name !== "timeline")
On 2015/08/21 at 18:21:09, pfeldman wrote:
> On 2015/08/21 02:41:12, samli wrote:
> > Not sure how to better handle this. Couldn't find anything in context that
would
> > help.
> > 
> > In the tests, the timeline record button is clicked while the elements panel
is
> > open. There was no action before, so this triggered directly.
> > This check is necessary because the keyboard shortcut triggers actions in
both
> > timeline/profiles.
> 
> For example, we could add timeline panel's instance by
WebInspector.TimelinePanel flavor in wasShown and remove it in willHide. We then
require this flavor to be non-empty via specifying the context in the
descriptor.

Done and fixed test. PTAL.

[samli, 2015-08-28 01:37:59]

The CQ bit was checked by samli@chromium.org

[samli, 2015-08-28 01:37:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulirish@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1273363002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-08-28 01:38:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273363002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273363002/140001

[commit-bot: I haz the power, 2015-08-28 03:45:06]

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

[commit-bot: I haz the power, 2015-08-28 03:45:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[samli, 2015-08-28 15:40:37]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2015-08-28 15:40:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273363002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273363002/140001

[pfeldman, 2015-08-28 16:19:47]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2015-08-28 16:22:02]

https://codereview.chromium.org/1273363002/diff/140001/LayoutTests/inspector/...
File
LayoutTests/inspector/tracing/styles-should-not-force-sync-style-recalc.html
(right):

https://codereview.chromium.org/1273363002/diff/140001/LayoutTests/inspector/...
LayoutTests/inspector/tracing/styles-should-not-force-sync-style-recalc.html:3:
<script src="../../http/tests/inspector/inspector-test.js"></script>
So we have two tests and I think I confused this one with the other one. There
is a test that checks that enabling timeline does not force style recalc and I
was talking about that one. And this one actually checks that CSSOM operations
with elements enabled (panel needs to be opened) does not force style recalc.

[pfeldman, 2015-08-28 16:24:45]

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/profiler/module.json (right):

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
Source/devtools/front_end/profiler/module.json:40: "type":
"@WebInspector.ActionDelegate",
You should specify context flavor here.

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
Source/devtools/front_end/timeline/TimelinePanel.js:1824: if (!panel || !(panel
instanceof WebInspector.TimelinePanel))
When you specify the context, this should become an assertion.

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/timeline/module.json (right):

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
Source/devtools/front_end/timeline/module.json:24: "type":
"@WebInspector.ActionDelegate",
ditto, context is missing.

[samli, 2015-08-28 17:21:23]

Done, PTAL.

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/profiler/module.json (right):

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
Source/devtools/front_end/profiler/module.json:40: "type":
"@WebInspector.ActionDelegate",
On 2015/08/28 at 16:24:45, pfeldman wrote:
> You should specify context flavor here.

Done.

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
Source/devtools/front_end/timeline/TimelinePanel.js:1824: if (!panel || !(panel
instanceof WebInspector.TimelinePanel))
On 2015/08/28 at 16:24:45, pfeldman wrote:
> When you specify the context, this should become an assertion.

Done.

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
File Source/devtools/front_end/timeline/module.json (right):

https://codereview.chromium.org/1273363002/diff/140001/Source/devtools/front_...
Source/devtools/front_end/timeline/module.json:24: "type":
"@WebInspector.ActionDelegate",
On 2015/08/28 at 16:24:45, pfeldman wrote:
> ditto, context is missing.

Done.

[pfeldman, 2015-08-28 17:29:11]

Are you deleting the test?

[pfeldman, 2015-08-28 17:29:42]

Oh, this is stupid code review bug.

[pfeldman, 2015-08-28 17:29:43]

Oh, this is stupid code review bug.

[pfeldman, 2015-08-28 17:30:59]

lgtm

[samli, 2015-08-28 17:52:53]

The CQ bit was checked by samli@chromium.org

[samli, 2015-08-28 17:52:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
paulirish@chromium.org
Link to the patchset: https://codereview.chromium.org/1273363002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-08-28 17:53:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273363002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273363002/160001

[commit-bot: I haz the power, 2015-08-28 18:48:03]

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

[commit-bot: I haz the power, 2015-08-28 18:48:04]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[samli, 2015-08-28 21:10:20]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2015-08-28 21:10:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273363002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273363002/160001

[commit-bot: I haz the power, 2015-08-28 21:47:18]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201428