[Password Generation] Wait longer to dismiss suggestion UI

components/autofill/content/renderer/password_generation_agent.cc

 // Copyright 2013 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 "components/autofill/content/renderer/password_generation_agent.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "components/autofill/content/common/autofill_messages.h"
@@ skipping 270 matching lines @@
   const blink::WebInputElement* element = toWebInputElement(&web_element);
   if (!element || *element != generation_element_)
     return false;
 
   if (password_is_generated_) {
     generation_element_.setShouldRevealPassword(true);
     ShowEditingPopup();
     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;
   }
 
   return false;
 }
 
 bool PasswordGenerationAgent::TextDidChangeInTextField(
     const blink::WebInputElement& element) {
   if (element != generation_element_)
     return false;
 
   if (element.value().isEmpty()) {
     if (password_is_generated_) {
       // User generated a password and then deleted it.
       password_generation::LogPasswordGenerationEvent(
           password_generation::PASSWORD_DELETED);
     }
 
     // Do not treat the password as generated.
     // TODO(gcasto): Set PasswordForm::type in the browser to TYPE_NORMAL.
     password_is_generated_ = false;
     generation_element_.setShouldRevealPassword(false);
 
     // 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 =
              password_elements_.begin();
          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;
 }
 
 void PasswordGenerationAgent::ShowGenerationPopup() {
   gfx::RectF bounding_box_scaled =
       GetScaledBoundingBox(render_view_->GetWebView()->pageScaleFactor(),
                            &generation_element_);
 
@@ skipping 19 matching lines @@
 
   password_generation::LogPasswordGenerationEvent(
       password_generation::EDITING_POPUP_SHOWN);
 }
 
 void PasswordGenerationAgent::HidePopup() {
   Send(new AutofillHostMsg_HidePasswordGenerationPopup(routing_id()));
 }
 
 }  // namespace autofill