[Password Generation] Wait longer to dismiss suggestion UI

components/autofill/content/renderer/password_generation_agent.cc

Index: components/autofill/content/renderer/password_generation_agent.cc
diff --git a/components/autofill/content/renderer/password_generation_agent.cc b/components/autofill/content/renderer/password_generation_agent.cc
index e90f1c9c9b79e7f5b34dff22b7846f83b03942bf..a5319ff7c0a76f6b1d81c34bfde2c6e7bfbc9130 100644
--- a/components/autofill/content/renderer/password_generation_agent.cc
+++ b/components/autofill/content/renderer/password_generation_agent.cc
@@ -288,10 +288,12 @@ bool PasswordGenerationAgent::FocusedNodeHasChanged(
     return true;
   }
 
-  // Only trigger if the password field is empty.
+  // Assume that if the password field has less than kMaximumOfferSize

[Ilya Sherman, 2014/08/06 22:25:08]

Optional nit: Hmm, "kMaximumOfferSize" makes me think that the UI should still
be shown when the max is reached, i.e. the code below looks like an off-by-one
error.  WDYT of either renaming the variable, or changing the constant from 5 to
4, and the comparison from "<" to "<="?

[Garrett Casto, 2014/08/08 18:19:19]

Ah, there actually is an off by one error here. This should be <=. Updated and
added a test.

+  // characters then the user is not finished typing their password and display
+  // the password suggestion.

[Ilya Sherman, 2014/08/06 22:25:08]

This comment suggests that we think the user probably is finished typing their
password once they've typed 5 characters.  Do we really think that user
passwords are so short?

[Garrett Casto, 2014/08/08 18:19:19]

I think the idea is that they aren't finish, so it's still possible to influence
them to use a generated password. I'll try to clarify this.

   if (!element->isReadOnly() &&
       element->isEnabled() &&
-      element->value().isEmpty()) {
+      element->value().length() < kMaximumOfferSize) {

[Ilya Sherman, 2014/08/06 22:25:08]

Optional nit: I think .size() is more commonly used than .length() in Chromium
code (though, obviously, they're equivalent).

[Garrett Casto, 2014/08/08 18:19:19]

blink::WebString doesn't have size()

     ShowGenerationPopup();
     return true;
   }
@@ -318,10 +320,7 @@ bool PasswordGenerationAgent::TextDidChangeInTextField(
 
     // Offer generation again.
     ShowGenerationPopup();
-  } else if (!password_is_generated_) {
-    // User has rejected the feature and has started typing a password.
-    HidePopup();
-  } else {
+  } else if (password_is_generated_) {
     password_edited_ = true;
     // Mirror edits to any confirmation password fields.
     for (std::vector<blink::WebInputElement>::iterator it =
@@ -329,6 +328,13 @@ bool PasswordGenerationAgent::TextDidChangeInTextField(
          it != password_elements_.end(); ++it) {
       it->setValue(element.value());
     }
+  } else if (element.value().length() > kMaximumOfferSize) {

[Ilya Sherman, 2014/08/06 22:25:08]

If the comparison above is "<", then the inverse here should be ">=".

[Garrett Casto, 2014/08/08 18:19:19]

This one is right, the other is wrong.

+    // User has rejected the feature and has started typing a password.
+    HidePopup();
+  } else {
+    // Password isn't generated and there is less than kMaximumOfferSize

[Ilya Sherman, 2014/08/06 22:25:08]

nit: "is less" -> "are fewer"

[Garrett Casto, 2014/08/08 18:19:19]

Done.

+    // characters typed, so keep offering the password.
+    ShowGenerationPopup();

[Ilya Sherman, 2014/08/06 22:25:08]

This looks like it'll show a new popup, rather than keeping the current one up. 
Is that intentional?

[Garrett Casto, 2014/08/08 18:19:19]

It actually just updates the bounds of the current popup if one exists.  Updated
the comment to reflect this.

   }
 
   return true;