Password bubble: The bots should run the bubble code.

Password bubble: The bots should run the bubble code.

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

[Mike West, 2014-06-10 09:58:27]

A precursor to "Bubbles for everyone!" that you already LGTM'd.

-mike

[vabr (Chromium), 2014-06-10 10:27:11]

LGTM!

[Mike West, 2014-06-10 10:28:05]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-06-10 10:31:45]

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

[commit-bot: I haz the power, 2014-06-10 20:26:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  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-10 21:51:32]

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

[commit-bot: I haz the power, 2014-06-10 21:51:33]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Mike West, 2014-06-11 13:01:51]

Please take another look, Vaclav. This seems to pass the tests locally; let's
see what the bots say.

-mike

[vabr (Chromium), 2014-06-11 13:58:43]

Thanks, Mike.
LGTM!

Having said that, I'm concerned that the test becomes increasingly confusing.
It already was surprising that it auto-accepted the infobar and consequently
saved the password.
Now, when I forget what I know and just look at the changes, the equivalent of
testing the infobar_shown() is sometimes "bubble shown" and sometimes "bubble
shown & save password".

Ideally, there would be two methods of the test harness, something like
void SavePasswordOnNextPrompt()
and
bool IsSavePasswordPromptVisible()

SavePasswordOnNextPrompt would have the test remember, that the next time it
sees a save password prompt, it should cause the password to be saved.

IsSavePasswordPromptVisible would be almost just
  if (ChromePasswordManagerClient::IsTheHotNewBubbleUIEnabled()) {
    return controller()->PasswordPendingUserDecision();
  else
    return observer.infobar_shown();

Only there is no |observer| in that context, and I would hate to pass it through
the argument.
Instead, observing infobar changes should be decoupled from the
NavigationObserver, and done by the test harness instead (when there is no
bubble).

What I just described is probably better to implement as a separate CL, though.
I'm volunteering to do that as a follow-up, but please let me know if you see
better options than the one described above.

Cheers,
Vaclav

[Mike West, 2014-06-11 14:15:30]

I'd like to just land this as-is in order to clear the way for launching in M37.

That said, the refactoring you've suggested makes sense to me. I'm not sure I
follow the "hate to pass it through the argument" part. Why would it be bad to
pass in the observer if that simplifies the code?

-mike

[vabr (Chromium), 2014-06-11 14:37:48]

Sounds good. Go ahead with this patch, and I'll follow up with the refactoring.

On 2014/06/11 14:15:30, Mike West wrote:
> I'd like to just land this as-is in order to clear the way for launching in
M37.
> 
> That said, the refactoring you've suggested makes sense to me. I'm not sure I
> follow the "hate to pass it through the argument" part. Why would it be bad to
> pass in the observer if that simplifies the code?

Because the observer is only relevant when there is no bubble. Seeing it there
unconditionally as an argument looks counter-intuitive to me. Also, the
NavigationObserver should observe the navigation, observing the state of the
prompt is a different task. I'll think through the details, put together a CL,
and send it to you for review.

Cheers,
Vaclav

> 
> -mike

[Mike West, 2014-06-11 14:47:00]

Brilliant! Looking forward to it...

[Mike West, 2014-06-11 14:47:04]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-06-11 14:48:14]

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

[commit-bot: I haz the power, 2014-06-11 21:27:39]

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

[commit-bot: I haz the power, 2014-06-11 21:48:18]

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

[commit-bot: I haz the power, 2014-06-11 21:48:19]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Mike West, 2014-06-12 07:52:02]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-06-12 07:54:18]

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

[commit-bot: I haz the power, 2014-06-12 12:40:55]

Change committed as 276640