[Password Manager] Use successful XHR submission as a signal for password saving

components/autofill/content/renderer/password_autofill_agent.cc

Index: components/autofill/content/renderer/password_autofill_agent.cc
diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
index 78679f3637b2e55e4d190878b87937ca3e6eccb6..b5e264a1548f8630f6e9ee8b4ec5bff4bb3f8df1 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -859,6 +859,39 @@ void PasswordAutofillAgent::OnDynamicFormsSeen() {
   SendPasswordForms(false /* only_visible */);
 }
 
+void PasswordAutofillAgent::XHRSucceeded() {
+  // If the user hasn't fully filled out the form, don't try to save.

[vabr (Chromium), 2015/03/17 10:14:38]

optional nit: To me, the comment duplicates what the method name already says.
But feel free to leave it if you feel otherwise.

[Garrett Casto, 2015/03/17 21:36:15]

I'm pretty indifferent. If you don't feel that it adds value, I'll remove it.

+  if (!ProvisionallySavedPasswordIsValid())
+    return;
+
+  // Prompt to save only if the form is now gone, either invisible or
+  // removed from the DOM.
+  blink::WebFrame* frame = render_frame()->GetWebFrame();
+  blink::WebVector<blink::WebFormElement> forms;
+  frame->document().forms(forms);
+
+  for (size_t i = 0; i < forms.size(); ++i) {
+    const blink::WebFormElement& form = forms[i];
+    bool is_form_visible = IsWebNodeVisible(form);
+
+    if (!is_form_visible) {

[vabr (Chromium), 2015/03/17 10:14:38]

Why not replace the bool directly with the call to IsWebNodeVisible(form) here?

[Garrett Casto, 2015/03/17 21:36:15]

Copy and paste issue. Fixed.

+      continue;
+    }
+
+    scoped_ptr<PasswordForm> password_form(
+        CreatePasswordForm(form, &nonscript_modified_values_));
+    if (password_form.get()) {
+      if (provisionally_saved_form_->action == password_form->action) {
+        // Form still exists, no save required.
+        return;
+      }
+    }
+  }
+  Send(new AutofillHostMsg_InPageNavigation(routing_id(),
+                                            *provisionally_saved_form_));
+  provisionally_saved_form_.reset();
+}
+
 void PasswordAutofillAgent::FirstUserGestureObserved() {
   gatekeeper_.OnUserGesture();
 }
@@ -1365,6 +1398,13 @@ void PasswordAutofillAgent::ProvisionallySavePassword(
   provisionally_saved_form_ = password_form.Pass();
 }
 
+bool PasswordAutofillAgent::ProvisionallySavedPasswordIsValid() {
+   return provisionally_saved_form_ &&
+       !provisionally_saved_form_->username_value.empty() &&
+       (!provisionally_saved_form_->password_value.empty() ||

[vabr (Chromium), 2015/03/17 10:14:38]

Did you mean to apply the negation to the whole disjunction instead of just to
the first operand?

[Garrett Casto, 2015/03/17 21:36:15]

Yikes. Nice catch. Added additional tests to verify that this works as expected
for signup forms as well.

+        provisionally_saved_form_->new_password_value.empty());
+}
+
 // LegacyPasswordAutofillAgent -------------------------------------------------
 
 PasswordAutofillAgent::LegacyPasswordAutofillAgent::LegacyPasswordAutofillAgent(