Simplify the default browser infobar.

chrome/browser/ui/startup/default_browser_prompt.cc

Index: chrome/browser/ui/startup/default_browser_prompt.cc
diff --git a/chrome/browser/ui/startup/default_browser_prompt.cc b/chrome/browser/ui/startup/default_browser_prompt.cc
index 268c76fb501a913cad018ca12db534f7b13f110d..fac916d21e648cbb4392574805163ab03dbcde52 100644
--- a/chrome/browser/ui/startup/default_browser_prompt.cc
+++ b/chrome/browser/ui/startup/default_browser_prompt.cc
@@ -6,13 +6,16 @@
 
 #include <string>
 
+#include "base/feature_list.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics_action.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "base/version.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
@@ -29,6 +32,7 @@
 #include "components/infobars/core/infobar.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/variations/variations_associated_data.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/user_metrics.h"
@@ -39,51 +43,55 @@
 
 namespace {
 
+const base::Feature kInfobarRefreshFeature{"InfobarRefresh",

[msw, 2016/03/08 18:01:02]

nit: add a comment here? what does infobar refresh mean?

[grt (UTC plus 2), 2016/03/11 16:04:31]

Done.

+                                           base::FEATURE_DISABLED_BY_DEFAULT};
+
 // The delegate for the infobar shown when Chrome is not the default browser.
+// Ownership of the delegate is given to the infobar itself, the lifetime of
+// which is bound to the containing WebContents.
 class DefaultBrowserInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
   // Creates a default browser infobar and delegate and adds the infobar to
   // |infobar_service|.
-  static void Create(InfoBarService* infobar_service, PrefService* prefs);
+  static void Create(InfoBarService* infobar_service, Profile* profile);
 
  private:
   // Possible user interactions with the default browser info bar.
   // Do not modify the ordering as it is important for UMA.
   enum InfoBarUserInteraction {
     // The user clicked the "Set as default" button.
-    START_SET_AS_DEFAULT,
-    // The user doesn't want to be reminded again.
-    DONT_ASK_AGAIN,
+    START_SET_AS_DEFAULT = 0,
+    // Value 1 is deprecated; do not re-use.

[msw, 2016/03/08 18:01:02]

nit: leave context with a comment (eg. "Value 1 (DONT_ASK_AGAIN) is
deprecated...") or a renamed enum val (eg. DONT_ASK_AGAIN_DEPRECATED)?

[grt (UTC plus 2), 2016/03/11 16:04:31]

Done.

     // The user did not interact with the info bar.
-    IGNORE_INFO_BAR,
+    IGNORE_INFO_BAR = 2,
     NUM_INFO_BAR_USER_INTERACTION_TYPES
   };
 
-  explicit DefaultBrowserInfoBarDelegate(PrefService* prefs);
+  explicit DefaultBrowserInfoBarDelegate(Profile* profile);
   ~DefaultBrowserInfoBarDelegate() override;
 
   void AllowExpiry() { should_expire_ = true; }
 
-  // ConfirmInfoBarDelegate:
+  // InfoBarDelegate:

[msw, 2016/03/08 18:01:02]

I'm not sure why you pulled out some of the InfoBarDelegate functions here, but
not the others (GetIdentifier, GetIconID, GetVectorIconID). Be consistent here.

[grt (UTC plus 2), 2016/03/11 16:04:31]

Ah, an oversight. I thought I'd gone through each one, but must have gotten off
the rails. Thanks for catching this. Done.

   Type GetInfoBarType() const override;
+  bool ShouldExpire(const NavigationDetails& details) const override;
+  void InfoBarDismissed() override;
+
+  // ConfirmInfoBarDelegate:
   infobars::InfoBarDelegate::InfoBarIdentifier GetIdentifier() const override;
   int GetIconId() const override;
   gfx::VectorIconId GetVectorIconId() const override;
-  bool ShouldExpire(const NavigationDetails& details) const override;
   base::string16 GetMessageText() const override;
+  int GetButtons() const override;
   base::string16 GetButtonLabel(InfoBarButton button) const override;
   bool OKButtonTriggersUACPrompt() const override;
   bool Accept() override;
-  bool Cancel() override;
 
-  // The prefs to use.
-  PrefService* prefs_;
-
-  // Whether the user clicked one of the buttons.
-  bool action_taken_;
+  // The WebContents's corresponding profile.
+  Profile* profile_;
 
   // Whether the info-bar should be dismissed on the next navigation.
-  bool should_expire_;
+  bool should_expire_ = false;
 
   // Used to delay the expiration of the info-bar.
   base::WeakPtrFactory<DefaultBrowserInfoBarDelegate> weak_factory_;
@@ -93,18 +101,14 @@ class DefaultBrowserInfoBarDelegate : public ConfirmInfoBarDelegate {
 
 // static
 void DefaultBrowserInfoBarDelegate::Create(InfoBarService* infobar_service,
-                                           PrefService* prefs) {
+                                           Profile* profile) {
   infobar_service->AddInfoBar(
       infobar_service->CreateConfirmInfoBar(scoped_ptr<ConfirmInfoBarDelegate>(
-          new DefaultBrowserInfoBarDelegate(prefs))));
+          new DefaultBrowserInfoBarDelegate(profile))));
 }
 
-DefaultBrowserInfoBarDelegate::DefaultBrowserInfoBarDelegate(PrefService* prefs)
-    : ConfirmInfoBarDelegate(),
-      prefs_(prefs),
-      action_taken_(false),
-      should_expire_(false),
-      weak_factory_(this) {
+DefaultBrowserInfoBarDelegate::DefaultBrowserInfoBarDelegate(Profile* profile)
+    : ConfirmInfoBarDelegate(), profile_(profile), weak_factory_(this) {
   // We want the info-bar to stick-around for few seconds and then be hidden
   // on the next navigation after that.
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
@@ -113,12 +117,7 @@ DefaultBrowserInfoBarDelegate::DefaultBrowserInfoBarDelegate(PrefService* prefs)
       base::TimeDelta::FromSeconds(8));
 }
 
-DefaultBrowserInfoBarDelegate::~DefaultBrowserInfoBarDelegate() {
-  if (!action_taken_)
-    UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.InfoBar.UserInteraction",
-                              InfoBarUserInteraction::IGNORE_INFO_BAR,
-                              NUM_INFO_BAR_USER_INTERACTION_TYPES);
-}
+DefaultBrowserInfoBarDelegate::~DefaultBrowserInfoBarDelegate() = default;
 
 infobars::InfoBarDelegate::Type DefaultBrowserInfoBarDelegate::GetInfoBarType()
     const {
@@ -129,6 +128,18 @@ infobars::InfoBarDelegate::Type DefaultBrowserInfoBarDelegate::GetInfoBarType()
 #endif
 }
 
+bool DefaultBrowserInfoBarDelegate::ShouldExpire(
+    const NavigationDetails& details) const {
+  return should_expire_ && ConfirmInfoBarDelegate::ShouldExpire(details);
+}
+
+void DefaultBrowserInfoBarDelegate::InfoBarDismissed() {
+  chrome::MarkDefaultBrowserPromptAsDismissed(profile_);
+  UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.InfoBar.UserInteraction",
+                            IGNORE_INFO_BAR,
+                            NUM_INFO_BAR_USER_INTERACTION_TYPES);
+}
+
 infobars::InfoBarDelegate::InfoBarIdentifier
 DefaultBrowserInfoBarDelegate::GetIdentifier() const {
   return DEFAULT_BROWSER_INFOBAR_DELEGATE;
@@ -146,20 +157,18 @@ gfx::VectorIconId DefaultBrowserInfoBarDelegate::GetVectorIconId() const {
 #endif
 }
 
-bool DefaultBrowserInfoBarDelegate::ShouldExpire(
-    const NavigationDetails& details) const {
-  return should_expire_ && ConfirmInfoBarDelegate::ShouldExpire(details);
-}
-
 base::string16 DefaultBrowserInfoBarDelegate::GetMessageText() const {
   return l10n_util::GetStringUTF16(IDS_DEFAULT_BROWSER_INFOBAR_SHORT_TEXT);
 }
 
+int DefaultBrowserInfoBarDelegate::GetButtons() const {
+  return BUTTON_OK;
+}
+
 base::string16 DefaultBrowserInfoBarDelegate::GetButtonLabel(
     InfoBarButton button) const {
-  return l10n_util::GetStringUTF16((button == BUTTON_OK) ?
-      IDS_SET_AS_DEFAULT_INFOBAR_BUTTON_LABEL :
-      IDS_DONT_ASK_AGAIN_INFOBAR_BUTTON_LABEL);
+  DCHECK_EQ(BUTTON_OK, button);
+  return l10n_util::GetStringUTF16(IDS_SET_AS_DEFAULT_INFOBAR_BUTTON_LABEL);
 }
 
 // Setting an app as the default browser doesn't require elevation directly, but
@@ -170,11 +179,10 @@ bool DefaultBrowserInfoBarDelegate::OKButtonTriggersUACPrompt() const {
 }
 
 bool DefaultBrowserInfoBarDelegate::Accept() {
-  action_taken_ = true;
   content::RecordAction(
       base::UserMetricsAction("DefaultBrowserInfoBar_Accept"));
   UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.InfoBar.UserInteraction",
-                            InfoBarUserInteraction::START_SET_AS_DEFAULT,
+                            START_SET_AS_DEFAULT,
                             NUM_INFO_BAR_USER_INTERACTION_TYPES);
   // The worker pointer is reference counted. While it is running, the
   // message loops of the FILE and UI thread will hold references to it
@@ -186,23 +194,11 @@ bool DefaultBrowserInfoBarDelegate::Accept() {
   return true;
 }
 
-bool DefaultBrowserInfoBarDelegate::Cancel() {

[msw, 2016/03/08 18:01:02]

I think it's a little odd that we don't offer 'No, thanks' or some other good
faith way to decline for now (besides the infobar's dismiss button), can you
confirm that's the intent of UX (or okay with them)? 

[grt (UTC plus 2), 2016/03/11 16:04:31]

I discussed this with LaForge, who tells me that this is consistent with other
UX. I will ask for confirmation on the bug.

-  action_taken_ = true;
-  content::RecordAction(
-      base::UserMetricsAction("DefaultBrowserInfoBar_DontAskAgain"));
-  UMA_HISTOGRAM_ENUMERATION("DefaultBrowser.InfoBar.UserInteraction",
-                            InfoBarUserInteraction::DONT_ASK_AGAIN,
-                            NUM_INFO_BAR_USER_INTERACTION_TYPES);
-  // User clicked "Don't ask me again", remember that.
-  prefs_->SetBoolean(prefs::kCheckDefaultBrowser, false);
-  return true;
-}
-
 void ResetCheckDefaultBrowserPref(const base::FilePath& profile_path) {
   Profile* profile =
       g_browser_process->profile_manager()->GetProfileByPath(profile_path);
   if (profile)
-    profile->GetPrefs()->SetBoolean(prefs::kCheckDefaultBrowser, true);
+    chrome::ResetDefaultBrowserPrompt(profile);
 }
 
 void ShowPrompt() {
@@ -218,9 +214,45 @@ void ShowPrompt() {
     return;
 
   DefaultBrowserInfoBarDelegate::Create(
-      InfoBarService::FromWebContents(web_contents),
-      Profile::FromBrowserContext(web_contents->GetBrowserContext())
-          ->GetPrefs());
+      InfoBarService::FromWebContents(web_contents), browser->profile());
+}
+
+// Returns true if the default browser prompt should be shown if Chrome is not
+// the user's default browser.
+bool ShouldShowDefaultBrowserPrompt(Profile* profile) {
+  // Do not show the prompt if the "suppress_default_browser_prompt_for_version"
+  // master preference is set to the current version.
+  const std::string disable_version_string =
+      g_browser_process->local_state()->GetString(
+          prefs::kBrowserSuppressDefaultBrowserPrompt);
+  const Version disable_version(disable_version_string);
+  DCHECK(disable_version_string.empty() || disable_version.IsValid());
+  if (disable_version.IsValid() &&
+      disable_version == Version(version_info::GetVersionNumber())) {
+    return false;
+  }
+
+  // Do not show if the prompt period has yet to pass since the user previously
+  // dismissed the infobar.
+  int64_t last_dismissed_value =
+      profile->GetPrefs()->GetInt64(prefs::kDefaultBrowserLastDismissed);
+  if (last_dismissed_value) {
+    if (!base::FeatureList::IsEnabled(kInfobarRefreshFeature))
+      return false;  // The refresh feature is not enabled.
+    int period_days = 0;
+    base::StringToInt(variations::GetVariationParamValue(
+                          "DefaultBrowserInfobarRefresh", "PeriodDays"),

[msw, 2016/03/08 18:01:02]

Are these usually string literals scattered in code? Why not use constants?

[grt (UTC plus 2), 2016/03/11 16:04:31]

I see both patterns used in the code. My inclination is to keep it as-is until
there's more than one occurrence. In this case, neither literal appears more
than once. I'd be glad to change it if you feel strongly, in which case would
you prefer having the constants defined at the top of the unnamed namespace, or
at function scope?

[msw, 2016/03/14 17:21:18]

Acknowledged. It's fine as-is.

+                      &period_days);
+    if (period_days <= 0 || period_days == std::numeric_limits<int>::max())
+      return false;  // Failed to parse a reasonable period.
+    base::Time show_on_or_after =
+        base::Time::FromInternalValue(last_dismissed_value) +
+        base::TimeDelta::FromDays(period_days);
+    if (base::Time::Now() < show_on_or_after)
+      return false;
+  }
+
+  return true;
 }
 
 void OnCheckIsDefaultBrowserFinished(
@@ -261,32 +293,27 @@ void ShowDefaultBrowserPrompt(Profile* profile) {
   // Reset preferences if kResetCheckDefaultBrowser is true.
   if (prefs->GetBoolean(prefs::kResetCheckDefaultBrowser)) {
     prefs->SetBoolean(prefs::kResetCheckDefaultBrowser, false);
-    prefs->SetBoolean(prefs::kCheckDefaultBrowser, true);
-  }
-
-  // Check if Chrome is the default browser but do not prompt if:
-  // - The user said "don't ask me again" on the infobar earlier.
-  // - The "suppress_default_browser_prompt_for_version" master preference is
-  //     set to the current version.
-  bool show_prompt = prefs->GetBoolean(prefs::kCheckDefaultBrowser);
-  if (show_prompt) {
-    const std::string disable_version_string =
-        g_browser_process->local_state()->GetString(
-            prefs::kBrowserSuppressDefaultBrowserPrompt);
-    const Version disable_version(disable_version_string);
-    DCHECK(disable_version_string.empty() || disable_version.IsValid());
-    if (disable_version.IsValid() &&
-        disable_version == Version(version_info::GetVersionNumber())) {
-      show_prompt = false;
-    }
+    ResetDefaultBrowserPrompt(profile);
   }
 
   scoped_refptr<shell_integration::DefaultBrowserWorker>(
-      new shell_integration::DefaultBrowserWorker(base::Bind(
-          &OnCheckIsDefaultBrowserFinished, profile->GetPath(), show_prompt)))
+      new shell_integration::DefaultBrowserWorker(
+          base::Bind(&OnCheckIsDefaultBrowserFinished, profile->GetPath(),
+                     ShouldShowDefaultBrowserPrompt(profile))))
       ->StartCheckIsDefault();
 }
 
+void MarkDefaultBrowserPromptAsDismissed(Profile* profile) {
+  DCHECK(profile);

[msw, 2016/03/08 18:01:02]

nit: dcheck before dereference isn't really needed (below too)

[grt (UTC plus 2), 2016/03/11 16:04:31]

Done.

+  profile->GetPrefs()->SetInt64(prefs::kDefaultBrowserLastDismissed,
+                                base::Time::Now().ToInternalValue());
+}
+
+void ResetDefaultBrowserPrompt(Profile* profile) {
+  DCHECK(profile);
+  profile->GetPrefs()->ClearPref(prefs::kDefaultBrowserLastDismissed);
+}
+
 #if !defined(OS_WIN)
 bool ShowFirstRunDefaultBrowserPrompt(Profile* profile) {
   return false;