Add heuristics to limit showing of new backspace UI bubble.

Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039, 615760
TEST=New "press alt-left to go back" UI should not appear when pressing backspace (to attempt to go back) unless backspace is pressed twice within three seconds.  After the UI is shown 5 times, it should not appear again.
TBR=phajdan.jr

Committed: https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a
Cr-Commit-Position: refs/heads/master@{#399114}

[Peter Kasting, 2016-06-07 07:58:52]

Description was changed from

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
==========

to

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
==========

[Peter Kasting, 2016-06-07 07:58:52]

pkasting@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Peter Kasting, 2016-06-07 08:04:23]

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_command_controller.cc (left):

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller.cc:328: // FALL THROUGH
I know you're super disappointed to see these fallthroughs disappear.

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm
(right):

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:44: if
(!new_back_shortcut_bubble_ || !new_back_shortcut_bubble_->IsVisible()) {
I don't love the fact that this almost exactly duplicates the code in
browser_view.cc.

I thought about placing this logic inside NewBackShortcutBubble itself, but that
seemed somehow wrong -- having callers create it and tell it to show, but it
actually deciding to keep itself hidden until later.

I wasn't sure if there was another good spot to put this code that would be
shared across platforms.  I suppose I could just say "it'll only be in for a few
milestones, who cares".

Your feedback welcome.

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/views/new...
File chrome/browser/ui/views/new_back_shortcut_bubble.cc (left):

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/views/new...
chrome/browser/ui/views/new_back_shortcut_bubble.cc:45: UpdateContent(forward);
Removing this was not strictly necessary, but did let me slightly simplify the
caller logic (to just unconditionally call UpdateContent()).  Of course, I could
have done that without removing this, but then this call would be completely
redundant.

[Matt Giuca, 2016-06-08 01:29:04]

And a minor nit on the CL description (sorry to bring this up again): please
wrap that last line to be consistent with the rest of your description.

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_command_controller.cc (left):

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller.cc:328: // FALL THROUGH
On 2016/06/07 08:04:23, Peter Kasting wrote:
> I know you're super disappointed to see these fallthroughs disappear.

:'(

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm
(right):

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm:44: if
(!new_back_shortcut_bubble_ || !new_back_shortcut_bubble_->IsVisible()) {
On 2016/06/07 08:04:23, Peter Kasting wrote:
> I don't love the fact that this almost exactly duplicates the code in
> browser_view.cc.
> 
> I thought about placing this logic inside NewBackShortcutBubble itself, but
that
> seemed somehow wrong -- having callers create it and tell it to show, but it
> actually deciding to keep itself hidden until later.
> 
> I wasn't sure if there was another good spot to put this code that would be
> shared across platforms.  I suppose I could just say "it'll only be in for a
few
> milestones, who cares".
> 
> Your feedback welcome.

Yeah I went through a similar thought process before I found this comment.
Unfortunate but I think you're right; there isn't a "better" place to put this
logic.

Please be sure to keep this version in sync if you make my suggested changes to
the other copy of this code.

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/views/new...
File chrome/browser/ui/views/new_back_shortcut_bubble.cc (left):

https://codereview.chromium.org/2041293002/diff/1/chrome/browser/ui/views/new...
chrome/browser/ui/views/new_back_shortcut_bubble.cc:45: UpdateContent(forward);
On 2016/06/07 08:04:23, Peter Kasting wrote:
> Removing this was not strictly necessary, but did let me slightly simplify the
> caller logic (to just unconditionally call UpdateContent()).  Of course, I
could
> have done that without removing this, but then this call would be completely
> redundant.

OK. I commented about this in the top of
BrowserView::MaybeShowNewBackShortcutBubble.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:33: void
RegisterBrowserViewProfilePrefs(
Just to be clear (for my own benefit): this means it is a per-profile pref, yes?
So each user will get their own 5 shows. (That's implied by the name but I
thought I'd check.)

Also I assume these are unsynced prefs.

Both of those are fine. (Unsynced is probably a good thing because if a user has
a Windows PC and a Mac PC we can train them on the different shortcuts.)

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
registry->RegisterBooleanPref(prefs::kUseCustomChromeFrame,
nit: Any reason to delete the above line? If anything I think there should be a
blank line here but none between the two Register*Pref calls.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:42:
registry->RegisterIntegerPref(prefs::kBackShortcutBubbleShownCount, 0);
I think we should have separate counters for the Back and Forward shortcuts.
Otherwise you could use up your 5 Back presses and never be taught the Forward
shortcut.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:995: if
(!new_back_shortcut_bubble_ || !new_back_shortcut_bubble_->IsVisible()) {
I think my (weak) preference would be to keep the constructor of NBSB calling
UpdateContent so you don't have to unconditionally call it here. Then you could
do an early exit and not have the bulk of this function indented:

if (new_back_shortcut_bubble_ && new_back_shortcut_bubble_->IsVisible()) {
  new_back_shortcut_bubble_->UpdateContent(forward);
  return;
}
...

Up to you.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1003: constexpr base::TimeDelta
kRepeatWindow = base::TimeDelta::FromSeconds(3);
Can this just be const?

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1004: if
(last_back_shortcut_press_time_.is_null() ||
// Only show the bubble if there have been two successive presses within the
last 3 seconds.

[Peter Kasting, 2016-06-09 19:42:51]

Description was changed from

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
==========

to

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
==========

[Peter Kasting, 2016-06-09 19:49:31]

I rewrapped the rest of the description.

I left the TEST= line unwrapped in case the QA folks have tools that
automatically pull any line beginning with that.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:33: void
RegisterBrowserViewProfilePrefs(
On 2016/06/08 01:29:03, Matt Giuca wrote:
> Just to be clear (for my own benefit): this means it is a per-profile pref,
yes?
> So each user will get their own 5 shows. (That's implied by the name but I
> thought I'd check.)

Yes.

> Also I assume these are unsynced prefs.

It's not synced here, but in thinking about it, I think syncing is the right
thing.

You're correct that users with different platforms will have slightly different
shortcuts, but the differences aren't very significant.  And I suspect the
common case is that people use the same platform.  I'd rather err on the side of
not annoying people.

So, making this syncable.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
registry->RegisterBooleanPref(prefs::kUseCustomChromeFrame,
On 2016/06/08 01:29:03, Matt Giuca wrote:
> nit: Any reason to delete the above line? If anything I think there should be
a
> blank line here but none between the two Register*Pref calls.

The code above is closely tied to this line: it's calculating the value to pass
to this function.  If we didn't need an ifdef, we'd have inlined that code into
this call.

Whereas this call and the one below have no relationship to each other; they're
for different prefs with different values.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:42:
registry->RegisterIntegerPref(prefs::kBackShortcutBubbleShownCount, 0);
On 2016/06/08 01:29:03, Matt Giuca wrote:
> I think we should have separate counters for the Back and Forward shortcuts.
> Otherwise you could use up your 5 Back presses and never be taught the Forward
> shortcut.

My instinct is that one counter for both is the way to go.

I asked Tyler to weigh in.  He also thought one counter would be best, noting
that the forward shortcut is pretty intuitive once one knows the back shortcut.

Neither of us feel violently about this, if you want to try to convince us
further.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:995: if
(!new_back_shortcut_bubble_ || !new_back_shortcut_bubble_->IsVisible()) {
On 2016/06/08 01:29:03, Matt Giuca wrote:
> I think my (weak) preference would be to keep the constructor of NBSB calling
> UpdateContent so you don't have to unconditionally call it here. Then you
could
> do an early exit and not have the bulk of this function indented:
> 
> if (new_back_shortcut_bubble_ && new_back_shortcut_bubble_->IsVisible()) {
>   new_back_shortcut_bubble_->UpdateContent(forward);
>   return;
> }
> ...
> 
> Up to you.

I originally wrote the code that way.  What I didn't like about it was that then
I had multiple function exit points all having to call UpdateContent().  This
seemed error-prone, so I eliminated the early-return so that UpdateContent()
would get called exactly once.  But once I'd done that it made the code simpler
to just do it in every case, at which point I didn't need this call.

It can be done either way, but there was a method to my madness :)

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1003: constexpr base::TimeDelta
kRepeatWindow = base::TimeDelta::FromSeconds(3);
On 2016/06/08 01:29:03, Matt Giuca wrote:
> Can this just be const?

Why?  We should use constexpr for true compile-time constants.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1004: if
(last_back_shortcut_press_time_.is_null() ||
On 2016/06/08 01:29:03, Matt Giuca wrote:
> // Only show the bubble if there have been two successive presses within the
> last 3 seconds.

Done.

[Matt Giuca, 2016-06-10 01:07:05]

lgtm

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:33: void
RegisterBrowserViewProfilePrefs(
On 2016/06/09 19:49:31, Peter Kasting wrote:
> On 2016/06/08 01:29:03, Matt Giuca wrote:
> > Just to be clear (for my own benefit): this means it is a per-profile pref,
> yes?
> > So each user will get their own 5 shows. (That's implied by the name but I
> > thought I'd check.)
> 
> Yes.
> 
> > Also I assume these are unsynced prefs.
> 
> It's not synced here, but in thinking about it, I think syncing is the right
> thing.
> 
> You're correct that users with different platforms will have slightly
different
> shortcuts, but the differences aren't very significant.  And I suspect the
> common case is that people use the same platform.  I'd rather err on the side
of
> not annoying people.
> 
> So, making this syncable.

Acknowledged.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:39:
registry->RegisterBooleanPref(prefs::kUseCustomChromeFrame,
On 2016/06/09 19:49:31, Peter Kasting wrote:
> On 2016/06/08 01:29:03, Matt Giuca wrote:
> > nit: Any reason to delete the above line? If anything I think there should
be
> a
> > blank line here but none between the two Register*Pref calls.
> 
> The code above is closely tied to this line: it's calculating the value to
pass
> to this function.  If we didn't need an ifdef, we'd have inlined that code
into
> this call.
> 
> Whereas this call and the one below have no relationship to each other;
they're
> for different prefs with different values.

Acknowledged.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:42:
registry->RegisterIntegerPref(prefs::kBackShortcutBubbleShownCount, 0);
On 2016/06/09 19:49:31, Peter Kasting wrote:
> On 2016/06/08 01:29:03, Matt Giuca wrote:
> > I think we should have separate counters for the Back and Forward shortcuts.
> > Otherwise you could use up your 5 Back presses and never be taught the
Forward
> > shortcut.
> 
> My instinct is that one counter for both is the way to go.
> 
> I asked Tyler to weigh in.  He also thought one counter would be best, noting
> that the forward shortcut is pretty intuitive once one knows the back
shortcut.
> 
> Neither of us feel violently about this, if you want to try to convince us
> further.

Not particularly :)

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:995: if
(!new_back_shortcut_bubble_ || !new_back_shortcut_bubble_->IsVisible()) {
On 2016/06/09 19:49:31, Peter Kasting wrote:
> On 2016/06/08 01:29:03, Matt Giuca wrote:
> > I think my (weak) preference would be to keep the constructor of NBSB
calling
> > UpdateContent so you don't have to unconditionally call it here. Then you
> could
> > do an early exit and not have the bulk of this function indented:
> > 
> > if (new_back_shortcut_bubble_ && new_back_shortcut_bubble_->IsVisible()) {
> >   new_back_shortcut_bubble_->UpdateContent(forward);
> >   return;
> > }
> > ...
> > 
> > Up to you.
> 
> I originally wrote the code that way.  What I didn't like about it was that
then
> I had multiple function exit points all having to call UpdateContent().  This
> seemed error-prone, so I eliminated the early-return so that UpdateContent()
> would get called exactly once.  But once I'd done that it made the code
simpler
> to just do it in every case, at which point I didn't need this call.
> 
> It can be done either way, but there was a method to my madness :)

Acknowledged.

https://codereview.chromium.org/2041293002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1003: constexpr base::TimeDelta
kRepeatWindow = base::TimeDelta::FromSeconds(3);
On 2016/06/09 19:49:31, Peter Kasting wrote:
> On 2016/06/08 01:29:03, Matt Giuca wrote:
> > Can this just be const?
> 
> Why?  We should use constexpr for true compile-time constants.

Acknowledged.

[Peter Kasting, 2016-06-10 02:02:05]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-06-10 02:03:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041293002/40001

[commit-bot: I haz the power, 2016-06-10 02:12:21]

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

[commit-bot: I haz the power, 2016-06-10 02:12:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Kasting, 2016-06-10 05:44:16]

Description was changed from

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
==========

to

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
TBR=phajdan.jr
==========

[Peter Kasting, 2016-06-10 05:44:16]

pkasting@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Peter Kasting, 2016-06-10 05:44:32]

TBRing phajdan.jr for trivial changes in chrome/test/base/.

[Peter Kasting, 2016-06-10 05:44:45]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-06-10 05:44:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041293002/40001

[commit-bot: I haz the power, 2016-06-10 05:49:18]

Description was changed from

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
TBR=phajdan.jr
==========

to

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
TBR=phajdan.jr
==========

[commit-bot: I haz the power, 2016-06-10 05:49:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-10 05:49:22]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 05:50:55]

Description was changed from

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
TBR=phajdan.jr
==========

to

==========
Add heuristics to limit showing of new backspace UI bubble.

This restricts the bubble to showing 5 times, and only showing when users
trigger backspace twice within three seconds (which likely indicates "hey, why
isn't the browser going back").

The bubble is also immediately hidden (by fading out) if the user successfully
navigates back or forward (by any means) while it's showing.

BUG=610039,615760
TEST=New "press alt-left to go back" UI should not appear when pressing
backspace (to attempt to go back) unless backspace is pressed twice within three
seconds.  After the UI is shown 5 times, it should not appear again.
TBR=phajdan.jr

Committed: https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a
Cr-Commit-Position: refs/heads/master@{#399114}
==========

[commit-bot: I haz the power, 2016-06-10 05:50:56]

Patchset 3 (id:??) landed as
https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a
Cr-Commit-Position: refs/heads/master@{#399114}