[PasswordManager] Improve detection of ignorable change password forms.

components/password_manager/core/browser/password_form_manager.h

Index: components/password_manager/core/browser/password_form_manager.h
diff --git a/components/password_manager/core/browser/password_form_manager.h b/components/password_manager/core/browser/password_form_manager.h
index efda7d3cf91490ceb1242a884e301d2c7b13310e..d55efff671180f79baf7ccf79cd1af476e15e148 100644
--- a/components/password_manager/core/browser/password_form_manager.h
+++ b/components/password_manager/core/browser/password_form_manager.h
@@ -84,7 +84,7 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // the same thread!
   bool HasCompletedMatching() const;
 
-  // Returns true if the observed form has both the current and new password
+  // Returns true if the |candidate| form has both the current and new password

[vabr (Chromium), 2015/02/10 18:54:57]

Most of this comment is out of date, please improve.

[vabr (Chromium), 2015/02/10 18:54:57]

I have another slight problem with the current wording:
The "Form" from the method name, means the HTML form on the page for which
|this| was created. In other words, it still means the observed_form_. The role
of the argument |candidate| is just to provide an evidence against the form
being ignorable. While |candidate| and |observed_form_| are both of the same
data type, the former represents just what the user typed here, while the latter
represents the HTML form in which the user typed it.

Therefore I suggest replacing |candidate| with a pair of strings, the username
and password values from |candidate|:

bool IsIgnorableChangePasswordForm(const base::string16& typed_username, const
base::string16& typed_password) const;

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

   // fields, and the username field was not explicitly marked with
   // autocomplete=username. In these cases it is not clear whether the username
   // field is the right guess (often such change password forms do not contain
@@ -92,7 +92,8 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // potentially malformed credential. Once we handle change password forms
   // correctly, or http://crbug.com/448351 gets implemented, this method should
   // be replaced accordingly.
-  bool IsIgnorableChangePasswordForm() const;
+  bool IsIgnorableChangePasswordForm(
+      const autofill::PasswordForm& candidate) const;
 
   // Determines if the user opted to 'never remember' passwords for this form.
   bool IsBlacklisted() const;
@@ -282,6 +283,12 @@ class PasswordFormManager : public PasswordStoreConsumer {
       const autofill::FormData& form_data,
       const autofill::ServerFieldType& password_type);
 
+  // Returns true if |candidate|'s username and password field value matches

[vabr (Chromium), 2015/02/10 18:54:57]

To avoid increasing the header file, I suggest changing this method to a
function in the anonymous namespace in the .cc file.
For that it will need to take an additional argument, a const ref to the
best_matches_, but that's OK. The name should be changed accordingly, to
something like
bool DoesCandidateMatchCredentials(const autofill::PasswordForm& candidate,
const autofill::PasswordFormMap& credentials);

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+  // with one of the stored password form within |best_matches_|; otherwise
+  // false.
+  bool HasMatchingCredentialsInStore(
+      const autofill::PasswordForm& candidate) const;
+
   // Set of PasswordForms from the DB that best match the form
   // being managed by this. Use a map instead of vector, because we most
   // frequently require lookups by username value in IsNewLogin.