TODO for the improved tests: make sure that PasswordForm::times_used count is properly updated during saving (and the list of other possible usernames is cleared).
This is not tested currently, and was brought up during refactoring in https://codereview.chromium.org/2086483002/#msg4. It will be easier to do once PFM is a smaller class and one can mock FormSaver instead of the whole PasswordStore.
The next step in the design doc is to separate store reading from PFM to CredentialMatcher. I plan to do that in 3 steps:
(1) Remove PasswordForm::ssl_valid, as described in bug 413020 . While not necessary for the other steps, it will make the refactoring simpler.
(2) Change PasswordStore::GetLogins to take just the PasswordForm::Scheme and signon_realm instead of the whole form. This is needed for the whole idea of this refactoring -- we can separate fetching credentials from PFMs, because the fetching does not actually depend on the whole form, just its origin (and whether it is an HTML form or not). The PasswordStore::GetLogins signature does not make that clear right now. I checked the use of the passed PasswordForm, and indeed just the scheme, signon_realm, ssl_valid and origin are used. The first two will still be passed. The third is removed in step (1) here. The origin is derived from signon_realm, so no need to pass that.
(3) Create CredentialMatcher as specified in the design doc https://docs.google.com/document/d/12CH_SV1gIJKaEIGTs7gWBiXrCMlBa3o32OyC2LRtPAo/edit?usp=sharing.
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/f6c4d75aab7ba327efc8e6e05d414b446fa63ecd
commit f6c4d75aab7ba327efc8e6e05d414b446fa63ecd
Author: vabr <vabr@chromium.org>
Date: Mon Aug 22 14:54:15 2016
Copy PasswordForm in FormSaverImpl::UpdatePreferredLoginState
UpdatePreferredLoginState needs to update PasswordStore with a slightly
modified version of some of the forms in PasswordFormManager's |best_matches_|.
Currently it does so by modifying |best_matches_| directly, and then updating
the PasswordStore.
That has the advantage of keeping an up-to-date copy in |best_matches_| without
reloading from PasswordStore (and also sparing PasswordForm copies inside
UpdatePreferredLoginState.
It also has a disadvantage: |best_matches_| cannot be const. That will become
a problem once the ownership of |best_matches_| will be transferred from
PasswordFormManager to a new class, FormFetcher, shared by multiple
PasswordFormManager instances. While it is possible to let all
PasswordFormManager instances modify the forms owned by a FormFetcher, that
will make the code hard to understand and prone to unexpected side-effects.
The advantage seems limited -- at the point UpdatePreferredLoginState is called,
the PasswordFormManager is saving a password form, and will be deleted soon.
It will not need the |best_matches_|. Once FormFetcher starts owning them,
it will need to wait for a refresh from PasswordStore to get them updated.
But that's not worse than today, when different PasswordFormManager instances do
not share their |best_matches_| at all.
BUG= 621355
Review-Url: https://codereview.chromium.org/2261723002
Cr-Commit-Position: refs/heads/master@{#413441}
[modify] https://crrev.com/f6c4d75aab7ba327efc8e6e05d414b446fa63ecd/components/password_manager/core/browser/form_saver_impl.cc
r436571 separated fetching passwords from PasswordFormManager, into FormFetcherImpl. The FFI is currently still owned and created by PFM.
One last big step remaining is to move the creation and ownership to PasswordManager.
The only unfinished part here was the plan to share the FormFetcher among PasswordFormManagers.
There has not been enough problems caused by the lack of sharing so far to warrant further refactoring. Depending on the potential future issues, there also might be other ways around it (e.g., PasswordStore cache).
Comment 1 by vabr@chromium.org
, Jun 20 2016