PasswordManager browser test: simplify checking for password prompt

PasswordManager browser test: simplify checking for password prompt

Currently, every single test case needs to check whether the infobar or tha password bubble is used, and do different checks accordingly. Even worse, auto-saving behaviour is different for those two, so people modifying existing tests are very likely to have false expectations.

This CL creates a PromptObserver class by decoupling prompt observing from navigation observation. PromptObserver takes care of the differences between infobar and bubble automatically, and provides a clear API to check for the prompt being shown, and to instruct the prompt to save passwords, respectively.

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

[vabr (Chromium), 2014-07-08 15:50:44]

Hi Balázs,

Please take a look.

Mike, highlighting this for you, because it is a refactoring related to bubble
testing, which I promised to you in
https://codereview.chromium.org/322093003/#msg9.

Cheers,
Vaclav

[vabr (Chromium), 2014-07-09 13:27:31]

The failure on Mac seems like a problem, I apparently got the infobar version
wrong (Mac is the only platform with infobars).

I'll try to fix it, and then ask for a review again.

Sorry about that,
Vaclav

[vabr (Chromium), 2014-07-10 14:07:29]

Hi Balázs,

Tests should be fixed now, please take a look.

Thanks!
Vaclav

[engedy, 2014-07-11 18:07:27]

LGTM modulo 2 comments, plus some nits.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:96: // Observes
save password prompt (bubble or infobar) for a specified
nits: Observes *the* save password prompt (bubble or infobar) for a specified
WebContents, keeps track *of whether or not* it is currently shown, and allows
accepting *saving* (?) the password through it.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:103: // Check if
the prompt is being currently shown.
nits: Please use third person in comments. (3 verbs.)

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:104: virtual
bool IsPromptShown() const = 0;
nit: IsShowingPrompt

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:107: // prompt
is no more visible.
nit: no longer visible afterwards.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:115: static
scoped_ptr<PromptObserver> Create(content::WebContents* web_contents);
Nice!

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:120: // Accepts
the password. Can assume that the prompt is currently shown, makes
Nit: the implementation can assume ... but is required to verify that ...

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:132: :
infobar_is_shown_(false),
nit: infobar_is_showing_ or is_infobar_showing_.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:147:
EXPECT_EQ(1u, infobar_service_->infobar_count());
I would assert here instead. (Or your if-solution, if the test will not hang
when no bubble is accepted.)

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:726:
PromptObserver::Create(WebContents()));
Could you please confirm that this really a different WebContents()? Otherwise
we could use the same observers.

[engedy, 2014-07-11 18:09:57]

On 2014/07/11 18:07:27, engedy wrote:
> LGTM modulo 2 comments, plus some nits.
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> File chrome/browser/password_manager/password_manager_browsertest.cc (right):
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:96: //
Observes
> save password prompt (bubble or infobar) for a specified
> nits: Observes *the* save password prompt (bubble or infobar) for a specified
> WebContents, keeps track *of whether or not* it is currently shown, and allows
> accepting *saving* (?) the password through it.
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:103: // Check
if
> the prompt is being currently shown.
> nits: Please use third person in comments. (3 verbs.)
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:104: virtual
> bool IsPromptShown() const = 0;
> nit: IsShowingPrompt
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:107: // prompt
> is no more visible.
> nit: no longer visible afterwards.
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:115: static
> scoped_ptr<PromptObserver> Create(content::WebContents* web_contents);
> Nice!
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:120: //
Accepts
> the password. Can assume that the prompt is currently shown, makes
> Nit: the implementation can assume ... but is required to verify that ...
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:132: :
> infobar_is_shown_(false),
> nit: infobar_is_showing_ or is_infobar_showing_.
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:147:
> EXPECT_EQ(1u, infobar_service_->infobar_count());
> I would assert here instead. (Or your if-solution, if the test will not hang
> when no bubble is accepted.)
> 
>
https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
> chrome/browser/password_manager/password_manager_browsertest.cc:726:
> PromptObserver::Create(WebContents()));
> Could you please confirm that this really a different WebContents()? Otherwise
> we could use the same observers.

To clarify: LGTM modulo (2 comments). I am not sure if the other grouping has
any meaning, but I do not have any first-aid training for cases when the patient
crashes with a parsing error. :-)

[vabr (Chromium), 2014-07-11 18:48:02]

Thanks!

I addressed the comments and will send this to the CQ.

Cheers,
Vaclav

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:96: // Observes
save password prompt (bubble or infobar) for a specified
On 2014/07/11 18:07:26, engedy wrote:
> nits: Observes *the* save password prompt (bubble or infobar) for a specified
> WebContents, keeps track *of whether or not* it is currently shown, and allows
> accepting *saving* (?) the password through it.

Done.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:103: // Check if
the prompt is being currently shown.
On 2014/07/11 18:07:27, engedy wrote:
> nits: Please use third person in comments. (3 verbs.)

Done.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:104: virtual
bool IsPromptShown() const = 0;
On 2014/07/11 18:07:26, engedy wrote:
> nit: IsShowingPrompt

Done.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:107: // prompt
is no more visible.
On 2014/07/11 18:07:27, engedy wrote:
> nit: no longer visible afterwards.

Done.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:115: static
scoped_ptr<PromptObserver> Create(content::WebContents* web_contents);
On 2014/07/11 18:07:26, engedy wrote:
> Nice!

Acknowledged.
(And thanks! :))

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:120: // Accepts
the password. Can assume that the prompt is currently shown, makes
On 2014/07/11 18:07:26, engedy wrote:
> Nit: the implementation can assume ... but is required to verify that ...

Done.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:132: :
infobar_is_shown_(false),
On 2014/07/11 18:07:27, engedy wrote:
> nit: infobar_is_showing_ or is_infobar_showing_.

Done, with a change to infobar_is_being_shown_, which was confirmed by Balázs as
acceptable during an offline chat.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:147:
EXPECT_EQ(1u, infobar_service_->infobar_count());
On 2014/07/11 18:07:26, engedy wrote:
> I would assert here instead. (Or your if-solution, if the test will not hang
> when no bubble is accepted.)

Done.
I don't think it would hang because of this. Usually, it would check the
password store, and complain about the credentials missing.

https://codereview.chromium.org/378643006/diff/80001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:726:
PromptObserver::Create(WebContents()));
On 2014/07/11 18:07:26, engedy wrote:
> Could you please confirm that this really a different WebContents()? Otherwise
> we could use the same observers.

The thing is that I'm not sure -- it should be the same, but I don't know the
exact rules when a WebContents can be swapped out. It is more important to me to
make sure the prompt observer is observing the right one (since I want it to
confirm that no prompt is shown), than to spare one variable.

(After speaking with Balázs offline, I'm leaving this as it is.)

[vabr (Chromium), 2014-07-11 18:52:12]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-07-11 18:53:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/378643006/120001

[commit-bot: I haz the power, 2014-07-11 23:35:35]

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-07-12 01:29:57]

Change committed as 282772