Introduce Finch parameter for hiding a password generation popup

If a user focuses a field for which password generation is promoted, a popup is
shown.

http://crrev.com/288935 introduced that this popup remains visible until the
user has typed 5 characters, hypothesizing that users look at their keyboard
while typing and therefore not notice the popup.

Our new hypothesis is that this a) may not be true, and b) may bother users
because the popup occludes part of the website, in particular it can occlude
password requirements for users who chose to invent their own password.

This CL introduces a Finch experiement such that we can test whether keeping the
popup visible until the user has typed >5 characters produces a statistically
significant higher number of generated passwords. If not, we can just hide the
password generation popup as soon as the user has typed a key.

The default configuration sticks to the 5 character threshold.

Bug: 859472
Change-Id: I1d2a82ce90f696dc32d97633ac29ba15f3ea9bbd
Reviewed-on: https://chromium-review.googlesource.com/1122399
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572131}

chrome/renderer/autofill/password_generation_agent_browsertest.cc

@@ -11,7 +11,6 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
-#include "base/test/scoped_feature_list.h"
 #include "chrome/renderer/autofill/fake_content_password_manager_driver.h"
 #include "chrome/renderer/autofill/fake_password_manager_client.h"
 #include "chrome/renderer/autofill/password_generation_test_utils.h"
@@ -153,8 +152,6 @@ class PasswordGenerationAgentTest : public ChromeRenderViewTest {
   FakePasswordManagerClient fake_pw_client_;
 
  private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-
   DISALLOW_COPY_AND_ASSIGN(PasswordGenerationAgentTest);
 };
 
@@ -518,63 +515,74 @@ TEST_F(PasswordGenerationAgentTest, AccountCreationFormsDetectedTest) {
 }
 
 TEST_F(PasswordGenerationAgentTest, MaximumOfferSize) {
-  base::HistogramTester histogram_tester;
-
-  LoadHTMLWithUserGesture(kAccountCreationFormHTML);
-  SetNotBlacklistedMessage(password_generation_, kAccountCreationFormHTML);
-  SetAccountCreationFormsDetectedMessage(password_generation_,
-                                         GetMainFrame()->GetDocument(), 0, 1);
-  ExpectAutomaticGenerationAvailable("first_password", true);
-
-  WebDocument document = GetMainFrame()->GetDocument();
-  WebElement element =
-      document.GetElementById(WebString::FromUTF8("first_password"));
-  ASSERT_FALSE(element.IsNull());
-  WebInputElement first_password_element = element.To<WebInputElement>();
+  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);
 
-  // Make a password just under maximum offer size.
-  SimulateUserInputChangeForElement(
-      &first_password_element,
-      std::string(password_generation_->kMaximumOfferSize - 1, 'a'));
-  // There should now be a message to show the UI.
-  EXPECT_TRUE(GetCalledAutomaticGenerationStatusChangedTrue());
-  fake_pw_client_.reset_called_automatic_generation_status_changed_true();
+    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();
+    }
 
-  fake_pw_client_.reset_called_password_generation_rejected_by_typing();
-  // Simulate a user typing a password just over maximum offer size.
-  SimulateUserTypingASCIICharacter('a', false);
-  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();
+    // 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();
 
-  // 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();
+    // 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();
 
-  // 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());
+    // 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();
+    // 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);
+    // 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);
+    histogram_tester.ExpectBucketCount(
+        "PasswordGeneration.Event",
+        autofill::password_generation::GENERATION_POPUP_SHOWN, 1);
+  }
 }
 
 TEST_F(PasswordGenerationAgentTest, DynamicFormTest) {

components/autofill/content/renderer/password_generation_agent.cc

@@ -10,7 +10,9 @@
 
 #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/form_classifier.h"
@@ -163,6 +165,23 @@ 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(
@@ -191,7 +210,8 @@ PasswordGenerationAgent::PasswordGenerationAgent(
           base::CommandLine::ForCurrentProcess()->HasSwitch(
               switches::kShowAutofillSignatures)),
       password_agent_(password_agent),
-      binding_(this) {
+      binding_(this),
+      maximum_offer_size_(GetMaximumOfferSize()) {
   LogBoolean(Logger::STRING_GENERATION_RENDERER_ENABLED, enabled_);
   registry->AddInterface(base::Bind(&PasswordGenerationAgent::BindRequest,
                                     base::Unretained(this)));
@@ -576,11 +596,11 @@ bool PasswordGenerationAgent::FocusedNodeHasChanged(
     return true;
   }
 
-  // Assume that if the password field has less than kMaximumOfferSize
+  // 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.
   if (!element->IsReadOnly() && element->IsEnabled() &&
-      element->Value().length() <= kMaximumOfferSize) {
+      element->Value().length() <= maximum_offer_size_) {
     MaybeOfferAutomaticGeneration();
     return true;
   }
@@ -620,11 +640,11 @@ bool PasswordGenerationAgent::TextDidChangeInTextField(
     if (presaved_form) {
       GetPasswordManagerClient()->PresaveGeneratedPassword(*presaved_form);
     }
-  } else if (element.Value().length() > kMaximumOfferSize) {
+  } else if (element.Value().length() > maximum_offer_size_) {
     // User has rejected the feature and has started typing a password.
     GenerationRejectedByTyping();
   } else {
-    // Password isn't generated and there are fewer than kMaximumOfferSize
+    // 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();

components/autofill/content/renderer/password_generation_agent.h

@@ -68,7 +68,13 @@ class PasswordGenerationAgent : public content::RenderFrameObserver,
   void OnFieldAutofilled(const blink::WebInputElement& password_element);
 
   // The length that a password can be before the UI is hidden.
-  static const size_t kMaximumOfferSize = 5;
+  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;
+  }
+#endif
 
  protected:
   // Returns true if the document for |render_frame()| is one that we should
@@ -227,6 +233,9 @@ class PasswordGenerationAgent : public content::RenderFrameObserver,
 
   mojo::Binding<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);
 };