Autofill popup should not be presented when autocomplete='off', even if

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 2e747a3f7c4bfe46bd66f7da3b5b5863253b8820..9da9ae24cbbba8a60187977d6ac05b7f09130026 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -26,6 +26,7 @@
 #include "third_party/WebKit/public/web/WebInputEvent.h"
 #include "third_party/WebKit/public/web/WebNode.h"
 #include "third_party/WebKit/public/web/WebNodeList.h"
+#include "third_party/WebKit/public/web/WebPasswordFormData.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 #include "third_party/WebKit/public/web/WebView.h"
@@ -320,6 +321,13 @@ bool PasswordAutofillAgent::DidClearAutofillSelection(
 
 bool PasswordAutofillAgent::ShowSuggestions(
     const blink::WebInputElement& element) {
+  blink::WebPasswordFormData form_data(element.form());

[Garrett Casto, 2013/12/27 22:09:48]

You also need to verify that element.form() is valid. It doesn't look like this
is checked previously in the call chain and it's possible to have input elements
that aren't in a form.

[jww, 2013/12/27 23:04:39]

I believe this is irrelevant given my changes per your comment below. Please
correct me if I'm wrong.

+  scoped_ptr<PasswordForm> password_form = CreatePasswordForm(element.form());
+  if (!ShouldIgnoreAutocompleteOffForPasswordFields() && password_form.get() &&
+      (!element.autoComplete() || !password_form->password_autocomplete_set ||
+       !form_data.passwordShouldAutocomplete))

[Ilya Sherman, 2013/12/27 21:43:22]

Why do you need both "!password_form->password_autocomplete_set" and
"!form_data.passwordShouldAutocomplete"?  It seems like all that matters is the
latter one -- whether autocomplete is *currently* disabled for the actual HTML
element.  If autocomplete had previously been disabled, but then got enabled by
the time we get here, I think it should be fine to show the popup.  Is there an
interesting case that I'm overlooking?

[Garrett Casto, 2013/12/27 22:09:48]

passwordShouldAutocomplete implies password_autocomplete_set, so there is no
reason to check both.

It seems like the right thing to do is move this seciton after the iterator
check. Then you check if both the password field (iter->second.password_field)
and the username field are fillable in the same manner that
FillUserNameAndPassword does. You might want to factor out the code that checks
that into a different function, as it does seem to get a lot of use
(IsElementEditable && ShouldIgnore...).

[jww, 2013/12/27 23:04:39]

Done.

+    return true;
+
   LoginToPasswordInfoMap::const_iterator iter =
       login_to_password_info_.find(element);
   if (iter == login_to_password_info_.end())