Simplify the default browser infobar.

chrome/browser/ui/startup/default_browser_prompt.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/startup/default_browser_prompt.h"
 
 #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"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/infobars/core/confirm_infobar_delegate.h"
 #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"
 #include "content/public/browser/web_contents.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/vector_icons_public.h"
 
 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; }
 
+  // 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:
-  Type GetInfoBarType() const override;
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(DefaultBrowserInfoBarDelegate);
 };
 
 // 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(
       FROM_HERE, base::Bind(&DefaultBrowserInfoBarDelegate::AllowExpiry,
                             weak_factory_.GetWeakPtr()),
       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 {
 #if defined(OS_WIN)
   return WARNING_TYPE;
 #else
   return PAGE_ACTION_TYPE;
 #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;
 }
 
 int DefaultBrowserInfoBarDelegate::GetIconId() const {
   return IDR_PRODUCT_LOGO_32;
 }
 
 gfx::VectorIconId DefaultBrowserInfoBarDelegate::GetVectorIconId() const {
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
   return gfx::VectorIconId::VECTOR_ICON_NONE;
 #else
   return gfx::VectorIconId::CHROME_PRODUCT;
 #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
 // it does require registering it as the protocol handler for "http", so if
 // protocol registration in general requires elevation, this does as well.
 bool DefaultBrowserInfoBarDelegate::OKButtonTriggersUACPrompt() const {
   return shell_integration::IsElevationNeededForSettingDefaultProtocolClient();
 }
 
 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
   // and it will be automatically freed once all its tasks have finished.
   scoped_refptr<shell_integration::DefaultBrowserWorker>(
       new shell_integration::DefaultBrowserWorker(
           shell_integration::DefaultWebClientWorkerCallback()))
       ->StartSetAsDefault();
   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() {
   Browser* browser = chrome::FindLastActive();
   if (!browser)
     return;  // Reached during ui tests.
 
   // In ChromeBot tests, there might be a race. This line appears to get
   // called during shutdown and |tab| can be NULL.
   content::WebContents* web_contents =
       browser->tab_strip_model()->GetActiveWebContents();
   if (!web_contents)
     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(
     const base::FilePath& profile_path,
     bool show_prompt,
     shell_integration::DefaultWebClientUIState state) {
   if (state == shell_integration::STATE_IS_DEFAULT) {
     // Notify the user in the future if Chrome ceases to be the user's chosen
     // default browser.
     ResetCheckDefaultBrowserPref(profile_path);
@@ skipping 20 matching lines @@
       prefs::kDefaultBrowserSettingEnabled)) {
     // Handling of the browser.default_browser_setting_enabled policy setting is
     // taken care of in BrowserProcessImpl.
     return;
   }
 
   PrefService* prefs = profile->GetPrefs();
   // 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;
 }
 #endif
 
 }  // namespace chrome