Make FormFetcher a PasswordStoreConsumer

Make FormFetcher a PasswordStoreConsumer

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355
Committed: https://crrev.com/daecd125374ae8d6526b58e56e0429550f78aca4
Cr-Commit-Position: refs/heads/master@{#436571}

[vabr (Chromium), 2016-08-25 14:14:15]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 14:14:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 14:39:44]

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

[commit-bot: I haz the power, 2016-08-25 14:39:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vabr (Chromium), 2016-10-10 13:43:40]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 13:43:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 14:02:58]

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

[commit-bot: I haz the power, 2016-10-10 14:02:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vabr (Chromium), 2016-10-10 14:22:53]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 14:22:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 15:28:27]

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

[commit-bot: I haz the power, 2016-10-10 15:28:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2016-11-11 09:12:50]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 09:13:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 10:00:39]

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

[commit-bot: I haz the power, 2016-11-11 10:00:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2016-11-11 16:11:13]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 16:11:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 17:28:11]

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

[commit-bot: I haz the power, 2016-11-11 17:28:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vabr (Chromium), 2016-11-29 06:29:28]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 06:29:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 06:52:39]

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

[commit-bot: I haz the power, 2016-11-29 06:52:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vabr (Chromium), 2016-11-29 09:03:04]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 09:03:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-11-29 10:00:15]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 10:00:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 12:15:12]

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

[commit-bot: I haz the power, 2016-11-29 12:15:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vabr (Chromium), 2016-11-30 11:08:32]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 11:08:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-11-30 11:11:08]

Description was changed from

==========
[WIP] Make FormFetcher a PasswordStoreConsumer

COMMIT=false
This CL is only uploaded as a backup so far. The tests are being updated.

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355
==========

to

==========
Make FormFetcher a PasswordStoreConsumer

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355
==========

[commit-bot: I haz the power, 2016-11-30 12:06:42]

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

[commit-bot: I haz the power, 2016-11-30 12:06:43]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-11-30 13:24:28]

vabr@chromium.org changed reviewers:
+ vasilii@chromium.org

[vabr (Chromium), 2016-11-30 13:24:30]

Hi Vasilii,

Could you please review? It is not urgent at all.

This is the second-last step in the PasswordFormManager vs. PasswordStore
refactoring. The final one will be sharing the FormFetchers among PFMs for the
same origin.

Cheers,
Vaclav

[vasilii, 2016-12-01 10:24:51]

Just nits, no fundamental concerns.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/fake_form_fetcher.h (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/fake_form_fetcher.h:66: State state_ =
State::NOT_WAITING;
What's the point of assigning NOT_WAITING and then in the constructor assigning
WAITING?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/form_fetcher.h (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher.h:75: virtual void
Update() = 0;
Could you use a better name? For me "Update" means pushing something to the
PasswordStore instead of getting.
"RetrieveMatchingLogins", "GetLogins", "LoadMatchingLogins".

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl.cc (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:63: :
form_digest_(std::move(form_digest)), client_(client) {
I think it would be useful to have std::is_move_constructible somewhere or may
be declare the default move constructor/assignment in PasswordStore::FormDigest.
It's easy to degrade to double copying by accident if somebody adds a destructor
for example..

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:64:
FormFetcherImpl::Update();
What's the reason for such verbosity? Anyway, "Constructors should never call
virtual functions". That's the style guide. I think calling Update explicitly
where needed is better.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:96: state_ =
State::NOT_WAITING;
Why not moving it to the begin of the method and dropping line 123?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:134:
weak_interactions_stats_ = MakeWeakCopies(interactions_stats_);
I think I'll rewrite the code to use std::vector<InteractionsStats>.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:153: state_ =
State::WAITING;
Should it go after "return" in line 160?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl_unittest.cc
(right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:142:
mock_store_ = new testing::NiceMock<MockPasswordStore>();
Will something break if the mock isn't nice?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:300:
EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
This should go before the line which triggers the call.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:312:
ProcessMatches(UnorderedElementsAre(Pointee(form_a)), 0u))
ProcessMatches(_, _) ?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:336: //
below.
One more reason for not calling Update() in the constructor.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:146:
other_possible_username_action_(
What happened to provisionally_saved_form_?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:400:
fetcher.SetNonFederated({&match}, 0u);
I wonder why you don't just call ProcessMatches directly?

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_manager.h:176: // Only for
testing.
Add #if defined(UNIT_TEST)) of ForTesting suffix
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md)

[vabr (Chromium), 2016-12-02 08:52:48]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 08:53:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 10:03:25]

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

[commit-bot: I haz the power, 2016-12-02 10:03:25]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-12-02 14:06:05]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-02 14:11:26]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 14:11:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-02 14:53:53]

Thanks, Vasilii!

Could you please review again?

Cheers,
Vaclav

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/fake_form_fetcher.h (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/fake_form_fetcher.h:66: State state_ =
State::NOT_WAITING;
On 2016/12/01 10:24:50, vasilii wrote:
> What's the point of assigning NOT_WAITING and then in the constructor
assigning
> WAITING?

Good catch, this is likely confusing.

As in the production fetcher, I should not be calling Fetch/Update from the
constructor. At that point, initialising state_ to NOT_WAITING actually makes
sense. So I am keeping the NOT_WAITING, but removing the Fetch/Update from the
constructor.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/form_fetcher.h (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher.h:75: virtual void
Update() = 0;
On 2016/12/01 10:24:50, vasilii wrote:
> Could you use a better name? For me "Update" means pushing something to the
> PasswordStore instead of getting.
> "RetrieveMatchingLogins", "GetLogins", "LoadMatchingLogins".

Sure. What about Fetch()? The verb matches the name of the class. I could add
"MatchingLogins" but the method also fetches the site statistics, so I propose
to keep it short and more general.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl.cc (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:63: :
form_digest_(std::move(form_digest)), client_(client) {
On 2016/12/01 10:24:50, vasilii wrote:
> I think it would be useful to have std::is_move_constructible somewhere or may
> be declare the default move constructor/assignment in
PasswordStore::FormDigest.
> It's easy to degrade to double copying by accident if somebody adds a
destructor
> for example..

Thanks for catching this. I went for defining the move constructor of
FormDigest, because that would prevent this kind of issues also on other
potential places where FormDigest is moved.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:64:
FormFetcherImpl::Update();
On 2016/12/01 10:24:50, vasilii wrote:
> What's the reason for such verbosity? Anyway, "Constructors should never call
> virtual functions". That's the style guide. I think calling Update explicitly
> where needed is better.

Done.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:96: state_ =
State::NOT_WAITING;
On 2016/12/01 10:24:50, vasilii wrote:
> Why not moving it to the begin of the method and dropping line 123?

Good point, all the codepaths through this method exit with NOT_WAITING, and
state_ is not used in the method body here.
Changed.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:134:
weak_interactions_stats_ = MakeWeakCopies(interactions_stats_);
On 2016/12/01 10:24:50, vasilii wrote:
> I think I'll rewrite the code to use std::vector<InteractionsStats>.

Sounds good, and I'm happy to do it. I propose to leave it for a follow-up CL,
though. This one is rather big already.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:153: state_ =
State::WAITING;
On 2016/12/01 10:24:50, vasilii wrote:
> Should it go after "return" in line 160?

Done.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl_unittest.cc
(right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:142:
mock_store_ = new testing::NiceMock<MockPasswordStore>();
On 2016/12/01 10:24:50, vasilii wrote:
> Will something break if the mock isn't nice?

No. Not even a warning. Changed to just MockPasswordStore.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:300:
EXPECT_CALL(*mock_store_, GetLogins(form_digest_, form_fetcher_.get()));
On 2016/12/01 10:24:50, vasilii wrote:
> This should go before the line which triggers the call.

Done.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:312:
ProcessMatches(UnorderedElementsAre(Pointee(form_a)), 0u))
On 2016/12/01 10:24:50, vasilii wrote:
> ProcessMatches(_, _) ?

Done.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:336: //
below.
On 2016/12/01 10:24:50, vasilii wrote:
> One more reason for not calling Update() in the constructor.

Acknowledged.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:146:
other_possible_username_action_(
On 2016/12/01 10:24:50, vasilii wrote:
> What happened to provisionally_saved_form_?

It is a unique_ptr, its default constructor results in the same result as
initialising it with nullptr.

I admit this clean-up is unrelated to this CL. Happy to split it if you want me
to.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_form_manager_unittest.cc:400:
fetcher.SetNonFederated({&match}, 0u);
On 2016/12/01 10:24:50, vasilii wrote:
> I wonder why you don't just call ProcessMatches directly?

SetNonFederated keeps the fetcher status consistent.

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/2263933002/diff/240001/components/password_ma...
components/password_manager/core/browser/password_manager.h:176: // Only for
testing.
On 2016/12/01 10:24:50, vasilii wrote:
> Add #if defined(UNIT_TEST)) of ForTesting suffix
>
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md)

Done.

[commit-bot: I haz the power, 2016-12-02 15:16:26]

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

[commit-bot: I haz the power, 2016-12-02 15:16:28]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-12-05 14:35:58]

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/fake_form_fetcher.cc (right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/fake_form_fetcher.cc:14:
FakeFormFetcher::FakeFormFetcher() {}
= default for consistency.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl_unittest.cc
(right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:146:
testing::Mock::VerifyAndClearExpectations(mock_store_.get());
what exactly expectations do you verify? The object was just created.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:150: if
(mock_store_.get())
How can it be null?

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:163:
base::RunLoop().RunUntilIdle();
VerifyAndClearExpectations?

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:169:
testing::NiceMock<MockConsumer> consumer_;
Seems it should be a MockConsumer or StrictMock<MockConsumer>.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/password_store.cc:80:
origin(std::move(other.origin)) {}
You should just say =default.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/password_store.h:75:
FormDigest(FormDigest&& other);
It seems like you suppressed the default T& T::operator=(const T&&). It should
be added too.

[vabr (Chromium), 2016-12-06 09:11:15]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 09:11:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-06 09:54:07]

Thanks, Vasilii!

I addressed your comments, please take a look.

Cheers,
Vaclav

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/fake_form_fetcher.cc (right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/fake_form_fetcher.cc:14:
FakeFormFetcher::FakeFormFetcher() {}
On 2016/12/05 14:35:57, vasilii wrote:
> = default for consistency.

Done.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/form_fetcher_impl_unittest.cc
(right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:146:
testing::Mock::VerifyAndClearExpectations(mock_store_.get());
On 2016/12/05 14:35:57, vasilii wrote:
> what exactly expectations do you verify? The object was just created.

Good point, this was forgotten since the time when the fetcher called the store
in the constructor.

Removing now.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:150: if
(mock_store_.get())
On 2016/12/05 14:35:57, vasilii wrote:
> How can it be null?

I don't think it can. Removed the "if".

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:163:
base::RunLoop().RunUntilIdle();
On 2016/12/05 14:35:57, vasilii wrote:
> VerifyAndClearExpectations?

Done.

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:169:
testing::NiceMock<MockConsumer> consumer_;
On 2016/12/05 14:35:57, vasilii wrote:
> Seems it should be a MockConsumer or StrictMock<MockConsumer>.

Both work, but for simplicity, I chose the non-modified MockConsumer. (I'm not
sure why I started with the nice one.)

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/password_store.cc:80:
origin(std::move(other.origin)) {}
On 2016/12/05 14:35:57, vasilii wrote:
> You should just say =default.

Using it now, also for the copy constructor.

(I should also have checked the definition of the default move constructor, to
learn that it would call move- and not copy-constructors  of data members
recursively.)

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
File components/password_manager/core/browser/password_store.h (right):

https://codereview.chromium.org/2263933002/diff/300001/components/password_ma...
components/password_manager/core/browser/password_store.h:75:
FormDigest(FormDigest&& other);
On 2016/12/05 14:35:57, vasilii wrote:
> It seems like you suppressed the default T& T::operator=(const T&&). It should
> be added too.

Added both the move-assignment and copy-assignment operators.

[commit-bot: I haz the power, 2016-12-06 10:16:07]

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

[commit-bot: I haz the power, 2016-12-06 10:16:08]

Dry run: This issue passed the CQ dry run.

[vasilii, 2016-12-06 10:38:08]

https://codereview.chromium.org/2263933002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2263933002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_store.cc:76:
PasswordStore::FormDigest& PasswordStore::FormDigest::operator=(
This and below should also be default.

[vabr (Chromium), 2016-12-06 10:41:26]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-12-06 10:41:48]

https://codereview.chromium.org/2263933002/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/2263933002/diff/340001/components/password_ma...
components/password_manager/core/browser/password_store.cc:76:
PasswordStore::FormDigest& PasswordStore::FormDigest::operator=(
On 2016/12/06 10:38:07, vasilii wrote:
> This and below should also be default.

Done.

[vasilii, 2016-12-06 10:53:54]

lgtm

[vabr (Chromium), 2016-12-06 11:04:23]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2016-12-06 11:04:30]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-12-06 11:04:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 11:40:38]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1481022270678090,
"parent_rev": "71a179fd734c64503301522dffc44398acc0ae12", "commit_rev":
"5013e9afa977ee53f62a63e6064fe54648be9d7c"}

[commit-bot: I haz the power, 2016-12-06 11:41:06]

Description was changed from

==========
Make FormFetcher a PasswordStoreConsumer

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355
==========

to

==========
Make FormFetcher a PasswordStoreConsumer

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355
==========

[commit-bot: I haz the power, 2016-12-06 11:41:07]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-12-06 11:43:01]

Description was changed from

==========
Make FormFetcher a PasswordStoreConsumer

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355
==========

to

==========
Make FormFetcher a PasswordStoreConsumer

Instead of getting the new store results from PasswordFormManager, the
FormFetcher will now itself be a PasswordStoreConsumer, relieving the
PasswordFormManager from that duty.

The FormFetcher will still stay owned by a PasswordFormManager after this CL,
changing that is the task of a follow-up CL. However, in tests it will already
be possible to inject a mock version of the FormFetcher. This is used to
simplify and revise some tests, notably password_form_manager_unittest.cc.

BUG=621355

Committed: https://crrev.com/daecd125374ae8d6526b58e56e0429550f78aca4
Cr-Commit-Position: refs/heads/master@{#436571}
==========

[commit-bot: I haz the power, 2016-12-06 11:43:02]

Patchset 19 (id:??) landed as
https://crrev.com/daecd125374ae8d6526b58e56e0429550f78aca4
Cr-Commit-Position: refs/heads/master@{#436571}