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)
@@ -58,7 +58,8 @@
 };
 
 // TODO(timsteele): use a hash of some sort in the future?
-bool PasswordFormManager::DoesManage(const PasswordForm& form) const {
+bool PasswordFormManager::DoesManage(const PasswordForm& form,
+                                     ActionMatch action_match) const {
   if (form.scheme != PasswordForm::SCHEME_HTML)
       return observed_form_.signon_realm == form.signon_realm;
 
@@ -69,10 +70,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 action match is required, 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 (action_match == kActionMatchRequired)

[tim (not reviewing), 2012/09/05 21:40:59]

nit - chromium style diverges from Google style here and prefers MACRO_STYLE for
enum values. (see http://dev.chromium.org/developers/coding-style)

[guohui, 2012/09/17 13:28:55]

Done.

+      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 +176,7 @@
 
 void PasswordFormManager::ProvisionallySave(const PasswordForm& credentials) {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
-  DCHECK(DoesManage(credentials));
+  DCHECK(DoesManage(credentials, kActionMatchNotRequired));
 
   // 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 +188,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 +199,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;

[tim (not reviewing), 2012/09/05 21:40:59]

Could we add a test that verifies this gets updated on the "new username" case
as well? See TestEmptyAction.

[guohui, 2012/09/17 13:28:55]

Updated TestNewLogin to verify the action gets populated correctly on the new
username case. Also created a new test case TestUpdateAction to verify the
action get updated on the case when username and password are the same as saved
but action URL has changed.

+
   pending_credentials_.password_value = credentials.password_value;
   pending_credentials_.preferred = credentials.preferred;
 }