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

chrome/renderer/autofill/password_autofill_agent_browsertest.cc

Index: chrome/renderer/autofill/password_autofill_agent_browsertest.cc
diff --git a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
index a71ea37ba8cb08939a8d4bc0e89582f249b16532..70961c7085f89597e59728ea169df22413154e71 100644
--- a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
+++ b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
@@ -254,6 +254,41 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
                                      GetMainFrame(), username_element_);
   }
 
+  // Tests that no suggestion popup is generated when the username_element_ is
+  // edited.
+  void ExpectNoSuggestionsPopup() {
+    // This EXPECT_TRUE is interesting because of a quirk in how we handle the
+    // corner case of autocomplete='off' fields that also have a remembered
+    // password and username. Normally, if a unknown username is enterted, the

[Ilya Sherman, 2014/01/07 00:56:23]

nit: "a unknown" -> "an unknown"

[jww, 2014/01/07 20:00:25]

Done.

+    // autofill agent will return "false" from ShowSuggestions, but note that
+    // we're expecting "true" here.

[Ilya Sherman, 2014/01/07 00:56:23]

This sentence doesn't make sense anymore, since the tested username is known,
rather than unknown.  Please update this comment.  (Maybe it's worth testing
with an unknown username as well? Up to you.)

[jww, 2014/01/07 20:00:25]

Done.

+    //
+    // This is because of the specific case of an autocomplete='off' form that
+    // also has a remembered username and password (http://crbug.com/326679). To
+    // fix the DCHECK that this case used to hit, we return "true" from
+    // ShowSuggestions for all forms that are autocomplete='off', prentending
+    // that we've successfully shown a selection box to the user. Of course, we
+    // haven't so a message is never sent to the browser on acceptance, and the
+    // DCHECK isn't hit (and nothing is filled).
+    //
+    // However, because we do it for all usernames (known or unknown), we can
+    // differentiate this from before the patch, since it used to be that
+    // ShowSuggestions would return "false" on unknown usernames. However, we
+    // couldn't return "false" in the new case, because that would cause
+    // QueryAutofillSuggestions to be called, ultimately leading to the DCHECK
+    // being hit again.
+    //
+    // Thus, the regression test is to make sure that we are returing "true" for
+    // all calls to ShowSuggestions on autocomplete='off' forms, even for
+    // unknown usernames.
+    EXPECT_TRUE(autofill_agent_->password_autofill_agent_->ShowSuggestions(
+        username_element_));
+
+    // And as a final sanity check, we also make sure that no "show suggestions"

[Ilya Sherman, 2014/01/07 00:56:23]

nit: s/"we"//

[jww, 2014/01/07 20:00:25]

Done.

+    // IPC to the browser was generated.
+    EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+                    AutofillHostMsg_ShowPasswordSuggestions::ID) == NULL);

[Ilya Sherman, 2014/01/07 00:56:23]

nit: I'd write this as either

EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
                AutofillHostMsg_ShowPasswordSuggestions::ID));

or as an EXPECT_EQ.  EXPECT_TRUE(a == b) is an unusual pattern.

[jww, 2014/01/07 20:00:25]

Done.

+  }
 
   void SimulateKeyDownEvent(const WebInputElement& element,
                             ui::KeyboardCode key_code) {
@@ -806,4 +841,33 @@ TEST_F(PasswordAutofillAgentTest, NoDOMActivationTest) {
   CheckTextFieldsDOMState(kAliceUsername, true, "", true);
 }
 
+// The following two tests are regression tests for http://crbug.com/326679

[Ilya Sherman, 2014/01/07 00:56:23]

nit: Please have one comment per test, in case tests move around or whatnot. 
It's fine if both comments are something very brief like "Regression test for
http://crbug.com/326679"

[jww, 2014/01/07 20:00:25]

Done.

+TEST_F(PasswordAutofillAgentTest, SelectUsernameWithUsernameAutofillOff) {
+  // Simulate the browser sending back the login info.
+  SimulateOnFillPasswordForm(fill_data_);
+
+  // Set the main password element to autocomplete='off'

[Ilya Sherman, 2014/01/07 00:56:23]

nit: Please update this comment.

[jww, 2014/01/07 20:00:25]

Done.

+  username_element_.setAttribute(WebString::fromUTF8("autocomplete"),
+                                 WebString::fromUTF8("off"));
+
+  // Simulate the user changing the username to some known username.
+  SimulateUsernameChange(kAliceUsername, true);
+
+  ExpectNoSuggestionsPopup();
+}
+
+TEST_F(PasswordAutofillAgentTest, SelectUsernameWithPasswordAutofillOff) {
+  // Simulate the browser sending back the login info.
+  SimulateOnFillPasswordForm(fill_data_);
+
+  // Set the main password element to autocomplete='off'
+  password_element_.setAttribute(WebString::fromUTF8("autocomplete"),
+                                 WebString::fromUTF8("off"));
+
+  // Simulate the user changing the username to some known username.
+  SimulateUsernameChange(kAliceUsername, true);
+
+  ExpectNoSuggestionsPopup();
+}
+
 }  // namespace autofill