ash/chromeos: Add shift-alt-p shortcut to open palette.

ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=641555
Committed: https://crrev.com/a0ff8862ad36a4a40bc4498397c18cd57a812e28
Cr-Commit-Position: refs/heads/master@{#414831}

[jdufault, 2016-08-25 01:11:35]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 01:12:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 01:44:57]

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

[commit-bot: I haz the power, 2016-08-25 01:44:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdufault, 2016-08-25 20:00:30]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 20:01:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 20:24:07]

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

[commit-bot: I haz the power, 2016-08-25 20:24:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdufault, 2016-08-25 21:08:52]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 21:09:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 21:27:39]

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

[commit-bot: I haz the power, 2016-08-25 21:27:40]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdufault, 2016-08-25 22:16:59]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 22:17:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-08-25 22:39:29]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[jdufault, 2016-08-25 22:39:38]

Patchset #1 (id:1) has been deleted

[jdufault, 2016-08-25 22:39:45]

Patchset #1 (id:20001) has been deleted

[jdufault, 2016-08-25 22:39:56]

Patchset #1 (id:40001) has been deleted

[jdufault, 2016-08-25 22:40:02]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-08-25 22:40:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-08-25 23:48:36]

Description was changed from

==========
Add shift-alt-p to open palette.

BUG=
==========

to

==========
Add shift-alt-p shortcut to open palette.

BUG=616112
==========

[jdufault, 2016-08-25 23:49:48]

jdufault@chromium.org changed reviewers:
+ jamescook@chromium.org

[jdufault, 2016-08-25 23:52:27]

jamescook@ PTAL. This is an accessibility patch that I should be able to merge
into m54, so feel free to look when you have time tomorrow or next week.

[commit-bot: I haz the power, 2016-08-26 00:03:16]

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

[commit-bot: I haz the power, 2016-08-26 00:03:17]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-08-26 01:05:51]

Also, please add a TEST= line to the CL description, even if it is "none" or
"manual"

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:71: #include
"ash/common/palette_delegate.h"
Since this code is in ash/common you should be able to put code from this file
in ash/common/accelerators/accelerator_controller.cc

You'll want to use WmShell::Get()->GetRootWindowForNewWindows() instead of
GetTargetRootWindow().

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:332: return
status_area_widget && ash::IsPaletteFeatureEnabled() &&
nit: StatusAreaWidget is created in the SHelfWidget constructor, so there should
always be a status_area_widget, I don't think you need the if() condition.

nit: no ash::

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:342: if
(status_area_widget) {
ditto

https://codereview.chromium.org/2276233002/diff/80001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2276233002/diff/80001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:720: Accelerator that opens up the stylus
palette bubble.
How about something like "Number of times the user opened the stylus palette
bubble using a keyboard shortcut" or similar.

[jdufault, 2016-08-26 02:14:42]

Description was changed from

==========
Add shift-alt-p shortcut to open palette.

BUG=616112
==========

to

==========
Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=616112
==========

[jdufault, 2016-08-26 02:44:40]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[jdufault, 2016-08-26 02:44:56]

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:71: #include
"ash/common/palette_delegate.h"
On 2016/08/26 01:05:51, James Cook wrote:
> Since this code is in ash/common you should be able to put code from this file
> in ash/common/accelerators/accelerator_controller.cc
> 
> You'll want to use WmShell::Get()->GetRootWindowForNewWindows() instead of
> GetTargetRootWindow().

Done.

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:332: return
status_area_widget && ash::IsPaletteFeatureEnabled() &&
On 2016/08/26 01:05:51, James Cook wrote:
> nit: StatusAreaWidget is created in the SHelfWidget constructor, so there
should
> always be a status_area_widget, I don't think you need the if() condition.
> 
> nit: no ash::

Done.

https://codereview.chromium.org/2276233002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:342: if
(status_area_widget) {
On 2016/08/26 01:05:51, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2276233002/diff/80001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/2276233002/diff/80001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:720: Accelerator that opens up the stylus
palette bubble.
On 2016/08/26 01:05:51, James Cook wrote:
> How about something like "Number of times the user opened the stylus palette
> bubble using a keyboard shortcut" or similar.

Done.

[commit-bot: I haz the power, 2016-08-26 02:45:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 05:24:47]

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

[commit-bot: I haz the power, 2016-08-26 05:24:48]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-08-26 15:49:25]

LGTM

https://codereview.chromium.org/2276233002/diff/100001/ash/mus/accelerators/a...
File ash/mus/accelerators/accelerator_controller_delegate_mus.cc (right):

https://codereview.chromium.org/2276233002/diff/100001/ash/mus/accelerators/a...
ash/mus/accelerators/accelerator_controller_delegate_mus.cc:63: case
SHOW_STYLUS_TOOLS:
nit: Remove this one. Per the comment above it's only for ones that haven't been
ported to the Wm* world.

[James Cook, 2016-08-26 15:50:29]

On 2016/08/26 15:49:25, James Cook wrote:
> LGTM
> 
>
https://codereview.chromium.org/2276233002/diff/100001/ash/mus/accelerators/a...
> File ash/mus/accelerators/accelerator_controller_delegate_mus.cc (right):
> 
>
https://codereview.chromium.org/2276233002/diff/100001/ash/mus/accelerators/a...
> ash/mus/accelerators/accelerator_controller_delegate_mus.cc:63: case
> SHOW_STYLUS_TOOLS:
> nit: Remove this one. Per the comment above it's only for ones that haven't
been
> ported to the Wm* world.

Also, I recommend adding either "ash", "chromeos", or both to the CL
description. It really helps when less experienced developers are trying to
figure out which CL to blame when something blames. The string "chromeos" can
save you from having Windows crashes assigned to you. :-)

[jdufault, 2016-08-26 18:45:05]

Description was changed from

==========
Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=616112
==========

to

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=616112
==========

[jdufault, 2016-08-26 18:46:41]

jdufault@chromium.org changed reviewers:
+ isherman@chromium.org, stevenjb@chromium.org

[jdufault, 2016-08-26 18:48:14]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[jdufault, 2016-08-26 18:48:42]

jdufault@chromium.org changed reviewers:
- isherman@chromium.org, stevenjb@chromium.org

[commit-bot: I haz the power, 2016-08-26 18:48:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdufault, 2016-08-26 18:49:10]

> Also, I recommend adding either "ash", "chromeos", or both to the CL
> description. It really helps when less experienced developers are trying to
> figure out which CL to blame when something blames. The string "chromeos" can
> save you from having Windows crashes assigned to you. :-)

Done, thanks.

https://codereview.chromium.org/2276233002/diff/100001/ash/mus/accelerators/a...
File ash/mus/accelerators/accelerator_controller_delegate_mus.cc (right):

https://codereview.chromium.org/2276233002/diff/100001/ash/mus/accelerators/a...
ash/mus/accelerators/accelerator_controller_delegate_mus.cc:63: case
SHOW_STYLUS_TOOLS:
On 2016/08/26 15:49:25, James Cook wrote:
> nit: Remove this one. Per the comment above it's only for ones that haven't
been
> ported to the Wm* world.

Ah, there is a already comment at the top of the switch statement to this efect
that I missed.

[jdufault, 2016-08-26 18:49:32]

jdufault@chromium.org changed reviewers:
+ isherman@chromium.org, stevenjb@chromium.org

[jdufault, 2016-08-26 18:50:24]

stevenjb@ PTAL at chrome/browser/ui/ash/palette_delegate_chromeos*
isherman@ PTAL at tools/metrics/actions/actions.xml

Thanks!

[commit-bot: I haz the power, 2016-08-26 19:35:30]

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

[commit-bot: I haz the power, 2016-08-26 19:35:31]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-08-26 20:54:31]

https://codereview.chromium.org/2276233002/diff/120001/ash/common/accelerator...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2276233002/diff/120001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:357: return
IsPaletteFeatureEnabled() &&
Can we just test whether palette_delegate() is null?

https://codereview.chromium.org/2276233002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/palette/palette_tray.h (right):

https://codereview.chromium.org/2276233002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/palette/palette_tray.h:61: bool OpenBubble();
Document, and possibly rename to ShowPalette() or somesuch; OpenBubble seems a
bit to implementation specific.

[jdufault, 2016-08-26 21:33:13]

https://codereview.chromium.org/2276233002/diff/120001/ash/common/accelerator...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2276233002/diff/120001/ash/common/accelerator...
ash/common/accelerators/accelerator_controller.cc:357: return
IsPaletteFeatureEnabled() &&
On 2016/08/26 20:54:31, stevenjb wrote:
> Can we just test whether palette_delegate() is null?

Ah, good catch. I've made it so that the palette delegate is only constructed
when the palette feature is enabled.

https://codereview.chromium.org/2276233002/diff/120001/ash/common/system/chro...
File ash/common/system/chromeos/palette/palette_tray.h (right):

https://codereview.chromium.org/2276233002/diff/120001/ash/common/system/chro...
ash/common/system/chromeos/palette/palette_tray.h:61: bool OpenBubble();
On 2016/08/26 20:54:31, stevenjb wrote:
> Document, and possibly rename to ShowPalette() or somesuch; OpenBubble seems a
> bit to implementation specific.

Done.

[jdufault, 2016-08-26 21:33:18]

The CQ bit was checked by jdufault@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 21:34:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2016-08-26 21:34:53]

actions.xml lgtm

[jdufault, 2016-08-26 21:41:30]

Description was changed from

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=616112
==========

to

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=641555
==========

[stevenjb, 2016-08-26 21:52:31]

lgtm

[jdufault, 2016-08-26 21:52:50]

The CQ bit was unchecked by jdufault@chromium.org

[jdufault, 2016-08-26 21:52:50]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-08-26 21:52:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2276233002/#ps140001
(title: "Address comments")

[commit-bot: I haz the power, 2016-08-26 21:53:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 22:22:33]

Description was changed from

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=641555
==========

to

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=641555
==========

[commit-bot: I haz the power, 2016-08-26 22:22:35]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-08-26 22:25:34]

Description was changed from

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=641555
==========

to

==========
ash/chromeos: Add shift-alt-p shortcut to open palette.

TEST=manual
BUG=641555

Committed: https://crrev.com/a0ff8862ad36a4a40bc4498397c18cd57a812e28
Cr-Commit-Position: refs/heads/master@{#414831}
==========

[commit-bot: I haz the power, 2016-08-26 22:25:36]

Patchset 4 (id:??) landed as
https://crrev.com/a0ff8862ad36a4a40bc4498397c18cd57a812e28
Cr-Commit-Position: refs/heads/master@{#414831}