Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It also promotes cmd-[ over cmd-left on Mac.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit backspace twice while outside a textfield to trigger the "Use alt-left to go back" UI.  It should say "Left arrow" instead of "<-".

Committed: https://crrev.com/00d3a368e97975b82f4dc4f483f6e8c6e85331c0
Cr-Commit-Position: refs/heads/master@{#439747}

[Peter Kasting, 2016-11-29 00:01:50]

pkasting@chromium.org changed reviewers:
+ shrike@chromium.org, sky@chromium.org

[Peter Kasting, 2016-11-29 00:01:50]

shrike: Mac
sky: Rest

Jayson, can you check what shortcut this actually chooses to show on Mac?  I'm
not sure whether this will choose cmd-[ or not.  If not, then we wouldn't use
that in other UI for "back" elsewhere either, and should fix that more centrally
(maybe that would involve changing the order of which shortcut is listed first
in a table somewhere?).

[sky, 2016-11-29 00:14:54]

https://codereview.chromium.org/2540523003/diff/1/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2540523003/diff/1/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:167: virtual void
MaybeShowNewBackShortcutBubble(int command_id) = 0;
I like the boolean here (or use an enum) as it makes it clear there are only two
possibilities. Using a command_id means any int can be supplied, which isn't
what you want. How about leaving this (or change to enum)?

[shrike, 2016-11-29 00:32:15]

On 2016/11/29 00:01:50, Peter Kasting wrote:
> shrike: Mac
> sky: Rest
> 
> Jayson, can you check what shortcut this actually chooses to show on Mac?  I'm
> not sure whether this will choose cmd-[ or not.  If not, then we wouldn't use
> that in other UI for "back" elsewhere either, and should fix that more
centrally
> (maybe that would involve changing the order of which shortcut is listed first
> in a table somewhere?).

Hello pkasting@,

With your change, Chrome is showing "Cmd+[", which I believe is the desired
outcome?

[Peter Kasting, 2016-11-29 00:58:33]

On 2016/11/29 00:32:15, shrike wrote:
> On 2016/11/29 00:01:50, Peter Kasting wrote:
> > shrike: Mac
> > sky: Rest
> > 
> > Jayson, can you check what shortcut this actually chooses to show on Mac? 
I'm
> > not sure whether this will choose cmd-[ or not.  If not, then we wouldn't
use
> > that in other UI for "back" elsewhere either, and should fix that more
> centrally
> > (maybe that would involve changing the order of which shortcut is listed
first
> > in a table somewhere?).
> 
> Hello pkasting@,
> 
> With your change, Chrome is showing "Cmd+[", which I believe is the desired
> outcome?

Yep, that's what we want.  Yay!

[Peter Kasting, 2016-11-29 00:58:38]

https://codereview.chromium.org/2540523003/diff/1/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2540523003/diff/1/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:167: virtual void
MaybeShowNewBackShortcutBubble(int command_id) = 0;
On 2016/11/29 00:14:54, sky wrote:
> I like the boolean here (or use an enum) as it makes it clear there are only
two
> possibilities. Using a command_id means any int can be supplied, which isn't
> what you want. How about leaving this (or change to enum)?

I thought about this some when making the change, but I thought it was kinda
better for the new_back_shortcut_bubble.cc code to be agnostic about the actual
commands.  It will actually work correctly with any command ID now, so if we
want to use it for some kind of more general case we can.

I dunno, it's not a huge deal either way, it just seemed slightly better?  I can
change it back if you feel strongly.

[sky, 2016-11-29 04:22:57]

LGTM with boolean

https://codereview.chromium.org/2540523003/diff/1/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/2540523003/diff/1/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:167: virtual void
MaybeShowNewBackShortcutBubble(int command_id) = 0;
On 2016/11/29 00:58:38, Peter Kasting wrote:
> On 2016/11/29 00:14:54, sky wrote:
> > I like the boolean here (or use an enum) as it makes it clear there are only
> two
> > possibilities. Using a command_id means any int can be supplied, which isn't
> > what you want. How about leaving this (or change to enum)?
> 
> I thought about this some when making the change, but I thought it was kinda
> better for the new_back_shortcut_bubble.cc code to be agnostic about the
actual
> commands.  It will actually work correctly with any command ID now, so if we
> want to use it for some kind of more general case we can.

It's certainly more general and it only works right now because the
implementation treats it as a boolean. By that I mean you have a conditional
checking for one command id and assuming everything is false. If someone called
this with the wrong thing they wouldn't know they were using it incorrectly.

> I dunno, it's not a huge deal either way, it just seemed slightly better?  I
can
> change it back if you feel strongly.

I prefer the boolean.

[Peter Kasting, 2016-12-20 06:16:11]

Description was changed from

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It may also choose to promote cmd-[ over cmd-left on Mac,
not sure.

This also plumbs the signal about which command to use as a command ID instead
of a bool, which is slightly simpler at the callee and slightly easier to read
at the caller.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".
==========

to

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It may also choose to promote cmd-[ over cmd-left on Mac,
not sure.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".
==========

[Peter Kasting, 2016-12-20 06:16:26]

Description was changed from

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It may also choose to promote cmd-[ over cmd-left on Mac,
not sure.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".
==========

to

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It also promotes cmd-[ over cmd-left on Mac.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".
==========

[Peter Kasting, 2016-12-20 06:16:32]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-12-20 06:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2540523003/#ps20001
(title: "Remove refactoring")

[commit-bot: I haz the power, 2016-12-20 06:16:51]

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-12-20 08:13:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482214592033150,
"parent_rev": "b3a00f980df7999da13d986893e91c69abc28a3b", "commit_rev":
"927351d738bbc573f50c1f224674a1eece47a6d8"}

[commit-bot: I haz the power, 2016-12-20 08:13:32]

Description was changed from

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It also promotes cmd-[ over cmd-left on Mac.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".
==========

to

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It also promotes cmd-[ over cmd-left on Mac.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".

Review-Url: https://codereview.chromium.org/2540523003
==========

[commit-bot: I haz the power, 2016-12-20 08:13:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-20 08:15:38]

Description was changed from

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It also promotes cmd-[ over cmd-left on Mac.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".

Review-Url: https://codereview.chromium.org/2540523003
==========

to

==========
Use standardized accelerator-to-shortcut code for new backspace shortcut UI.

This changes the left and right arrow symbols into text (e.g. "left arrow"),
reducing ambiguity.  It also promotes cmd-[ over cmd-left on Mac.

BUG=637547
TEST=On a new profile, have a page in your "back" session history, then hit
backspace twice while outside a textfield to trigger the "Use alt-left to go
back" UI.  It should say "Left arrow" instead of "<-".

Committed: https://crrev.com/00d3a368e97975b82f4dc4f483f6e8c6e85331c0
Cr-Commit-Position: refs/heads/master@{#439747}
==========

[commit-bot: I haz the power, 2016-12-20 08:15:39]

Patchset 2 (id:??) landed as
https://crrev.com/00d3a368e97975b82f4dc4f483f6e8c6e85331c0
Cr-Commit-Position: refs/heads/master@{#439747}