Merge pull request #24291 from brave/improve_day_zero_expt_manager_init

Improved DayZeroBrowserUIExptManager initialization

browser/brave_browser_main_parts.cc

@@ -99,11 +99,10 @@ void BraveBrowserMainParts::PreBrowserStart() {
 #endif
 
 #if BUILDFLAG(IS_WIN)
-  // As this uses first run sentinel time, PreBrowserStart() is good place to
-  // initialize.
+  // As DayZeroBrowserUIExptManager uses first run sentinel time,
+  // PreBrowserStart() is good place to initialize.
   day_zero_browser_ui_expt_manager_ =
-      std::make_unique<DayZeroBrowserUIExptManager>(
-          g_browser_process->profile_manager());
+      DayZeroBrowserUIExptManager::Create(g_browser_process->profile_manager());
 #endif
 
   ChromeBrowserMainParts::PreBrowserStart();

browser/ui/day_zero_browser_ui_expt/BUILD.gn

@@ -26,3 +26,20 @@ source_set("unit_tests") {
     "//testing/gtest",
   ]
 }
+
+source_set("browser_tests") {
+  testonly = true
+  defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
+
+  sources = [ "day_zero_browser_ui_expt_browsertest.cc" ]
+
+  deps = [
+    "//base",
+    "//brave/components/brave_rewards/common",
+    "//chrome/browser",
+    "//chrome/browser/ui",
+    "//chrome/test:test_support_ui",
+    "//components/prefs",
+    "//content/test:test_support",
+  ]
+}

browser/ui/day_zero_browser_ui_expt/day_zero_browser_ui_expt_browsertest.cc

@@ -0,0 +1,56 @@
+/* Copyright (c) 2024 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#include "base/command_line.h"
+#include "base/test/scoped_feature_list.h"
+#include "brave/browser/brave_browser_features.h"
+#include "brave/components/brave_rewards/common/pref_names.h"
+#include "chrome/browser/first_run/first_run.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/test/browser_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// Check DayZeroBrowserUIExptManager is initialized properly.
+// This test will catch first run sentinel creation time fetching timing
+// changes.
+class DayZeroBrowserUIExptBrowserTest
+    : public InProcessBrowserTest,
+      public testing::WithParamInterface<bool> {
+ public:
+  DayZeroBrowserUIExptBrowserTest() {
+    if (IsDayZeroEnabled()) {
+      feature_list_.InitAndEnableFeature(features::kBraveDayZeroExperiment);
+    }
+  }
+
+  ~DayZeroBrowserUIExptBrowserTest() override = default;
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // In browser test, first run sentinel file is not created w/o this switch.
+    command_line->AppendSwitch(switches::kForceFirstRun);
+  }
+
+  bool IsDayZeroEnabled() { return GetParam(); }
+
+  base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_P(DayZeroBrowserUIExptBrowserTest, InitTest) {
+  auto* prefs = browser()->profile()->GetPrefs();
+
+  // Button is hidden by default when expt feature is enabled.
+  const bool button_is_hidden =
+      !prefs->GetBoolean(brave_rewards::prefs::kShowLocationBarButton);
+  EXPECT_EQ(IsDayZeroEnabled(), button_is_hidden);
+}
+
+INSTANTIATE_TEST_SUITE_P(DayZeroExpt,
+                         DayZeroBrowserUIExptBrowserTest,
+                         testing::Bool());

browser/ui/day_zero_browser_ui_expt/day_zero_browser_ui_expt_manager.cc

@@ -20,22 +20,43 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "components/prefs/pref_service.h"
 
-DayZeroBrowserUIExptManager::DayZeroBrowserUIExptManager(
-    ProfileManager* profile_manager)
-    : profile_manager_(*profile_manager) {
+namespace {
+constexpr int kDayZeroFeatureDurationInDays = 1;
+}  // namespace
+
+// static
+std::unique_ptr<DayZeroBrowserUIExptManager>
+DayZeroBrowserUIExptManager::Create(ProfileManager* profile_manager) {
+  if (!base::FeatureList::IsEnabled(features::kBraveDayZeroExperiment)) {
+    return nullptr;
+  }
+
   // This class should be instantiated after getting valid first run time;
   if (brave_stats::GetFirstRunTime(nullptr).is_null()) {
-    CHECK_IS_TEST();
+    // This should not be happened in production but not 100% in the wild(ex,
+    // corrupted user data). Just early return for safe. If upstream changes the
+    // timing of fetching first run time, browser test will catch this.
+    LOG(ERROR) << __func__ << " This should be happened only on test.";
+    return nullptr;
   }
 
-  // If one day passed since first run or day zero expt feature is off,
-  // we don't need to touch original default pref values.
-  // Just early return and this class is no-op.
-  if (IsPassedOneDaySinceFirstRun() ||
-      !base::FeatureList::IsEnabled(features::kBraveDayZeroExperiment)) {
-    return;
+  // If one day passed since first run, we don't need to touch original default
+  // pref values. Just early return and this class is no-op.
+  if (base::Time::Now() - brave_stats::GetFirstRunTime(nullptr) >=
+      base::Days(kDayZeroFeatureDurationInDays)) {
+    VLOG(2) << __func__ << " Already passed day zero feature duration.";
+    return nullptr;
   }
 
+  // base::WrapUnique for using private ctor.
+  return base::WrapUnique(new DayZeroBrowserUIExptManager(profile_manager));
+}
+
+DayZeroBrowserUIExptManager::DayZeroBrowserUIExptManager(
+    ProfileManager* profile_manager,
+    std::optional<base::Time> mock_first_run_time)
+    : profile_manager_(*profile_manager),
+      first_run_time_for_testing_(mock_first_run_time) {
   for (auto* profile : profile_manager_->GetLoadedProfiles()) {
     if (!profile->IsRegularProfile()) {
       continue;
@@ -65,6 +86,8 @@ void DayZeroBrowserUIExptManager::OnProfileManagerDestroying() {
 }
 
 void DayZeroBrowserUIExptManager::SetForDayZeroBrowserUI(Profile* profile) {
+  VLOG(2) << __func__ << " Update prefs for day zero expt.";
+
   auto* prefs = profile->GetPrefs();
   prefs->SetDefaultPrefValue(kNewTabPageShowRewards, base::Value(false));
   prefs->SetDefaultPrefValue(kNewTabPageShowBraveTalk, base::Value(false));
@@ -79,6 +102,8 @@ void DayZeroBrowserUIExptManager::SetForDayZeroBrowserUI(Profile* profile) {
 }
 
 void DayZeroBrowserUIExptManager::ResetForDayZeroBrowserUI(Profile* profile) {
+  VLOG(2) << __func__ << " Update prefs for day zero expt.";
+
   auto* prefs = profile->GetPrefs();
   prefs->SetDefaultPrefValue(kNewTabPageShowRewards, base::Value(true));
   prefs->SetDefaultPrefValue(kNewTabPageShowBraveTalk, base::Value(true));
@@ -107,26 +132,36 @@ void DayZeroBrowserUIExptManager::ResetBrowserUIStateForAllProfiles() {
   }
 }
 
-bool DayZeroBrowserUIExptManager::IsPassedOneDaySinceFirstRun() const {
-  return brave_stats::GetFirstRunTime(nullptr) - base::Time::Now() >=
-         base::Days(1);
-}
-
 void DayZeroBrowserUIExptManager::StartResetTimer() {
-  auto remained_time_to_one_day_since_first_run =
-      brave_stats::GetFirstRunTime(nullptr) - base::Time::Now();
+  auto remained_time_to_expt_duration_since_first_run =
+      GetFirstRunTime() - base::Time::Now();
 
   // Convenient switch only for testing purpose.
   constexpr char kUseShortExpirationForDayZeroExpt[] =
       "use-short-expiration-for-day-zero-expt";
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           kUseShortExpirationForDayZeroExpt)) {
-    remained_time_to_one_day_since_first_run += base::Minutes(2);
+    remained_time_to_expt_duration_since_first_run += base::Minutes(2);
   } else {
-    remained_time_to_one_day_since_first_run += base::Days(1);
+    remained_time_to_expt_duration_since_first_run +=
+        base::Days(kDayZeroFeatureDurationInDays);
+  }
+
+  // If remained time is negative, reset to original here.
+  if (remained_time_to_expt_duration_since_first_run < base::TimeDelta()) {
+    ResetBrowserUIStateForAllProfiles();
+    return;
   }
 
   reset_timer_.Start(
-      FROM_HERE, remained_time_to_one_day_since_first_run, this,
+      FROM_HERE, remained_time_to_expt_duration_since_first_run, this,
       &DayZeroBrowserUIExptManager::ResetBrowserUIStateForAllProfiles);
 }
+
+base::Time DayZeroBrowserUIExptManager::GetFirstRunTime() const {
+  if (first_run_time_for_testing_) {
+    return *first_run_time_for_testing_;
+  }
+
+  return brave_stats::GetFirstRunTime(nullptr);
+}

browser/ui/day_zero_browser_ui_expt/day_zero_browser_ui_expt_manager.h

@@ -6,8 +6,12 @@
 #ifndef BRAVE_BROWSER_UI_DAY_ZERO_BROWSER_UI_EXPT_DAY_ZERO_BROWSER_UI_EXPT_MANAGER_H_
 #define BRAVE_BROWSER_UI_DAY_ZERO_BROWSER_UI_EXPT_DAY_ZERO_BROWSER_UI_EXPT_MANAGER_H_
 
+#include <memory>
+#include <optional>
+
 #include "base/memory/raw_ref.h"
 #include "base/scoped_observation.h"
+#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/profiles/profile_manager_observer.h"
 
@@ -17,7 +21,9 @@ class ProfileManager;
 
 class DayZeroBrowserUIExptManager : public ProfileManagerObserver {
  public:
-  explicit DayZeroBrowserUIExptManager(ProfileManager* profile_manater);
+  static std::unique_ptr<DayZeroBrowserUIExptManager> Create(
+      ProfileManager* profile_manager);
+
   ~DayZeroBrowserUIExptManager() override;
   DayZeroBrowserUIExptManager(const DayZeroBrowserUIExptManager&) = delete;
   DayZeroBrowserUIExptManager& operator=(const DayZeroBrowserUIExptManager&) =
@@ -28,15 +34,23 @@ class DayZeroBrowserUIExptManager : public ProfileManagerObserver {
   void OnProfileManagerDestroying() override;
 
  private:
+  friend class DayZeroBrowserUIExptTest;
+
+  // |mock_first_run_time| only for testing.
+  DayZeroBrowserUIExptManager(
+      ProfileManager* profile_manager,
+      std::optional<base::Time> mock_first_run_time = std::nullopt);
+
   void SetForDayZeroBrowserUI(Profile* profile);
   void ResetForDayZeroBrowserUI(Profile* profile);
   void ResetBrowserUIStateForAllProfiles();
-  bool IsPassedOneDaySinceFirstRun() const;
   void StartResetTimer();
+  base::Time GetFirstRunTime() const;
 
   // When fire, we'll reset browser UI to original.
   base::OneShotTimer reset_timer_;
   raw_ref<ProfileManager> profile_manager_;
+  std::optional<base::Time> first_run_time_for_testing_;
   base::ScopedObservation<ProfileManager, ProfileManagerObserver> observation_{
       this};
 };

browser/ui/day_zero_browser_ui_expt/day_zero_browser_ui_expt_unittest.cc

@@ -37,8 +37,17 @@ class DayZeroBrowserUIExptTest : public testing::Test,
   void SetUp() override {
     ASSERT_TRUE(testing_profile_manager_.SetUp());
     observation_.Observe(g_browser_process->profile_manager());
-    manager_ = std::make_unique<DayZeroBrowserUIExptManager>(
-        g_browser_process->profile_manager());
+    if (IsDayZeroEnabled()) {
+      // Get mock first run time and uset it for current time also.
+      base::Time first_run_time;
+      if (base::Time::FromString("2500-01-01", &first_run_time)) {
+        task_environment_.AdvanceClock(first_run_time - base::Time::Now());
+      }
+
+      // base::WrapUnique for using private ctor.
+      manager_ = base::WrapUnique(new DayZeroBrowserUIExptManager(
+          g_browser_process->profile_manager(), first_run_time));
+    }
   }
 
   void CheckBrowserHasDayZeroUI(Profile* profile) {

test/BUILD.gn

@@ -984,6 +984,7 @@ test("brave_browser_tests") {
 
   if (is_win) {
     public_configs = [ "//build/config/win:delayloads" ]
+    deps += [ "//brave/browser/ui/day_zero_browser_ui_expt:browser_tests" ]
   }
 
   if (use_aura) {