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

chrome/renderer/autofill/password_autofill_agent_browsertest.cc

 // Copyright (c) 2012 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 "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/test/base/chrome_render_view_test.h"
 #include "components/autofill/content/common/autofill_messages.h"
 #include "components/autofill/content/renderer/autofill_agent.h"
 #include "components/autofill/content/renderer/form_autofill_util.h"
@@ skipping 236 matching lines @@
     // PasswordAutocompleteManager::TextDidChangeInTextField() for details.
     base::MessageLoop::current()->RunUntilIdle();
   }
 
   void SimulateUsernameChange(const std::string& username,
                               bool move_caret_to_end) {
     SimulateUsernameChangeForElement(username, move_caret_to_end,
                                      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) {
     blink::WebKeyboardEvent key_event;
     key_event.windowsKeyCode = key_code;
     autofill_agent_->textFieldDidReceiveKeyDown(element, key_event);
   }
 
   void CheckTextFieldsStateForElements(const WebInputElement& username_element,
                                        const std::string& username,
@@ skipping 532 matching lines @@
 
 // Verfies that a DOM-activated UI event will not cause an autofill.
 TEST_F(PasswordAutofillAgentTest, NoDOMActivationTest) {
   // Trigger the initial autocomplete.
   SimulateOnFillPasswordForm(fill_data_);
 
   ExecuteJavaScript(kJavaScriptClick);
   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