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..bc693168e4ae7cc1e5cededbf1eb3895738e4bcd 100644
--- a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
+++ b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
@@ -254,6 +254,40 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
                                      GetMainFrame(), username_element_);
   }
 
+  void SimulateShowSuggestionsWithAutocompleteOff(WebInputElement& element) {

[Ilya Sherman, 2013/12/27 23:17:09]

Please document this method.

[jww, 2013/12/31 00:33:05]

Done.

+    // Set the main password element to autocomplete='off'
+    element.setAttribute(WebString::fromUTF8("autocomplete"),
+                         WebString::fromUTF8("off"));
+
+    // Simulate the user changing the username to some unknown username.
+    SimulateUsernameChange("foobar", true);

[Ilya Sherman, 2013/12/27 23:17:09]

IMO, lines 258-263 should be kept in the individual test cases, especially since
I think it's important to test known usernames as well as unknown ones (known
ones, I'd expect, are the ones that would have previously shown a suggestions
popup).  The remaining lines I would recommend keeping factored out, since the
comment is long.  For those, I would name the method containing them something
starting with "Expect", e.g. "ExpectNoSuggestionsPopup()".

[jww, 2013/12/31 00:33:05]

Because of the nature of the test setup, it actually doesn't matter if the
usernames are known are unknown; in either case, the show suggestions message
will be sent. Just to clean things up, I actually changed this to use known
usernames.
On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote:
Done.

+
+    // 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
+    // autofill agent will return "false" from ShowSuggestions, but note that
+    // we're expecting "true" here.
+    //
+    // 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_)); }
 
   void SimulateKeyDownEvent(const WebInputElement& element,
                             ui::KeyboardCode key_code) {
@@ -806,4 +840,22 @@ TEST_F(PasswordAutofillAgentTest, NoDOMActivationTest) {
   CheckTextFieldsDOMState(kAliceUsername, true, "", true);
 }
 
+// Regression test for http://crbug.com/326679
+TEST_F(PasswordAutofillAgentTest, SelectUsernameWithAutofillOff) {
+  // Do the test with the password element having autocomplete='off' set
+
+  // Simulate the browser sending back the login info.
+  SimulateOnFillPasswordForm(fill_data_);
+
+  SimulateShowSuggestionsWithAutocompleteOff(password_element_);
+
+  // Clear the text fields to start fresh.
+  ClearUsernameAndPasswordFields();

[Ilya Sherman, 2013/12/27 23:17:09]

Please make a separate test case (TEST_F invocation), rather than resetting
state within the test.  Shorter test cases are easier to keep simple and
correct, and also tend to be clearer.

[jww, 2013/12/31 00:33:05]

Done.

+
+  // Do the test with the username element having autocomplete='off' set
+  SimulateOnFillPasswordForm(fill_data_);
+
+  SimulateShowSuggestionsWithAutocompleteOff(username_element_);
+}
+
 }  // namespace autofill