[PasswordManager] Improve detection of ignorable change password forms.

components/password_manager/core/browser/password_form_manager.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_MANAGER_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_MANAGER_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 66 matching lines @@
   // conceivable that a user (or ui test) could attempt to submit a login
   // prompt before the callback has occured, which would InvokeLater a call to
   // PasswordManager::ProvisionallySave, which would interact with this object
   // before the db has had time to answer with matching password entries.
   // This is intended to be a one-time check; if the return value is false the
   // expectation is caller will give up. This clearly won't work if you put it
   // in a loop and wait for matching to complete; you're (supposed to be) on
   // 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
   // the username at all), and the user should not be bothered with saving a
   // 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;
 
   // Used by PasswordManager to determine whether or not to display
   // a SavePasswordBar when given the green light to save the PasswordForm
   // managed by this.
   bool IsNewLogin() const;
 
   // Returns true if the current pending credentials were found using
@@ skipping 169 matching lines @@
 
   // Remove possible_usernames that may contains sensitive information and
   // duplicates.
   void SanitizePossibleUsernames(autofill::PasswordForm* form);
 
   // Helper function to delegate uploading to the AutofillManager.
   virtual void UploadPasswordForm(
       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.
   autofill::PasswordFormMap best_matches_;
 
   // Cleans up when best_matches_ goes out of scope.
   STLValueDeleter<autofill::PasswordFormMap> best_matches_deleter_;
 
   // The PasswordForm from the page or dialog managed by |this|.
   const autofill::PasswordForm observed_form_;
@@ skipping 58 matching lines @@
   ManagerAction manager_action_;
   UserAction user_action_;
   SubmitResult submit_result_;
 
   DISALLOW_COPY_AND_ASSIGN(PasswordFormManager);
 };
 
 }  // namespace password_manager
 
 #endif  // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_MANAGER_H_