Remove the deprecated Alt+Shift shortcut completely

Remove the deprecated Alt+Shift shortcut completely

The Alt+shift showtcut was disabled in M53, but since then we kept its
deprecation notification. Now it's time to remove it entirely in M58.

BUG=672905
TEST=manually, Alt+Shift should still do nothing, and should show no notification.

Review-Url: https://codereview.chromium.org/2658853003
Cr-Commit-Position: refs/heads/master@{#447013}
Committed: https://chromium.googlesource.com/chromium/src/+/bd98638fcb71657f577d763ae2099f30be6df7d7

[afakhry, 2017-01-26 20:47:38]

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

[commit-bot: I haz the power, 2017-01-26 20:48:13]

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, 2017-01-26 21:39:37]

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

[commit-bot: I haz the power, 2017-01-26 21:39:38]

Dry run: This issue passed the CQ dry run.

[afakhry, 2017-01-26 21:41:27]

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

[afakhry, 2017-01-26 21:41:27]

James, can you please review this CL? Thanks!

[James Cook, 2017-01-26 22:24:52]

Hooray for killing old, inconsistent shortcuts!

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_controller.cc:631: if (delegate_) {
It's not your fault, but this function is deeply nested and hard to read. Can
you try to make it less nest-y, either by pulling out the deprecated accelerator
handling into a separate function or inverting some conditionals?

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_table.cc (left):

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_table.cc:227: NEXT_IME,
"Ash.Accelerators.Deprecated.NextIME",
Does this need to be marked obsolete in histograms.xml?

[afakhry, 2017-01-27 03:33:26]

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

[commit-bot: I haz the power, 2017-01-27 03:34:12]

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

[afakhry, 2017-01-27 03:35:06]

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_controller.cc:631: if (delegate_) {
On 2017/01/26 22:24:52, James Cook wrote:
> It's not your fault, but this function is deeply nested and hard to read. Can
> you try to make it less nest-y, either by pulling out the deprecated
accelerator
> handling into a separate function or inverting some conditionals?

Actually, it is my fault, I'm the one who wrote this in the first place. :D I
should have known better. Thanks for suggesting the clean up.

Please take a look at the refactored code.

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
File ash/common/accelerators/accelerator_table.cc (left):

https://codereview.chromium.org/2658853003/diff/1/ash/common/accelerators/acc...
ash/common/accelerators/accelerator_table.cc:227: NEXT_IME,
"Ash.Accelerators.Deprecated.NextIME",
On 2017/01/26 22:24:52, James Cook wrote:
> Does this need to be marked obsolete in histograms.xml?

Done.

[commit-bot: I haz the power, 2017-01-27 05:17:23]

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

[commit-bot: I haz the power, 2017-01-27 05:17:24]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-01-27 16:40:30]

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_controller.cc:623: if
(!MaybeDeprecatedAcceleratorPressed(action, accelerator))
nit: I wonder if there's a better name for this function. It's hard for me to
interpret the return value. Do you think it would be clearer if that was called
MaybeHandleDeprecatedAccelerator() and invert the return value?  Then the return
value means "I handled it" or "I consumed it". It's just hard for me to reason
about why that function returning false makes this function return false.

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_controller.h:168: // If |accelerator| is a
deprecated accelerator, It performs the appropriate
super nit: It -> it

https://codereview.chromium.org/2658853003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2658853003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1575: +    Obsolete since M58. The
switch to the next IME action has two accelerators:
I think you need to add an <obsolete> tag, e.g.

 <obsolete>
    Deprecated 12/2016 in Issue 672205 with the addition of
    Accessibility.ModeFlag.
  </obsolete>

[afakhry, 2017-01-27 17:27:41]

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_controller.cc:623: if
(!MaybeDeprecatedAcceleratorPressed(action, accelerator))
On 2017/01/27 16:40:30, James Cook wrote:
> nit: I wonder if there's a better name for this function. It's hard for me to
> interpret the return value. Do you think it would be clearer if that was
called
> MaybeHandleDeprecatedAccelerator() and invert the return value?  Then the
return
> value means "I handled it" or "I consumed it". It's just hard for me to reason
> about why that function returning false makes this function return false.

I thought of that too, I didn't want to name it *Handle*DeprecatedAccelerator()
since it doesn't actually handle the accelerator, rather does the pre-handling
(UMA reporting, showing the notification, and determining if the accelerator's
action should be performed).

We can either change it to make it actually handle the accelerator, but then
we'd be handling the accelerator in two places,
or, instead of returning a confusing bool, we return an enum value that tells us
what to do next, whether proceed with handling the accelerator, or stop.

I went for the second solution. Please let me know what you think.

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
File ash/common/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/2658853003/diff/20001/ash/common/accelerators...
ash/common/accelerators/accelerator_controller.h:168: // If |accelerator| is a
deprecated accelerator, It performs the appropriate
On 2017/01/27 16:40:30, James Cook wrote:
> super nit: It -> it

Done.

https://codereview.chromium.org/2658853003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2658853003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1575: +    Obsolete since M58. The
switch to the next IME action has two accelerators:
On 2017/01/27 16:40:30, James Cook wrote:
> I think you need to add an <obsolete> tag, e.g.
> 
>  <obsolete>
>     Deprecated 12/2016 in Issue 672205 with the addition of
>     Accessibility.ModeFlag.
>   </obsolete>

Done.

[James Cook, 2017-01-27 18:55:37]

I like it! LGTM

[afakhry, 2017-01-27 19:59:47]

afakhry@chromium.org changed reviewers:
+ mpearson@chromium.org

[afakhry, 2017-01-27 19:59:47]

+mpearson@ for histograms.xml

[Mark P, 2017-01-28 06:17:18]

histograms.xml lgtm rubber stamp

[afakhry, 2017-01-30 16:55:20]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2017-01-30 16:55:34]

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, 2017-01-30 18:07:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485795320624760,
"parent_rev": "224e43ce1ca4f6ca6f2cd8d677b8684f4b7c2351", "commit_rev":
"bd98638fcb71657f577d763ae2099f30be6df7d7"}

[commit-bot: I haz the power, 2017-01-30 18:07:42]

Description was changed from

==========
Remove the deprecated Alt+Shift shortcut completely

The Alt+shift showtcut was disabled in M53, but since then we kept its
deprecation notification. Now it's time to remove it entirely in M58.

BUG=672905
TEST=manually, Alt+Shift should still do nothing, and should show no
notification.
==========

to

==========
Remove the deprecated Alt+Shift shortcut completely

The Alt+shift showtcut was disabled in M53, but since then we kept its
deprecation notification. Now it's time to remove it entirely in M58.

BUG=672905
TEST=manually, Alt+Shift should still do nothing, and should show no
notification.

Review-Url: https://codereview.chromium.org/2658853003
Cr-Commit-Position: refs/heads/master@{#447013}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd98638fcb71657f577d763ae209...
==========

[commit-bot: I haz the power, 2017-01-30 18:07:44]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/bd98638fcb71657f577d763ae209...