Fix for no password save when action URL is modified

chrome/browser/password_manager/password_form_manager.cc

Index: chrome/browser/password_manager/password_form_manager.cc
===================================================================
--- chrome/browser/password_manager/password_form_manager.cc	(revision 152419)
+++ chrome/browser/password_manager/password_form_manager.cc	(working copy)
@@ -57,8 +57,13 @@
          kManagerActionMax * submit_result_);
 };
 
+bool PasswordFormManager::DoesManage(const PasswordForm& form) const {
+  return PasswordFormManager::DoesManage(form, true);
+}
+
 // TODO(timsteele): use a hash of some sort in the future?
-bool PasswordFormManager::DoesManage(const PasswordForm& form) const {
+bool PasswordFormManager::DoesManage(const PasswordForm& form,
+                                     bool require_action_match) const {

[tim (not reviewing), 2012/09/05 20:43:37]

nit - please use an enum instead of bool here, e.g.
ACTION_MATCH_REQUIRED
ACTION_MATCH_NOT_REQUIRED

and don't overload the method -- I think we should call it explicitly inline?
Otherwise we should rename the method to distinguish.

[guohui, 2012/09/05 21:30:14]

Done.

   if (form.scheme != PasswordForm::SCHEME_HTML)
       return observed_form_.signon_realm == form.signon_realm;
 
@@ -69,10 +74,14 @@
     return false;
   }
 
-  // The action URL must also match, but the form is allowed to have an empty
-  // action URL (See bug 1107719).
-  if (form.action.is_valid() && (form.action != observed_form_.action))
-    return false;
+  // When require_action_match is set to true, the action URL must match, but
+  // the form is allowed to have an empty action URL (See bug 1107719).
+  // Otherwise ignore action URL, this is to allow saving password form with
+  // dynamically changed action URL (See bug 27246).
+  if (form.action.is_valid() && (form.action != observed_form_.action)) {
+    if (require_action_match)
+      return false;
+  }
 
   // If this is a replay of the same form in the case a user entered an invalid
   // password, the origin of the new form may equal the action of the "first"
@@ -171,7 +180,7 @@
 
 void PasswordFormManager::ProvisionallySave(const PasswordForm& credentials) {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
-  DCHECK(DoesManage(credentials));
+  DCHECK(DoesManage(credentials, false));
 
   // Make sure the important fields stay the same as the initially observed or
   // autofilled ones, as they may have changed if the user experienced a login
@@ -183,11 +192,6 @@
     // The user signed in with a login we autofilled.
     pending_credentials_ = *it->second;
     is_new_login_ = false;
-    // If the user selected credentials we autofilled from a PasswordForm
-    // that contained no action URL (IE6/7 imported passwords, for example),
-    // bless it with the action URL from the observed form. See bug 1107719.
-    if (pending_credentials_.action.is_empty())
-      pending_credentials_.action = observed_form_.action;
 
     // Check to see if we're using a known username but a new password.
     if (pending_credentials_.password_value != credentials.password_value)
@@ -199,6 +203,13 @@
     pending_credentials_.username_value = credentials.username_value;
   }
 
+  pending_credentials_.action = credentials.action;
+  // If the user selected credentials we autofilled from a PasswordForm
+  // that contained no action URL (IE6/7 imported passwords, for example),
+  // bless it with the action URL from the observed form. See bug 1107719.
+  if (pending_credentials_.action.is_empty())
+    pending_credentials_.action = observed_form_.action;
+
   pending_credentials_.password_value = credentials.password_value;
   pending_credentials_.preferred = credentials.preferred;
 }