Set the minimum length of the generated password to 4.

Dismiss the generation prompt when > 5 characters typed.

Bug: 869890,859472
Change-Id: Ib842b92857a79e92e411e7247ad0cc4a322dd364
Reviewed-on: https://chromium-review.googlesource.com/1167507Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581885}

chrome/renderer/autofill/password_generation_agent_browsertest.cc

@@ -520,75 +520,114 @@ TEST_F(PasswordGenerationAgentTest, AccountCreationFormsDetectedTest) {
   ExpectAutomaticGenerationAvailable("first_password", true);
 }
 
-TEST_F(PasswordGenerationAgentTest, MaximumOfferSize) {
-  for (size_t maximum_offer_size : {0, 5}) {
-    SCOPED_TRACE(testing::Message()
-                 << "maximum_offer_size = " << maximum_offer_size);
-    password_generation_->set_maximum_offer_size_for_testing(
-        maximum_offer_size);
-    EXPECT_EQ(maximum_offer_size, password_generation_->maximum_offer_size());
-
-    base::HistogramTester histogram_tester;
-
-    LoadHTMLWithUserGesture(kAccountCreationFormHTML);
-    SetNotBlacklistedMessage(password_generation_, kAccountCreationFormHTML);
-    SetAccountCreationFormsDetectedMessage(password_generation_,
-                                           GetMainFrame()->GetDocument(), 0, 1);
-    // There should now be a message to show the UI.
-    ExpectAutomaticGenerationAvailable("first_password", true);
+TEST_F(PasswordGenerationAgentTest, MaximumCharsForGenerationOffer) {
+  base::HistogramTester histogram_tester;
 
-    WebDocument document = GetMainFrame()->GetDocument();
-    WebElement element =
-        document.GetElementById(WebString::FromUTF8("first_password"));
-    ASSERT_FALSE(element.IsNull());
-    WebInputElement first_password_element = element.To<WebInputElement>();
-
-    // Make a password just under maximum offer size.
-    if (password_generation_->maximum_offer_size() > 0) {
-      SimulateUserInputChangeForElement(
-          &first_password_element,
-          std::string(password_generation_->maximum_offer_size(), 'a'));
-
-      // There should still be a message to show the UI.
-      EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
-      fake_pw_client_.reset_called_automatic_generation_status_changed_true();
-      fake_pw_client_.reset_called_password_generation_rejected_by_typing();
-    }
+  LoadHTMLWithUserGesture(kAccountCreationFormHTML);
+  SetNotBlacklistedMessage(password_generation_, kAccountCreationFormHTML);
+  SetAccountCreationFormsDetectedMessage(password_generation_,
+                                         GetMainFrame()->GetDocument(), 0, 1);
+  // There should now be a message to show the UI.
+  ExpectAutomaticGenerationAvailable("first_password", true);
 
-    // Simulate a user typing a password just over maximum offer size.
-    SimulateUserTypingASCIICharacter('a', true);
-    // There should now be a message that generation was rejected.
-    fake_pw_client_.Flush();
-    EXPECT_TRUE(
-        fake_pw_client_.called_password_generation_rejected_by_typing());
-    fake_pw_client_.reset_called_show_manual_pw_generation_popup();
+  WebDocument document = GetMainFrame()->GetDocument();
+  WebElement element =
+      document.GetElementById(WebString::FromUTF8("first_password"));
+  ASSERT_FALSE(element.IsNull());
+  WebInputElement first_password_element = element.To<WebInputElement>();
 
-    // Simulate the user deleting characters. The generation popup should be
-    // shown again.
-    SimulateUserTypingASCIICharacter(ui::VKEY_BACK, true);
-    // There should now be a message to show the UI.
-    EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
-    fake_pw_client_.reset_called_automatic_generation_status_changed_true();
+  // Make a password just under maximum offer size.
+  SimulateUserInputChangeForElement(
+      &first_password_element,
+      std::string(PasswordGenerationAgent::kMaximumCharsForGenerationOffer,
+                  'a'));
 
-    // Change focus. Bubble should be hidden, but that is handled by
-    // AutofilAgent, so no messages are sent.
-    ExecuteJavaScriptForTests("document.getElementById('username').focus();");
-    EXPECT_FALSE(GetCalledAutomaticGenerationStatusChangedTrue());
+  // There should still be a message to show the UI.
+  EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
+  fake_pw_client_.reset_called_automatic_generation_status_changed_true();
+  fake_pw_client_.reset_called_password_generation_rejected_by_typing();
 
-    // Focusing the password field will bring up the generation UI again.
-    ExecuteJavaScriptForTests(
-        "document.getElementById('first_password').focus();");
-    EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
-    fake_pw_client_.reset_called_automatic_generation_status_changed_true();
+  // Simulate a user typing a password just over maximum offer size.
+  SimulateUserTypingASCIICharacter('a', true);
+  // There should now be a message that generation was rejected.
+  fake_pw_client_.Flush();
+  EXPECT_TRUE(fake_pw_client_.called_password_generation_rejected_by_typing());
+  fake_pw_client_.reset_called_password_generation_rejected_by_typing();
 
-    // Loading a different page triggers UMA stat upload. Verify that only one
-    // display event is sent.
-    LoadHTMLWithUserGesture(kSigninFormHTML);
+  // Simulate the user deleting characters. The generation popup should be
+  // shown again.
+  SimulateUserTypingASCIICharacter(ui::VKEY_BACK, true);
+  // There should now be a message to show the UI.
+  EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
+  fake_pw_client_.reset_called_automatic_generation_status_changed_true();
 
-    histogram_tester.ExpectBucketCount(
-        "PasswordGeneration.Event",
-        autofill::password_generation::GENERATION_POPUP_SHOWN, 1);
-  }
+  // Change focus. Bubble should be hidden, but that is handled by
+  // AutofilAgent, so no messages are sent.
+  ExecuteJavaScriptForTests("document.getElementById('username').focus();");
+  EXPECT_FALSE(GetCalledAutomaticGenerationStatusChangedTrue());
+
+  // Focusing the password field will bring up the generation UI again.
+  ExecuteJavaScriptForTests(
+      "document.getElementById('first_password').focus();");
+  EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
+  fake_pw_client_.reset_called_automatic_generation_status_changed_true();
+
+  // Loading a different page triggers UMA stat upload. Verify that only one
+  // display event is sent.
+  LoadHTMLWithUserGesture(kSigninFormHTML);
+
+  histogram_tester.ExpectBucketCount(
+      "PasswordGeneration.Event",
+      autofill::password_generation::GENERATION_POPUP_SHOWN, 1);
+}
+
+TEST_F(PasswordGenerationAgentTest, MinimumLengthForEditedPassword) {
+  LoadHTMLWithUserGesture(kAccountCreationFormHTML);
+  SetNotBlacklistedMessage(password_generation_, kAccountCreationFormHTML);
+  SetAccountCreationFormsDetectedMessage(password_generation_,
+                                         GetMainFrame()->GetDocument(), 0, 1);
+
+  // Generate a new password.
+  base::string16 password = base::ASCIIToUTF16("random_password");
+  EXPECT_CALL(fake_pw_client_,
+              PresaveGeneratedPassword(testing::Field(
+                  &autofill::PasswordForm::password_value, password)));
+  password_generation_->GeneratedPasswordAccepted(password);
+  fake_pw_client_.Flush();
+  testing::Mock::VerifyAndClearExpectations(&fake_pw_client_);
+
+  // Delete most of the password.
+  FocusField("first_password");
+  size_t max_chars_to_delete =
+      password.length() -
+      PasswordGenerationAgent::kMinimumLengthForEditedPassword;
+  EXPECT_CALL(fake_pw_client_, PresaveGeneratedPassword(testing::_))
+      .Times(testing::AtLeast(1));
+  for (size_t i = 0; i < max_chars_to_delete; ++i)
+    SimulateUserTypingASCIICharacter(ui::VKEY_BACK, false);
+  fake_pw_client_.Flush();
+  testing::Mock::VerifyAndClearExpectations(&fake_pw_client_);
+  EXPECT_FALSE(GetCalledAutomaticGenerationStatusChangedTrue());
+
+  // Delete one more character. The state should move to offering generation.
+  EXPECT_CALL(fake_pw_client_, PasswordNoLongerGenerated(testing::_));
+  SimulateUserTypingASCIICharacter(ui::VKEY_BACK, true);
+  fake_pw_client_.Flush();
+  testing::Mock::VerifyAndClearExpectations(&fake_pw_client_);
+  EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
+
+  // The first password field is still non empty. The second one should be
+  // cleared.
+  WebDocument document = GetMainFrame()->GetDocument();
+  WebElement element =
+      document.GetElementById(WebString::FromUTF8("first_password"));
+  ASSERT_FALSE(element.IsNull());
+  WebInputElement first_password_element = element.To<WebInputElement>();
+  element = document.GetElementById(WebString::FromUTF8("second_password"));
+  ASSERT_FALSE(element.IsNull());
+  WebInputElement second_password_element = element.To<WebInputElement>();
+  EXPECT_NE(base::string16(), first_password_element.Value().Utf16());
+  EXPECT_EQ(base::string16(), second_password_element.Value().Utf16());
 }
 
 TEST_F(PasswordGenerationAgentTest, DynamicFormTest) {
@@ -876,7 +915,7 @@ TEST_F(PasswordGenerationAgentTest, RevealPassword) {
   const char* kTextFieldId = "username";
 
   ExpectAutomaticGenerationAvailable(kGenerationElementId, true);
-  base::string16 password = base::ASCIIToUTF16("pwd");
+  base::string16 password = base::ASCIIToUTF16("long_pwd");
   EXPECT_CALL(fake_pw_client_,
               PresaveGeneratedPassword(testing::Field(
                   &autofill::PasswordForm::password_value, password)));
@@ -911,7 +950,7 @@ TEST_F(PasswordGenerationAgentTest, JavascriptClearedTheField) {
 
   const char kGenerationElementId[] = "first_password";
   ExpectAutomaticGenerationAvailable(kGenerationElementId, true);
-  base::string16 password = base::ASCIIToUTF16("pwd");
+  base::string16 password = base::ASCIIToUTF16("long_pwd");
   EXPECT_CALL(fake_pw_client_,
               PresaveGeneratedPassword(testing::Field(
                   &autofill::PasswordForm::password_value, password)));

components/autofill/content/renderer/password_generation_agent.cc

@@ -10,9 +10,7 @@
 
 #include "base/command_line.h"
 #include "base/logging.h"
-#include "base/metrics/field_trial_params.h"
 #include "base/stl_util.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/autofill/content/renderer/form_autofill_util.h"
 #include "components/autofill/content/renderer/password_autofill_agent.h"
@@ -168,23 +166,6 @@ void CopyElementValueToOtherInputElements(
   }
 }
 
-// Returns the number of characters the user may type while password generation
-// is offered. Once the user has typed more than the given number, a password
-// generation popup is removed.
-// TODO(crbug.com/859472) Delete this function once we know a good value from
-// the field trial.
-size_t GetMaximumOfferSize() {
-  std::string string_value = base::GetFieldTrialParamValue(
-      "PasswordGenerationMaximumOfferSize", "maximum_offer_size");
-  size_t parsed_value = 0;
-  if (!base::StringToSizeT(string_value, &parsed_value)) {
-    // This is the historic default value and remains in place for the time
-    // being. The hypothesis is, though, that 0 would be a better default.
-    return 5;
-  }
-  return parsed_value;
-}
-
 }  // namespace
 
 PasswordGenerationAgent::AccountCreationFormData::AccountCreationFormData(
@@ -212,8 +193,7 @@ PasswordGenerationAgent::PasswordGenerationAgent(
           base::CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kShowAutofillSignatures)),
       password_agent_(password_agent),
-      binding_(this),
-      maximum_offer_size_(GetMaximumOfferSize()) {
+      binding_(this) {
   LogBoolean(Logger::STRING_GENERATION_RENDERER_ENABLED, enabled_);
   registry->AddInterface(base::BindRepeating(
       &PasswordGenerationAgent::BindRequest, base::Unretained(this)));
@@ -377,6 +357,9 @@ void PasswordGenerationAgent::FormNotBlacklisted(const PasswordForm& form) {
 
 void PasswordGenerationAgent::GeneratedPasswordAccepted(
     const base::string16& password) {
+  // static cast is workaround for linker error.
+  DCHECK_LE(static_cast<size_t>(kMinimumLengthForEditedPassword),
+            password.size());
   password_is_generated_ = true;
   password_edited_ = false;
   password_generation::LogPasswordGenerationEvent(
@@ -591,7 +574,8 @@ bool PasswordGenerationAgent::FocusedNodeHasChanged(
   }
 
   if (password_is_generated_) {
-    if (generation_element_.Value().IsEmpty()) {
+    if (generation_element_.Value().length() <
+        kMinimumLengthForEditedPassword) {
       PasswordNoLongerGenerated();
     } else {
       generation_element_.SetShouldRevealPassword(true);
@@ -600,11 +584,11 @@ bool PasswordGenerationAgent::FocusedNodeHasChanged(
     return true;
   }
 
-  // Assume that if the password field has less than maximum_offer_size_
-  // characters then the user is not finished typing their password and display
-  // the password suggestion.
+  // Assume that if the password field has less than
+  // |kMaximumCharsForGenerationOffer| characters then the user is not finished
+  // typing their password and display the password suggestion.
   if (!element->IsReadOnly() && element->IsEnabled() &&
-      element->Value().length() <= maximum_offer_size_) {
+      element->Value().length() <= kMaximumCharsForGenerationOffer) {
     MaybeOfferAutomaticGeneration();
     return true;
   }
@@ -627,32 +611,34 @@ bool PasswordGenerationAgent::TextDidChangeInTextField(
     return false;
   }
 
-  if (element.Value().IsEmpty()) {
-    // The call may pop up a generation prompt.
-    MaybeOfferAutomaticGeneration();
-    // Tell the browser that the state isn't "editing" anymore. The browser
-    // should hide the editing prompt if it wasn't replaced above.
-    if (password_is_generated_) {
-      // User generated a password and then deleted it.
-      PasswordNoLongerGenerated();
-    }
-  } else if (password_is_generated_) {
-    password_edited_ = true;
-    // Mirror edits to any confirmation password fields.
-    CopyElementValueToOtherInputElements(&element,
-        &generation_form_data_->password_elements);
-    std::unique_ptr<PasswordForm> presaved_form(CreatePasswordFormToPresave());
-    if (presaved_form) {
-      GetPasswordManagerClient()->PresaveGeneratedPassword(*presaved_form);
-    }
-  } else if (element.Value().length() > maximum_offer_size_) {
+  if (!password_is_generated_ &&
+      element.Value().length() > kMaximumCharsForGenerationOffer) {
     // User has rejected the feature and has started typing a password.
     GenerationRejectedByTyping();
   } else {
-    // Password isn't generated and there are fewer than maximum_offer_size_
-    // characters typed, so keep offering the password. Note this function
-    // will just keep the previous popup if one is already showing.
-    MaybeOfferAutomaticGeneration();
+    const bool leave_editing_state =
+        password_is_generated_ &&
+        element.Value().length() < kMinimumLengthForEditedPassword;
+    if (!password_is_generated_ || leave_editing_state) {
+      // The call may pop up a generation prompt, replacing the editing prompt
+      // if it was previously shown.
+      MaybeOfferAutomaticGeneration();
+    }
+    if (leave_editing_state) {
+      // Tell the browser that the state isn't "editing" anymore. The browser
+      // should hide the editing prompt if it wasn't replaced above.
+      PasswordNoLongerGenerated();
+    } else if (password_is_generated_) {
+      password_edited_ = true;
+      // Mirror edits to any confirmation password fields.
+      CopyElementValueToOtherInputElements(
+          &element, &generation_form_data_->password_elements);
+      std::unique_ptr<PasswordForm> presaved_form(
+          CreatePasswordFormToPresave());
+      if (presaved_form) {
+        GetPasswordManagerClient()->PresaveGeneratedPassword(*presaved_form);
+      }
+    }
   }
   return true;
 }
@@ -712,8 +698,11 @@ void PasswordGenerationAgent::PasswordNoLongerGenerated() {
     password.SetAutofillState(WebAutofillState::kNotFilled);
   password_generation::LogPasswordGenerationEvent(
       password_generation::PASSWORD_DELETED);
-  CopyElementValueToOtherInputElements(
-      &generation_element_, &generation_form_data_->password_elements);
+  // Clear all other password fields.
+  for (WebInputElement& element : generation_form_data_->password_elements) {
+    if (generation_element_ != element)
+      element.SetAutofillValue(blink::WebString());
+  }
   std::unique_ptr<PasswordForm> presaved_form(CreatePasswordFormToPresave());
   if (presaved_form)
     GetPasswordManagerClient()->PasswordNoLongerGenerated(*presaved_form);

components/autofill/content/renderer/password_generation_agent.h

@@ -35,6 +35,15 @@ class PasswordAutofillAgent;
 class PasswordGenerationAgent : public content::RenderFrameObserver,
                                 public mojom::PasswordGenerationAgent {
  public:
+  // Maximum number of characters typed by user while the generation is still
+  // offered. When the (kMaximumCharsForGenerationOffer + 1)-th character is
+  // typed, the generation becomes unavailable.
+  static const size_t kMaximumCharsForGenerationOffer = 5;
+
+  // User can edit the generated password. If the length falls below this value,
+  // the password is no longer considered generated.
+  static const size_t kMinimumLengthForEditedPassword = 4;
+
   PasswordGenerationAgent(content::RenderFrame* render_frame,
                           PasswordAutofillAgent* password_agent,
                           blink::AssociatedInterfaceRegistry* registry);
@@ -64,14 +73,7 @@ class PasswordGenerationAgent : public content::RenderFrameObserver,
   // Called right before PasswordAutofillAgent filled |password_element|.
   void OnFieldAutofilled(const blink::WebInputElement& password_element);
 
-  // The length that a password can be before the UI is hidden.
-  size_t maximum_offer_size() const { return maximum_offer_size_; }
-
 #if defined(UNIT_TEST)
-  void set_maximum_offer_size_for_testing(size_t maximum_offer_size) {
-    maximum_offer_size_ = maximum_offer_size;
-  }
-
   // This method requests the autofill::mojom::PasswordManagerClient which binds
   // requests the binding if it wasn't bound yet.
   void RequestPasswordManagerClientForTesting() { GetPasswordManagerClient(); }
@@ -228,9 +230,6 @@ class PasswordGenerationAgent : public content::RenderFrameObserver,
 
   mojo::AssociatedBinding<mojom::PasswordGenerationAgent> binding_;
 
-  // The length that a password can be before the UI is hidden.
-  size_t maximum_offer_size_;
-
   DISALLOW_COPY_AND_ASSIGN(PasswordGenerationAgent);
 };