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..2d591c37b137852ef4397b53ead6a71897015287 100644
--- a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
+++ b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc
@@ -254,6 +254,10 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
                                      GetMainFrame(), username_element_);
   }
 
+  bool SimulateShowSuggestions() {
+    return autofill_agent_->password_autofill_agent_->ShowSuggestions(
+        username_element_);
+  }
 
   void SimulateKeyDownEvent(const WebInputElement& element,
                             ui::KeyboardCode key_code) {
@@ -806,4 +810,44 @@ TEST_F(PasswordAutofillAgentTest, NoDOMActivationTest) {
   CheckTextFieldsDOMState(kAliceUsername, true, "", true);
 }
 
+// Regression test for http://crbug.com/326679
+TEST_F(PasswordAutofillAgentTest, SelectUsernameOnAutofillOff) {

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

nit: "On" -> "When" or "With" or something like that, so that it doesn't look
like it's contrasting with "Off"

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

Done.

+  // Set the main password element to autocomplete='off'
+  password_element_.setAttribute(WebString::fromUTF8("autocomplete"),
+                                 WebString::fromUTF8("off"));

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

It looks like the code refuses to show suggestions if autocomplete is off for
*either* the password field or the username field.  Right now, though, you're
only testing the case where the password field disables autocomplete.  Please
add test coverage for the username case as well.

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

Done.

+
+  // Simulate the browser sending back the login info, it triggers the
+  // autocomplete.
+  SimulateOnFillPasswordForm(fill_data_);
+
+  // Simulate the user changing the username to some unknown username.
+  SimulateUsernameChange("foobar", true);
+
+  // 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(SimulateShowSuggestions());

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

Please also test that no AutofillHostMsg_ShowPasswordSuggestions message is
generated for a known username (and I guess make sure that it had been being
generated prior to this CL).  That seems like a more direct test of the property
you're trying to verify.

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

That's a great idea. I didn't attempt it because I thought there was no way to
access the browser process from the browser test to check if an IPC handler
fired (or, even harder, if it didn't fire).

Your suggestion, though, of checking for the creation (or lack thereof) of the
message makes a lot of sense, though. Do you have any pointers on how to do this
(or perhaps an example browser test that does this)?
On 2013/12/27 21:43:22, Ilya Sherman (Away thru Jan 6) wrote:

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

I think something like this ought to work:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au...

If that doesn't quite help enough on its own, you might try a search for
something like "file:autofill file:renderer ipc sink" on
https://code.google.com/p/chromium/codesearch

On 2013/12/27 23:04:39, jww wrote:

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

Done.

+}
+
 }  // namespace autofill