New smart passsword bubble algorithm.

chrome/browser/ui/passwords/password_bubble_experiment.cc

Index: chrome/browser/ui/passwords/password_bubble_experiment.cc
diff --git a/chrome/browser/ui/passwords/password_bubble_experiment.cc b/chrome/browser/ui/passwords/password_bubble_experiment.cc
index 276a038e6a4e20cef8503e302217bc8bb390dbad..dc1bcc1312fb1065fb33beab4a873392d6f481e7 100644
--- a/chrome/browser/ui/passwords/password_bubble_experiment.cc
+++ b/chrome/browser/ui/passwords/password_bubble_experiment.cc
@@ -6,9 +6,7 @@
 
 #include "base/metrics/field_trial.h"
 #include "base/prefs/pref_service.h"
-#include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/time/time.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/variations/variations_associated_data.h"
@@ -16,190 +14,44 @@
 namespace password_bubble_experiment {
 namespace {
 
-bool IsNegativeEvent(password_manager::metrics_util::UIDismissalReason reason) {
-  return (reason == password_manager::metrics_util::NO_DIRECT_INTERACTION ||
-          reason == password_manager::metrics_util::CLICKED_NOPE ||
-          reason == password_manager::metrics_util::CLICKED_NEVER);
-}
-
-// "TimeSpan" experiment -----------------------------------------------------
-
-bool ExtractTimeSpanParams(base::TimeDelta* time_delta, int* nopes_limit) {
-  std::map<std::string, std::string> params;
-  bool retrieved = variations::GetVariationParams(kExperimentName, &params);
-  if (!retrieved)
-    return false;
-  int days = 0;
-  if (!base::StringToInt(params[kParamTimeSpan], &days) ||
-      !base::StringToInt(params[kParamTimeSpanNopeThreshold], nopes_limit))
-    return false;
-  *time_delta = base::TimeDelta::FromDays(days);
-  return true;
-}
-
-bool OverwriteTimeSpanPrefsIfNeeded(PrefService* prefs,
-                                    base::TimeDelta time_span) {
-  base::Time beginning = base::Time::FromInternalValue(
-      prefs->GetInt64(prefs::kPasswordBubbleTimeStamp));
-  base::Time now = base::Time::Now();
-  if (beginning + time_span < now) {
-    prefs->SetInt64(prefs::kPasswordBubbleTimeStamp, now.ToInternalValue());
-    prefs->SetInteger(prefs::kPasswordBubbleNopesCount, 0);
-    return true;
-  }
-  return false;
-}
-
-// If user dismisses the bubble >= kParamTimeSpanNopeThreshold times during
-// kParamTimeSpan days then the bubble isn't shown until the end of this time
-// span.
-bool ShouldShowBubbleTimeSpanExperiment(PrefService* prefs) {
-  base::TimeDelta time_span;
-  int nopes_limit = 0;
-  if (!ExtractTimeSpanParams(&time_span, &nopes_limit)) {
-    VLOG(2) << "Can't read parameters for "
-        << kExperimentName  << " experiment";
-    return true;
-  }
-  // Check if the new time span has started.
-  if (OverwriteTimeSpanPrefsIfNeeded(prefs, time_span))
-    return true;
-  int current_nopes = prefs->GetInteger(prefs::kPasswordBubbleNopesCount);
-  return current_nopes < nopes_limit;
-}
-
-// Increase the "Nope" counter in prefs and start a new time span if needed.
-void UpdateTimeSpanPrefs(
-    PrefService* prefs,
-    password_manager::metrics_util::UIDismissalReason reason) {
-  if (!IsNegativeEvent(reason))
-    return;
-  base::TimeDelta time_span;
-  int nopes_limit = 0;
-  if (!ExtractTimeSpanParams(&time_span, &nopes_limit)) {
-    VLOG(2) << "Can't read parameters for "
-        << kExperimentName << " experiment";
-    return;
-  }
-  OverwriteTimeSpanPrefsIfNeeded(prefs, time_span);
-  int current_nopes = prefs->GetInteger(prefs::kPasswordBubbleNopesCount);
-  prefs->SetInteger(prefs::kPasswordBubbleNopesCount, current_nopes + 1);
-}
-
-// "Probability" experiment --------------------------------------------------
-
-bool ExtractProbabilityParams(unsigned* history_length, unsigned* saves) {
-  std::map<std::string, std::string> params;
-  bool retrieved = variations::GetVariationParams(kExperimentName, &params);
-  if (!retrieved)
-    return false;
-  return base::StringToUint(params[kParamProbabilityInteractionsCount],
-                            history_length) &&
-      base::StringToUint(params[kParamProbabilityFakeSaves], saves);
-}
-
-std::vector<int> ReadInteractionHistory(PrefService* prefs) {
-  std::vector<int> interactions;
-  const base::ListValue* list =
-      prefs->GetList(prefs::kPasswordBubbleLastInteractions);
-  if (!list)
-    return interactions;
-  for (const base::Value* value : *list) {
-    int out_value;
-    if (value->GetAsInteger(&out_value))
-      interactions.push_back(out_value);
-  }
-  return interactions;
-}
-
-// We keep the history of last kParamProbabilityInteractionsCount interactions
-// with the bubble. We implicitly add kParamProbabilityFakeSaves "Save" clicks.
-// If there are x "Save" clicks among those kParamProbabilityInteractionsCount
-// then the bubble is shown with probability (x + kParamProbabilityFakeSaves)/
-// (kParamProbabilityInteractionsCount + kParamProbabilityFakeSaves).
-bool ShouldShowBubbleProbabilityExperiment(PrefService* prefs) {
-  unsigned history_length = 0, fake_saves = 0;
-  if (!ExtractProbabilityParams(&history_length, &fake_saves)) {
-    VLOG(2) << "Can't read parameters for "
-        << kExperimentName << " experiment";
-    return true;
-  }
-  std::vector<int> interactions = ReadInteractionHistory(prefs);
-  unsigned real_saves =
-      std::count(interactions.begin(), interactions.end(),
-                 password_manager::metrics_util::CLICKED_SAVE);
-  return (interactions.size() + fake_saves) * base::RandDouble() <=
-      real_saves + fake_saves;
-}
-
-void UpdateProbabilityPrefs(
-    PrefService* prefs,
-    password_manager::metrics_util::UIDismissalReason reason) {
-  if (!IsNegativeEvent(reason) &&
-      reason != password_manager::metrics_util::CLICKED_SAVE)
-    return;
-  unsigned history_length = 0, fake_saves = 0;
-  if (!ExtractProbabilityParams(&history_length, &fake_saves)) {
-    VLOG(2) << "Can't read parameters for "
-        << kExperimentName << " experiment";
-    return;
-  }
-  std::vector<int> interactions = ReadInteractionHistory(prefs);
-  interactions.push_back(reason);
-  size_t history_beginning = interactions.size() > history_length ?
-      interactions.size() - history_length : 0;
-  base::ListValue value;
-  for (size_t i = history_beginning; i < interactions.size(); ++i)
-    value.AppendInteger(interactions[i]);
-  prefs->Set(prefs::kPasswordBubbleLastInteractions, value);
+// Return the number of consecutive times the user clicked "Not now" in the
+// "Save password" bubble.
+int GetCurrentNopesCount(PrefService* prefs) {
+  return prefs->GetInteger(prefs::kPasswordBubbleNopesCount);
 }
 
 } // namespace
 
 const char kExperimentName[] = "PasswordBubbleAlgorithm";
-const char kGroupTimeSpanBased[] = "TimeSpan";
-const char kGroupProbabilityBased[] = "Probability";
-const char kParamProbabilityFakeSaves[] = "saves_count";
-const char kParamProbabilityInteractionsCount[] = "last_interactions_count";
-const char kParamTimeSpan[] = "time_span";
-const char kParamTimeSpanNopeThreshold[] = "nope_threshold";
+const char kParamNopeThreshold[] = "consecutive_nope_threshold";
 
 void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry) {
-  registry->RegisterInt64Pref(
-      prefs::kPasswordBubbleTimeStamp,
-      0,
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   registry->RegisterIntegerPref(
       prefs::kPasswordBubbleNopesCount,
       0,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-  registry->RegisterListPref(
-      prefs::kPasswordBubbleLastInteractions,
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
-bool ShouldShowBubble(PrefService* prefs) {
+bool ShouldShowNeverForThisSiteDefault(PrefService* prefs) {
   if (!base::FieldTrialList::TrialExists(kExperimentName))
-    return true;
-  std::string group_name =
-      base::FieldTrialList::FindFullName(kExperimentName);
-
-  if (group_name == kGroupTimeSpanBased) {
-    return ShouldShowBubbleTimeSpanExperiment(prefs);
-  }
-  if (group_name == kGroupProbabilityBased) {
-    return ShouldShowBubbleProbabilityExperiment(prefs);
-  }
+    return false;
+  std::string param =
+      variations::GetVariationParamValue(kExperimentName, kParamNopeThreshold);
 
-  // The "Show Always" should be the default case.
-  return true;
+  int threshold = 0;
+  return base::StringToInt(param, &threshold) &&
+      GetCurrentNopesCount(prefs) >= threshold;
 }
 
 void RecordBubbleClosed(
     PrefService* prefs,
     password_manager::metrics_util::UIDismissalReason reason) {
-  UpdateTimeSpanPrefs(prefs, reason);
-  UpdateProbabilityPrefs(prefs, reason);
+  int nopes_count = GetCurrentNopesCount(prefs);
+  if (reason == password_manager::metrics_util::CLICKED_NOPE)
+    nopes_count++;
+  else
+    nopes_count = 0;

[vabr (Chromium), 2015/03/24 09:09:04]

I'm not sure why clicking "Never" or just abandoning the bubble
(metrics_util::NO_DIRECT_INTERACTION) should reset the nopes_count. What were
the reasons behind that?

[vasilii, 2015/03/24 10:30:53]

The case of NO_DIRECT_INTERACTION was discussed with Sabine. This experiment is
just temporary thing. Later we should implement this logic on a per domain
basis. So we don't want to opt out our users aggressively. Another point is that
NO_DIRECT_INTERACTION is not a strong signal. For example a page may contain a
redirect which took > 1 second. In this case user maybe wanted to save the
password but we closed it too fast.
"Never" wasn't discussed but it's my interpretation. Users which user "Never"
are not a target of this experiment. They may meaningfully decide when they want
to blacklist the site. Therefore, they don't need our hints.

[vabr (Chromium), 2015/03/24 10:40:59]

Fair enough for NO_DIRECT_INTERACTION. To be clear, I was not suggesting to
change nopes_count for NO_DIRECT_INTERACTION at all (in particular not to
increase it), so the redirect use-case would not be an option. But if you
discussed it, and it will be reconsidered once the experimental phase is over,
that's fine.

For clicking Never, I'm concerned about the users who only came across never
through Never becoming default after clicking Nope x-times. The current code
would switch back to Nope after the first time such Never is clicked, which
might seem like a glitch to the user, who did not discover the non-default parts
of the negative button (did it offer Never just while ago?).

[vasilii, 2015/03/24 11:00:04]

I changed the algorithm and the test.

+  prefs->SetInteger(prefs::kPasswordBubbleNopesCount, nopes_count);
 }
 
 }  // namespace password_bubble_experiment