Refactor of credential manager dispatcher unittests.

Refactor of credential manager dispatcher unittests.

BUG=450583
R=vabr@chromium.org

Committed: https://crrev.com/72fdb6ab836a1243792bc7ab0bbc390307354cec
Cr-Commit-Position: refs/heads/master@{#322758}

[vabr (Chromium), 2015-03-30 07:56:43]

Hi Tanja,

A quick pre-review, until I disappear for a training. I'll have a further look
once I'm back (within a hour).

Cheers,
Vaclav

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
File
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:52:
MockPasswordManagerClient(password_manager::PasswordStore* store)
optional nit: Separate the constructor from the MOCK* block with a blank line.

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:63:
PromptUserToSavePasswordPtr(manager.release(), type);
This looks fishy, why do you pass on the previously saved manager, and save the
current one without passing? Are you sure you did not want to call
manager_.release()?

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:231:
_, password_manager::CredentialSourceType::CREDENTIAL_SOURCE_API))
Isn't this leaking the manager? (Also on other places.)

[melandory, 2015-03-30 08:23:33]

Patchset #2 (id:20001) has been deleted

[melandory, 2015-03-30 08:40:23]

Ready for review again. Thanks!

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
File
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:52:
MockPasswordManagerClient(password_manager::PasswordStore* store)
On 2015/03/30 07:56:43, vabr (Chromium) wrote:
> optional nit: Separate the constructor from the MOCK* block with a blank line.

Done.

https://codereview.chromium.org/1044643002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:63:
PromptUserToSavePasswordPtr(manager.release(), type);
On 2015/03/30 07:56:43, vabr (Chromium) wrote:
> This looks fishy, why do you pass on the previously saved manager, and save
the
> current one without passing? Are you sure you did not want to call
> manager_.release()?

Done.

[vabr (Chromium), 2015-03-30 09:02:31]

LGTM with nits.
Thanks for improving the tests!

Cheers,
Vaclav

https://codereview.chromium.org/1044643002/diff/40001/components/password_man...
File
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1044643002/diff/40001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:343:
testing::_,
nit: testing::_ -> _

https://codereview.chromium.org/1044643002/diff/40001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:527:
EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _, _))
If I understand GMock correctly, the .Times(0) expectation from lines 515-518 is
still in effect through this whole test. So the lines 527-530 seem unnecessary.

[melandory, 2015-03-30 09:22:42]

https://codereview.chromium.org/1044643002/diff/40001/components/password_man...
File
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc
(right):

https://codereview.chromium.org/1044643002/diff/40001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:343:
testing::_,
On 2015/03/30 09:02:30, vabr (Chromium) wrote:
> nit: testing::_ -> _

Done.

https://codereview.chromium.org/1044643002/diff/40001/components/password_man...
components/password_manager/content/browser/credential_manager_dispatcher_unittest.cc:527:
EXPECT_CALL(*client_, PromptUserToChooseCredentialsPtr(_, _, _, _))
On 2015/03/30 09:02:30, vabr (Chromium) wrote:
> If I understand GMock correctly, the .Times(0) expectation from lines 515-518
is
> still in effect through this whole test. So the lines 527-530 seem
unnecessary.

Done.

[melandory, 2015-03-30 09:22:46]

The CQ bit was checked by melandory@chromium.org

[melandory, 2015-03-30 09:22:46]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1044643002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-30 09:22:57]

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

[commit-bot: I haz the power, 2015-03-30 09:43:42]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-30 09:44:47]

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