Introduce password_manager::FormFetcher

Introduce password_manager::FormFetcher

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

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

[vabr (Chromium), 2016-08-20 00:03:32]

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

[commit-bot: I haz the power, 2016-08-20 00:04:09]

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-20 00:17:54]

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

[commit-bot: I haz the power, 2016-08-20 00:17:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[vabr (Chromium), 2016-08-20 15:40:19]

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

[commit-bot: I haz the power, 2016-08-20 15:40:26]

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-20 17:10:47]

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

[commit-bot: I haz the power, 2016-08-20 17:10:48]

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-08-20 19:06:58]

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

[commit-bot: I haz the power, 2016-08-20 19:07:11]

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-08-20 19:23:10]

Description was changed from

==========
Introduce password_manager::FormFetcher

COMMIT=false
This is still a WIP. TODOs include tests for FormFetcher and TODO(vabr) present
in the diff.

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

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

to

==========
Introduce password_manager::FormFetcher

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

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

[commit-bot: I haz the power, 2016-08-20 19:50:03]

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

[commit-bot: I haz the power, 2016-08-20 19:50:04]

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-08-21 11:15:17]

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

[commit-bot: I haz the power, 2016-08-21 11:15:25]

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-08-21 11:55:48]

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

[commit-bot: I haz the power, 2016-08-21 11:55:54]

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-21 12:53:53]

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

[commit-bot: I haz the power, 2016-08-21 12:53:54]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-08-22 12:38:59]

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

[commit-bot: I haz the power, 2016-08-22 12:39:14]

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-22 12:41:34]

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

[commit-bot: I haz the power, 2016-08-22 12:41:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vabr (Chromium), 2016-08-22 13:45:58]

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

[commit-bot: I haz the power, 2016-08-22 13:46:09]

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-22 14:56:55]

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

[commit-bot: I haz the power, 2016-08-22 14:56:56]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-08-22 15:27:17]

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

[commit-bot: I haz the power, 2016-08-22 15:27:50]

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-22 17:03:04]

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

[commit-bot: I haz the power, 2016-08-22 17:03:05]

Dry run: This issue passed the CQ dry run.

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

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

[commit-bot: I haz the power, 2016-08-23 08:40:23]

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-23 10:51:22]

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

[commit-bot: I haz the power, 2016-08-23 10:51:23]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-08-23 11:00:55]

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

[vabr (Chromium), 2016-08-23 11:00:56]

Hi Vasilii,

While Vadym is reviewing the previous CL (dependency of this one), perhaps you
could start looking at this one?

I know it is a huge one again, sorry about it. Most of the touched files are
just mechanical changes, the core is the introduction of FormFetcher.

If you are concerned that FormFetcher does not fetch anything, stay assured that
https://codereview.chromium.org/2263933002/ will fix that. It was the only way I
found to keep the CL size reasonable.

This is obviously not urgent.

Thanks for your help!
Vaclav

[vasilii, 2016-08-23 15:01:13]

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher.h:46: const
std::vector<const autofill::PasswordForm*>& non_federated,
What is the reason that for federated credentials there is a getter while
non-federated can be retrieved only here?

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:39:
federated_matches.push_back(std::move(*federated));
std::move(first_federated, store_results->end(), federated_matches.begin());

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:105: state_ =
State::NOT_WAITING;
Shouldn't you change the state before calling the observers?

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:69:
base::string16 name_;  // |username_value| to filter
const ?

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:637:
need_to_refetch_ = false;
Curious to see how this will look like when you get rid of
OnGetPasswordStoreResults()

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1196: for
(const auto key_value : best_matches_) {
const auto&

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1205: const
auto it = best_matches_.find(form->username_value);
You probably would not write "const std::map::iterator it = ..." Then why having
it here?
Same below.

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:477:
std::vector<const autofill::PasswordForm*> blacklisted_matches_;
Who owns them now?

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:483:
std::unique_ptr<autofill::PasswordForm> new_blacklisted_;
The reason for this new member is unclear. You never read from it.

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

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

[vabr (Chromium), 2016-08-23 15:27:56]

Thanks for the comments!

I addressed them below, PTAL.

Cheers,
Vaclav

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher.h:46: const
std::vector<const autofill::PasswordForm*>& non_federated,
On 2016/08/23 15:01:13, vasilii wrote:
> What is the reason that for federated credentials there is a getter while
> non-federated can be retrieved only here?

The consumer needs a signal that the results are ready. So one alternative would
be having just a
virtual void NotifyResultsReady() = 0;
instead of ProcessMatches, and then add a getter for non-federated.

I decided to make non-federated part of the notification (which is called
ProcessMatches), because:
(1) The consumer would always need to get non-federated matches as the result of
the notification, and only sometimes the federated.
(2) The consumer, PasswordFormManager, actually has to process non-federated
logins (into best_matches), so it won't need to get them again from the fetcher,
unlike the federated matches, which can be used as they are.

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:39:
federated_matches.push_back(std::move(*federated));
On 2016/08/23 15:01:13, vasilii wrote:
> std::move(first_federated, store_results->end(), federated_matches.begin());

Done.

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl.cc:105: state_ =
State::NOT_WAITING;
On 2016/08/23 15:01:13, vasilii wrote:
> Shouldn't you change the state before calling the observers?

Indeed!
Done.

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/form_fetcher_impl_unittest.cc:69:
base::string16 name_;  // |username_value| to filter
On 2016/08/23 15:01:13, vasilii wrote:
> const ?

Done.

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

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:637:
need_to_refetch_ = false;
On 2016/08/23 15:01:13, vasilii wrote:
> Curious to see how this will look like when you get rid of
> OnGetPasswordStoreResults()

I won't get rid of it, only move it to FormFetcherImpl, see
https://codereview.chromium.org/2263933002/diff/40001/components/password_man...

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1196: for
(const auto key_value : best_matches_) {
On 2016/08/23 15:01:13, vasilii wrote:
> const auto&

Done.

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:1205: const
auto it = best_matches_.find(form->username_value);
On 2016/08/23 15:01:13, vasilii wrote:
> You probably would not write "const std::map::iterator it = ..." Then why
having
> it here?
> Same below.

If not, then just because it would be too long, which is not the case with
"auto". To me "const" tells the reader: this iterator is not used in any kind of
loop, just as a pointer.

But I'm fine with leaving the const out if you prefer, so I've done that.

Please let me know if I misunderstood and your comment was not about const.

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:477:
std::vector<const autofill::PasswordForm*> blacklisted_matches_;
On 2016/08/23 15:01:13, vasilii wrote:
> Who owns them now?

FormFetcher and possibly new_blacklisted_. I now updated the code comments as
well, thanks for pointing this out.

https://codereview.chromium.org/2252283005/diff/160001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:483:
std::unique_ptr<autofill::PasswordForm> new_blacklisted_;
On 2016/08/23 15:01:13, vasilii wrote:
> The reason for this new member is unclear. You never read from it.

It is just a storage, the access is usually through blacklisted_matches_. I
updated the comments to clarify this.

[commit-bot: I haz the power, 2016-08-23 15:28:01]

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-23 15:32:27]

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

[commit-bot: I haz the power, 2016-08-23 15:32:27]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vasilii, 2016-08-24 09:51:47]

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

https://codereview.chromium.org/2252283005/diff/180001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:294:
new_blacklisted_ = base::MakeUnique<PasswordForm>(observed_form_);
I'm afraid this implementation will not survive PermanentlyBlacklist();
PermanentlyBlacklist();
It can happen via UI. The result will be a dangling pointer.

[vabr (Chromium), 2016-08-24 11:45:12]

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

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

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-08-24 11:48:26]

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

[commit-bot: I haz the power, 2016-08-24 11:48:47]

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-08-24 11:48:50]

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

https://codereview.chromium.org/2252283005/diff/180001/components/password_ma...
components/password_manager/core/browser/password_form_manager.cc:294:
new_blacklisted_ = base::MakeUnique<PasswordForm>(observed_form_);
On 2016/08/24 09:51:47, vasilii wrote:
> I'm afraid this implementation will not survive PermanentlyBlacklist();
> PermanentlyBlacklist();
> It can happen via UI. The result will be a dangling pointer.

That's a great catch, thanks, Vasilii!

I went with just giving up on seeing non-null new_blacklisted_, because
re-creating it would lead to the same result anyway.

[vabr (Chromium), 2016-08-24 12:05:41]

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

[commit-bot: I haz the power, 2016-08-24 12:05:55]

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-08-24 12:06:07]

One more update after our chat, Vasilii, PTAL.
Thanks!
Vaclav

[vasilii, 2016-08-24 12:07:40]

lgtm

[vabr (Chromium), 2016-08-24 12:43:13]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2016-08-24 12:43:20]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-08-24 12:43:39]

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-24 13:23:30]

Description was changed from

==========
Introduce password_manager::FormFetcher

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

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

to

==========
Introduce password_manager::FormFetcher

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

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

[commit-bot: I haz the power, 2016-08-24 13:23:31]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-08-24 13:25:31]

Description was changed from

==========
Introduce password_manager::FormFetcher

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

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

to

==========
Introduce password_manager::FormFetcher

This CL adds a partial implementation of FormFetcher.

Despite its name, the added FormFetcher does not fetch forms from the
PasswordStore yet, this will come in a follow-up CL (see [1] for more context).

The CL focuses on a single change: PasswordFormManager (PFM) should no longer
own the fetched forms, their ownership is transferred to FormFetcher.
Ultimately, FormFetcher instances will be shared among PFM instances and will
do fetching for them, at which point it will be important that PFMs don't own
the forms. Because transferring the ownership is a significant change, it has
been separated in its own CL.

BUG=621355

Committed: https://crrev.com/859b5c46d834b8974188cb5fe037ecaa18748f5c
Cr-Commit-Position: refs/heads/master@{#414061}
==========

[commit-bot: I haz the power, 2016-08-24 13:25:33]

Patchset 14 (id:??) landed as
https://crrev.com/859b5c46d834b8974188cb5fe037ecaa18748f5c
Cr-Commit-Position: refs/heads/master@{#414061}