Decision whenever "Allow to collect URL?" bubble should be shown or not is...

Decision whenever "Allow to collect URL?" bubble should be shown or not is made based on Finch experiment.

If user belongs to group for which bubble should be shown, then
we calculate a time span when bubble should appear, if it's current span, when we show bubble and saving in settings information that bubble was shown, so we'll never show bubble again. If it's not current time span when we do not show bubble this time.

BUG=435080
R=vabr@chromium.org

Review URL: https://codereview.chromium.org/789613004

Cr-Commit-Position: refs/heads/master@{#308170}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -135,6 +135,7 @@ bool ChromePasswordManagerClient::IsPasswordManagerEnabledForCurrentPage()
 
 bool ChromePasswordManagerClient::ShouldAskUserToSubmitURL(const GURL& url) {
   return url.is_valid() && !url.is_empty() && url.has_host() &&
+         password_manager_.IsSavingEnabledForCurrentPage() &&
          password_manager::urls_collection_experiment::ShouldShowBubble(
              GetPrefs());
 }

components/password_manager/core/browser/password_manager_url_collection_experiment.cc

@@ -5,40 +5,108 @@
 #include "components/password_manager/core/browser/password_manager_url_collection_experiment.h"
 
 #include "base/prefs/pref_service.h"
+#include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
+#include "components/variations/variations_associated_data.h"
 
 namespace password_manager {
 namespace urls_collection_experiment {
 
 namespace {
 
-bool ShouldShowBubbleExperiment(PrefService* prefs) {
-  // TODO(melandory): Make decision based on Finch experiment parameters.
-  return false;
+const char kExperimentStartDate[] = "Mon, 15 Dec 00:00:00 2014 GMT";
+
+// A safe default value. Using this will not show the bubble.
+const int kDefaultValueForStartingDayFactor = -1;
+
+bool ExtractExperimentParams(
+    int* experiment_length_in_days,
+    base::TimeDelta* experiment_active_for_user_period) {
+  std::map<std::string, std::string> params;
+  if (!variations::GetVariationParams(kExperimentName, &params))
+    return false;
+  int days = 0;
+  if (!base::StringToInt(params[kParamExperimentLengthInDays],
+                         experiment_length_in_days) ||
+      !base::StringToInt(params[kParamActivePeriodInDays], &days)) {
+    return false;
+  }
+  *experiment_active_for_user_period = base::TimeDelta::FromDays(days);
+  return true;
+}
+
+bool ShouldShowBubbleInternal(PrefService* prefs, base::Clock* clock) {
+  if (prefs->GetBoolean(prefs::kAllowToCollectURLBubbleWasShown))
+    return false;
+  int experiment_length_in_days = 0;
+  base::TimeDelta experiment_active_for_user_period;
+  if (!ExtractExperimentParams(&experiment_length_in_days,
+                               &experiment_active_for_user_period)) {
+    return false;
+  }
+  base::Time now = clock->Now();
+  base::Time experiment_active_for_user_start =
+      DetermineStartOfActivityPeriod(prefs, experiment_length_in_days);
+  return (now >= experiment_active_for_user_start &&
+          now < experiment_active_for_user_start +
+                    experiment_active_for_user_period);
 }
 
 }  // namespace
 
+const char kExperimentName[] = "AskToSubmitURLBubble";
+const char kParamExperimentLengthInDays[] = "experiment_length_in_days";
+const char kParamActivePeriodInDays[] = "experiment_active_for_user_period";
+const char kParamBubbleStatus[] = "show_bubble_status";
+
 void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(
-      password_manager::prefs::kWasAllowToCollectURLBubbleShown,
+      password_manager::prefs::kAllowToCollectURLBubbleWasShown,
       false,  // bubble hasn't been shown yet
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterDoublePref(
+      password_manager::prefs::kAllowToCollectURLBubbleActivePeriodStartFactor,
+      kDefaultValueForStartingDayFactor,
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
-const char kExperimentName[] = "AskToSubmitURLBubble";
-
-bool ShouldShowBubble(PrefService* prefs) {
-  if (prefs->GetBoolean(prefs::kWasAllowToCollectURLBubbleShown)) {
-    return ShouldShowBubbleExperiment(prefs);
+base::Time DetermineStartOfActivityPeriod(PrefService* prefs,
+                                          int experiment_length_in_days) {
+  double active_period_start_second_factor =
+      prefs->GetDouble(prefs::kAllowToCollectURLBubbleActivePeriodStartFactor);
+  if (active_period_start_second_factor == kDefaultValueForStartingDayFactor) {
+    active_period_start_second_factor = base::RandDouble();
+    prefs->SetDouble(prefs::kAllowToCollectURLBubbleActivePeriodStartFactor,
+                     active_period_start_second_factor);
   }
+  base::Time beginning;
+  base::Time::FromString(kExperimentStartDate, &beginning);
+  const int kSecondsInDay = 24 * 60 * 60;
+  return beginning + base::TimeDelta::FromSeconds(
+                         kSecondsInDay * experiment_length_in_days *
+                         active_period_start_second_factor);
+}
+
+bool ShouldShowBubbleWithClock(PrefService* prefs, base::Clock* clock) {
+  std::string show_bubble =
+      variations::GetVariationParamValue(kExperimentName, kParamBubbleStatus);
+  if (show_bubble == "show_bubble")
+    return ShouldShowBubbleInternal(prefs, clock);
   // "Do not show" is the default case.
   return false;
 }
 
+bool ShouldShowBubble(PrefService* prefs) {
+  base::DefaultClock clock;
+  return ShouldShowBubbleWithClock(prefs, &clock);
+}
+
 void RecordBubbleClosed(PrefService* prefs) {
-  prefs->SetBoolean(password_manager::prefs::kWasAllowToCollectURLBubbleShown,
+  prefs->SetBoolean(password_manager::prefs::kAllowToCollectURLBubbleWasShown,
                     true);
 }
 

components/password_manager/core/browser/password_manager_url_collection_experiment.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_URL_COLLECTION_EXPERIMENT_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_MANAGER_URL_COLLECTION_EXPERIMENT_H_
 
+#include "base/time/time.h"
+
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
@@ -18,11 +20,14 @@ namespace urls_collection_experiment {
 
 void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry);
 
+// Implements an algorithm determining when the period starts, in which "Allow
+// to collect URL?" bubble can be shown.
+base::Time DetermineStartOfActivityPeriod(PrefService* prefs,
+                                          int experiment_length_in_days);
 // Based on |prefs| and experiment settings, decides whether to show the
 // "Allow to collect URL?" bubble and should be called before showing it.
 // The default value is false.
 bool ShouldShowBubble(PrefService* prefs);
-
 // Should be called when user dismisses the "Allow to collect URL?" bubble.
 // It stores the statistics about interactions with the bubble in |prefs|.
 void RecordBubbleClosed(PrefService* prefs);
@@ -30,6 +35,17 @@ void RecordBubbleClosed(PrefService* prefs);
 // The name of the finch experiment controlling the algorithm.
 extern const char kExperimentName[];
 
+// The name of the experiment parameter, value of which determines determines
+// how long the experiment is active.
+extern const char kParamExperimentLengthInDays[];
+
+// The bubble is shown only once and only within a certain period. The length of
+// the period is the value of the experiment parameter |kParamTimePeriodInDays|.
+extern const char kParamActivePeriodInDays[];
+/// The name of the experiment parameter, value of which defines whether
+// the bubble should appear or not.
+extern const char kParamBubbleStatus[];
+
 }  // namespace urls_collection_experiment
 }  // namespace password_manager
 

components/password_manager/core/browser/password_manager_url_collection_experiment_unittest.cc

@@ -4,48 +4,120 @@
 
 #include "components/password_manager/core/browser/password_manager_url_collection_experiment.h"
 
-#include "base/files/scoped_temp_dir.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/testing_pref_service.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/test/simple_test_clock.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/variations/entropy_provider.h"
+#include "components/variations/variations_associated_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace password_manager {
+namespace urls_collection_experiment {
+
+namespace {
+
+int kParamActivePeriodInDaysValue = 7;
+int kParamExperimentLengthInDaysValue = 365;
+const char kParamBubbleStatusValue[] = "show_bubble";
+const char kGroupShowBubbleWithClock[] = "ShowBubbleWithClock";
+const char kGroupNeverShowBubbleWithClock[] = "NeverShowBubbleWithClock";
+
+void SetupShowBubbleWithClockExperimentGroup() {
+  ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+      kExperimentName, kGroupShowBubbleWithClock));
+  std::map<std::string, std::string> params;
+  params[kParamActivePeriodInDays] =
+      base::IntToString(kParamActivePeriodInDaysValue);
+  params[kParamExperimentLengthInDays] =
+      base::IntToString(kParamExperimentLengthInDaysValue);
+  params[kParamBubbleStatus] = kParamBubbleStatusValue;
+  ASSERT_TRUE(variations::AssociateVariationParams(
+      kExperimentName, kGroupShowBubbleWithClock, params));
+}
+
+void SetupNeverShowBubbleWithClockExperimentGroup() {
+  ASSERT_TRUE(base::FieldTrialList::CreateFieldTrial(
+      kExperimentName, kGroupNeverShowBubbleWithClock));
+}
+
+}  // namespace
+
+// Same as ShouldShowBubble(PrefService* prefs), but specifies a mock
+// interface for clock functions for testing.
+bool ShouldShowBubbleWithClock(PrefService* prefs, base::Clock* clock);
+
 class PasswordManagerUrlsCollectionExperimentTest : public testing::Test {
  public:
+  PasswordManagerUrlsCollectionExperimentTest()
+      : field_trial_list_(new metrics::SHA1EntropyProvider("foo")) {}
+
   void SetUp() override {
     pref_service_.registry()->RegisterBooleanPref(
-        password_manager::prefs::kWasAllowToCollectURLBubbleShown, false);
+        prefs::kAllowToCollectURLBubbleWasShown, false);
+    pref_service_.registry()->RegisterDoublePref(
+        prefs::kAllowToCollectURLBubbleActivePeriodStartFactor, -1);
+  }
+  void TearDown() override { variations::testing::ClearAllVariationParams(); }
+
+  void SetTimeToBubbleActivePeriod(int experiment_length_in_days) {
+    base::Time starting_time =
+        DetermineStartOfActivityPeriod(prefs(), experiment_length_in_days);
+    test_clock()->SetNow(starting_time);
+  }
+
+  void SetTimePastBubbleActivePeriod(int experiment_length_in_days) {
+    base::Time starting_time =
+        DetermineStartOfActivityPeriod(prefs(), experiment_length_in_days);
+    test_clock()->SetNow(starting_time + base::TimeDelta::FromDays(
+                                             kParamActivePeriodInDaysValue));
+  }
+
+  void PretendBubbleWasAlreadyShown() {
+    prefs()->SetBoolean(prefs::kAllowToCollectURLBubbleWasShown, true);
   }
 
   PrefService* prefs() { return &pref_service_; }
 
+  base::SimpleTestClock* test_clock() { return &test_clock_; }
+
  private:
   TestingPrefServiceSimple pref_service_;
+  base::FieldTrialList field_trial_list_;
+  base::SimpleTestClock test_clock_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordManagerUrlsCollectionExperimentTest);
 };
 
-TEST_F(PasswordManagerUrlsCollectionExperimentTest, TestDefault) {
-  EXPECT_FALSE(
-      password_manager::urls_collection_experiment::ShouldShowBubble(prefs()));
+TEST_F(PasswordManagerUrlsCollectionExperimentTest,
+       TestShowBubbleWithClockGroupTimeSpanWhenBubbleShouldAppear) {
+  SetupShowBubbleWithClockExperimentGroup();
+  SetTimeToBubbleActivePeriod(kParamExperimentLengthInDaysValue);
+  EXPECT_TRUE(ShouldShowBubbleWithClock(prefs(), test_clock()));
 }
 
-TEST_F(PasswordManagerUrlsCollectionExperimentTest, TestMaybeShowBubbleGroup) {
-  // TODO(melandory) This test case should be rewritten when decision about
-  // should bubble be shown or not will be made based on Finch experiment
-  // http://crbug.com/435080.
-  EXPECT_FALSE(
-      password_manager::urls_collection_experiment::ShouldShowBubble(prefs()));
+TEST_F(PasswordManagerUrlsCollectionExperimentTest,
+       TestShowBubbleWithClockGroupTimeSpanWhenBubbleShouldNotAppear) {
+  SetupShowBubbleWithClockExperimentGroup();
+  SetTimePastBubbleActivePeriod(kParamExperimentLengthInDaysValue);
+  EXPECT_FALSE(ShouldShowBubbleWithClock(prefs(), test_clock()));
 }
 
-TEST_F(PasswordManagerUrlsCollectionExperimentTest, TestNeverShowBubbleGroup) {
-  EXPECT_FALSE(
-      password_manager::urls_collection_experiment::ShouldShowBubble(prefs()));
+TEST_F(PasswordManagerUrlsCollectionExperimentTest,
+       TestNeverShowBubbleWithClockGroup) {
+  SetupNeverShowBubbleWithClockExperimentGroup();
+  SetTimeToBubbleActivePeriod(kParamExperimentLengthInDaysValue);
+  EXPECT_FALSE(ShouldShowBubbleWithClock(prefs(), test_clock()));
 }
 
 TEST_F(PasswordManagerUrlsCollectionExperimentTest, TestBubbleWasAlreadyShown) {
-  prefs()->SetBoolean(password_manager::prefs::kWasAllowToCollectURLBubbleShown,
-                      true);
-  EXPECT_FALSE(
-      password_manager::urls_collection_experiment::ShouldShowBubble(prefs()));
+  SetupShowBubbleWithClockExperimentGroup();
+  SetTimeToBubbleActivePeriod(kParamExperimentLengthInDaysValue);
+  PretendBubbleWasAlreadyShown();
+  EXPECT_FALSE(ShouldShowBubbleWithClock(prefs(), test_clock()));
 }
+
+}  //  namespace urls_collection_experiment
+}  //  namespace password_manager

components/password_manager/core/common/password_manager_pref_names.cc

@@ -43,10 +43,14 @@ const char kPasswordManagerGroupsForDomains[] =
 const char kLocalProfileId[] = "profile.local_profile_id";
 #endif
 
-// Boolean that indicates whether "Allow to collect URL?" bubble was shown or
-// not.
-const char kWasAllowToCollectURLBubbleShown[] =
+// The value of this parameter is boolean that indicates whether
+// "Allow to collect URL?" bubble was shown or not.
+const char kAllowToCollectURLBubbleWasShown[] =
     "password_manager_url_collection_bubble.appearance_flag";
+// The value of this parameter is used to calculate the start day of the
+// period, in which the "Allow to collect URL?" bubble can be shown.
+const char kAllowToCollectURLBubbleActivePeriodStartFactor[] =
+    "password_manager_url_collection_bubble.active_period_start_id";
 }  // namespace prefs
 
 }  // namespace password_manager

components/password_manager/core/common/password_manager_pref_names.h

@@ -25,7 +25,9 @@ extern const char kPasswordManagerGroupsForDomains[];
 extern const char kLocalProfileId[];
 #endif
 
-extern const char kWasAllowToCollectURLBubbleShown[];
+extern const char kAllowToCollectURLBubbleWasShown[];
+extern const char kAllowToCollectURLBubbleActivePeriodStartFactor[];
+
 }  // namespace prefs
 
 }  // namespace password_manager