Password bubble: Ensure that the bubble stays open for at least 1 second.

Password bubble: Ensure that the bubble stays open for at least 1 second.

Upon navigation, we throw away everything in the password bubble. This
is intentional, but a poor experience on sites that do strange things on
login. Like `accounts.google.com` for example.

This patch ensures that we don't clear the bubble state if a navigation
occurs within a second. A future patch could extend this to not clearing
the bubble state if navigations occur within the same eTLD+1.

BUG=383296
TBR=awong@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278727

[Mike West, 2014-06-16 13:43:27]

Hey Vaclav!

Would you mind taking a look at this while I'm thinking about how to test it?

-mike

[vabr (Chromium), 2014-06-16 15:02:00]

Looks good so far, but will review once again with the tests.

Thanks!
Vaclav

https://codereview.chromium.org/338103003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/338103003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:176: if (timer_ &&
timer_->Elapsed() < base::TimeDelta::FromSeconds(1))
optional nit: You could make the 1 a named global constant; maybe consider
joining that with kMinimumPromptDisplayTime?

https://codereview.chromium.org/338103003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:180: if
(details.is_in_page)
optional nit: As the cheapest check, this should probably be first.

[Mike West, 2014-06-17 08:38:10]

Vaclav, mind taking another look?

-mike

[vabr (Chromium), 2014-06-17 09:46:25]

Thanks Mike. Still LGTM, but with a couple of comments:

(1) Did you think about user testing for what is the ideal magical number for
the delay? You use 1s now, but maybe that's too short for the average connection
speed?

(2) You might get asked to move the mock timer into base/test, but I'll leave
that to the rightful OWNERS of that code.

+ two comments in the code.

Thanks!
Vaclav

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h (right):

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:56: virtual
void DidNavigateMainFrame(
nit: Instead of manually declaring the parent's method here, and calling it in
the *.cc file, just replace the declaration with:
using ManagePasswordsUIController::DidNavigateMainFrame;

That will have the same effect, but be shorted and more readable.

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:114: //
Fake-navigate within a second, then see what happens!
nit: Explain why is one second a magical constant (and where to find it in the
production code), and why do we expect from navigations after shorter delays to
keep the bubble open.

Ditto below for the longer-delay variant.

[Mike West, 2014-06-17 10:49:35]

ajwong@: Would you mind taking a look at the base/timer/* changes?

On 2014/06/17 09:46:25, vabr (Chromium) wrote:
> (1) Did you think about user testing for what is the ideal magical number for
> the delay? You use 1s now, but maybe that's too short for the average
connection
> speed?

There's not a ton of harm in increasing this timeout, but certainly potential
for confusion the longer we make it. I could imagine clicking an external link
just as the timer is executing, meaning that I'd get the bubble popping up on an
unrelated site. *shrug*

I'm not really sure what kind of user testing we could do here other than adding
UMA statistics around navigation timings.

> (2) You might get asked to move the mock timer into base/test, but I'll leave
> that to the rightful OWNERS of that code.

I thought about that, but followed the example of base/timer/mock_timer.*. WDYT,
Albert?

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h (right):

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:56: virtual
void DidNavigateMainFrame(
On 2014/06/17 09:46:25, vabr (Chromium) wrote:
> nit: Instead of manually declaring the parent's method here, and calling it in
> the *.cc file, just replace the declaration with:
> using ManagePasswordsUIController::DidNavigateMainFrame;
> 
> That will have the same effect, but be shorted and more readable.

C++ is an amazing language. I'm not sure that's a positive statement. :)

Thanks!

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:114: //
Fake-navigate within a second, then see what happens!
On 2014/06/17 09:46:25, vabr (Chromium) wrote:
> nit: Explain why is one second a magical constant (and where to find it in the
> production code), and why do we expect from navigations after shorter delays
to
> keep the bubble open.
> 
> Ditto below for the longer-delay variant.

Done.

[vabr (Chromium), 2014-06-17 14:53:57]

Thanks, Mike, still LGTM.

On 2014/06/17 10:49:35, Mike West wrote:
> ajwong@: Would you mind taking a look at the base/timer/* changes?
> 
> On 2014/06/17 09:46:25, vabr (Chromium) wrote:
> > (1) Did you think about user testing for what is the ideal magical number
for
> > the delay? You use 1s now, but maybe that's too short for the average
> connection
> > speed?
> 
> There's not a ton of harm in increasing this timeout, but certainly potential
> for confusion the longer we make it. I could imagine clicking an external link
> just as the timer is executing, meaning that I'd get the bubble popping up on
an
> unrelated site. *shrug*
> 
> I'm not really sure what kind of user testing we could do here other than
adding
> UMA statistics around navigation timings.

There is already some UMA around this for the infobar, look for
kMinimumPromptDisplayTime in
chrome/browser/password_manager/save_password_infobar_delegate.cc.
Not necessarily in this CL, but we should probably extend that to the bubble and
make it compatible with the time check you introduced (i.e., set
kMinimumPromptDisplayTime to 1s, and report if a prompt persisted due to
navigation being shorter than that). That way if a user reports a problem with
the bubble disappearing, we can ask them to have a look at about:histograms and
see what happened.

> 
> > (2) You might get asked to move the mock timer into base/test, but I'll
leave
> > that to the rightful OWNERS of that code.
> 
> I thought about that, but followed the example of base/timer/mock_timer.*.
WDYT,
> Albert?
> 
>
https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
> File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h
(right):
> 
>
https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
> chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:56: virtual
> void DidNavigateMainFrame(
> On 2014/06/17 09:46:25, vabr (Chromium) wrote:
> > nit: Instead of manually declaring the parent's method here, and calling it
in
> > the *.cc file, just replace the declaration with:
> > using ManagePasswordsUIController::DidNavigateMainFrame;
> > 
> > That will have the same effect, but be shorted and more readable.
> 
> C++ is an amazing language. I'm not sure that's a positive statement. :)
> 
> Thanks!
> 
>
https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
> File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/338103003/diff/60001/chrome/browser/ui/passwo...
> chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:114: //
> Fake-navigate within a second, then see what happens!
> On 2014/06/17 09:46:25, vabr (Chromium) wrote:
> > nit: Explain why is one second a magical constant (and where to find it in
the
> > production code), and why do we expect from navigations after shorter delays
> to
> > keep the bubble open.
> > 
> > Ditto below for the longer-delay variant.
> 
> Done.

[Mike West, 2014-06-20 08:26:57]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-06-20 08:30:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/338103003/100001

[Mike West, 2014-06-20 08:41:29]

Moving MockElapsedTimer into my test file, as it's the only caller. If another
caller springs up in the future, I'll extract it into base/* at that point.

TBRing awong@ for the trivial changes to base/timer/elapsed_timer.h to enable
the mock implementation.

[Mike West, 2014-06-20 08:41:32]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2014-06-20 08:41:43]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-06-20 08:42:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/338103003/100001

[commit-bot: I haz the power, 2014-06-20 14:00:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-20 17:00:13]

Change committed as 278727