[Password Manager] Send a request to the password store if there is a password field on a page

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 51a16769c8209b2febc1529cdefd4983f43ae6d8..1144a24cf875dc6bc1133df1b8709bacef2a3776 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -599,6 +599,23 @@ void AnnotateFormsWithSignatures(
   }
 }
 
+// Returns true iff there is a password field in |frame|.
+bool HasPasswordField(const blink::WebLocalFrame& frame) {
+  CR_DEFINE_STATIC_LOCAL(blink::WebString, kPassword, ("password"));
+
+  const blink::WebElementCollection elements = frame.GetDocument().All();
+  for (blink::WebElement element = elements.FirstItem(); !element.IsNull();

[dvadym, 2017/04/13 09:22:53]

Nit: blink::WebElement -> const blink::WebElement&

[vabr (Chromium), 2017/04/13 10:12:58]

FirstItem returns a (temporary) value, not reference. Keeping a const ref to the
temporary might be OK (I believe there was some special rule in C++ about
extending the lifetime of objects for const refs specifically), but why rely on
magic if we can write the code to be clear?

Note that there should be no copy construction involved, the assignment should
just result in the returned temporary getting the name |element| and lifespan of
the for-loop. (I tested this locally with a dummy .cc file.)

[kolos1, 2017/04/13 13:26:06]

it is not possible to declare the variable as const, since it is changed on
every iteration.

+       element = elements.NextItem()) {
+    if (element.IsFormControlElement()) {
+      const blink::WebFormControlElement& control =
+          element.To<blink::WebFormControlElement>();
+      if (control.FormControlType() == kPassword)
+        return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1118,20 +1135,32 @@ void PasswordAutofillAgent::SendPasswordForms(bool only_visible) {
     }
   }
 
-  if (password_forms.empty() && !only_visible) {
-    // We need to send the PasswordFormsRendered message regardless of whether
-    // there are any forms visible, as this is also the code path that triggers
-    // showing the infobar.
-    return;
-  }
-
   if (only_visible) {
+    // Send the PasswordFormsRendered message regardless of whether there are
+    // any forms visible, as this is also the code path that triggers showing

[vabr (Chromium), 2017/04/13 06:44:18]

nit: (1) let's not say infobar when it's bubble on on desktop, and (2) let's be
more concrete about how the empty vector is used. Altogether I suggest this
comment instead of the current block:

Send the PasswordFormsRendered message regardless of whether |password_forms| is
empty. The empty |password_forms| are a possible signal to the browser that a
pending login attempt succeeded.

[vabr (Chromium), 2017/04/13 06:44:18]

optional nit: Replace "there are any forms visible" with "|password_forms|" are
empty. That means the same, but is less confusing in connection to the variable
name |only_visible| -- currently the reader might be surprised to see a block of
code guarded by "if (only_possible)" to have a comment saying "regardless of ...
visible".

[kolos1, 2017/04/13 08:35:48]

Done.

+    // the infobar.
     blink::WebFrame* main_frame = render_frame()->GetWebFrame()->Top();
     bool did_stop_loading = !main_frame || !main_frame->IsLoading();
     GetPasswordManagerDriver()->PasswordFormsRendered(password_forms,
                                                       did_stop_loading);
   } else {
-    GetPasswordManagerDriver()->PasswordFormsParsed(password_forms);
+    // If there is a password field, but the list of password forms is empty for
+    // some reason, add a dummy form to the list. It will cause a request to the
+    // store. Therefore, saved passwords will be available for filling on click.
+    if (password_forms.empty() && HasPasswordField(*frame)) {

[dvadym, 2017/04/13 09:22:53]

I'd create a boolean flag |request_to_store_sent| in PasswordAutofillAgent,
whether the request to store was sent and not to resend it again here. In this
implementation we can sent requests to browser many times for the same frame.

[kolos1, 2017/04/13 13:26:06]

Done.

+      std::unique_ptr<PasswordForm> password_form(new PasswordForm());

[dvadym, 2017/04/13 09:22:53]

It looks that you don't need unique_ptr here, you can just create PasswordForm
on stack, or probably even better to push_back an empty PasswordForm and then to
set its fields .

[kolos1, 2017/04/13 13:26:06]

Done.

+      // Set everything that |FormDigest| needs.
+      password_form->scheme = PasswordForm::SCHEME_HTML;
+      password_form->origin =
+          form_util::GetCanonicalOriginForDocument(frame->GetDocument());
+      GURL::Replacements rep;
+      rep.SetPathStr("");
+      password_form->signon_realm =
+          password_form->origin.ReplaceComponents(rep).spec();
+      password_forms.push_back(*password_form);
+    }
+    if (!password_forms.empty())
+      GetPasswordManagerDriver()->PasswordFormsParsed(password_forms);
   }
 }