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

components/autofill/content/renderer/password_autofill_agent.cc

 // Copyright 2013 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.
 
 #include "components/autofill/content/renderer/password_autofill_agent.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 841 matching lines @@
 
 bool PasswordAutofillAgent::OriginCanAccessPasswordManager(
     const blink::WebSecurityOrigin& origin) {
   return origin.canAccessPasswordManager();
 }
 
 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();
 }
 
 void PasswordAutofillAgent::SendPasswordForms(bool only_visible) {
   scoped_ptr<RendererSavePasswordProgressLogger> logger;
   if (logging_state_active_) {
     logger.reset(new RendererSavePasswordProgressLogger(this, routing_id()));
     logger->LogMessage(Logger::STRING_SEND_PASSWORD_FORMS_METHOD);
     logger->LogBoolean(Logger::STRING_ONLY_VISIBLE, only_visible);
@@ skipping 486 matching lines @@
   scoped_ptr<PasswordForm> password_form(
       CreatePasswordForm(form, &nonscript_modified_values_));
   if (!password_form || (restriction == RESTRICTION_NON_EMPTY_PASSWORD &&
                          password_form->password_value.empty() &&
                          password_form->new_password_value.empty())) {
     return;
   }
   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(
     content::RenderView* render_view,
     PasswordAutofillAgent* agent)
     : content::RenderViewObserver(render_view), agent_(agent) {
 }
 
 PasswordAutofillAgent::LegacyPasswordAutofillAgent::
     ~LegacyPasswordAutofillAgent() {
@@ skipping 10 matching lines @@
 void PasswordAutofillAgent::LegacyPasswordAutofillAgent::DidStopLoading() {
   agent_->DidStopLoading();
 }
 
 void PasswordAutofillAgent::LegacyPasswordAutofillAgent::
     DidStartProvisionalLoad(blink::WebLocalFrame* navigated_frame) {
   agent_->LegacyDidStartProvisionalLoad(navigated_frame);
 }
 
 }  // namespace autofill