[CrOS Updates] Fix bug which suppresses "update available" notification.

Before this CL, the notification was not shown because the stage was set
to UPGRADE_ANNOYANCE_NONE. This CL changes this logic such that if there
is no RelaunchNotification policy, this is set to UPGRADE_ANNOYANCE_LOW.

Fixed: 1019854
Change-Id: I63c58e9fbcea5f7b71983ed2e41d730e1f23cb95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1900298
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715529}

chrome/browser/upgrade_detector/upgrade_detector.cc

@@ -84,12 +84,25 @@ base::TimeDelta UpgradeDetector::GetRelaunchNotificationPeriod() {
       local_state->FindPreference(prefs::kRelaunchNotificationPeriod);
   const int value = preference->GetValue()->GetInt();
   // Enforce the preference's documented minimum value.
-  static constexpr base::TimeDelta kMinValue = base::TimeDelta::FromHours(1);
+  constexpr base::TimeDelta kMinValue = base::TimeDelta::FromHours(1);
   if (preference->IsDefaultValue() || value < kMinValue.InMilliseconds())
     return base::TimeDelta();
   return base::TimeDelta::FromMilliseconds(value);
 }
 
+// static
+bool UpgradeDetector::IsRelaunchNotificationPolicyEnabled() {
+  // Not all tests provide a PrefService for local_state().
+  auto* local_state = g_browser_process->local_state();
+  if (!local_state)
+    return false;
+
+  // "Chrome menu only" means that the policy is disabled.
+  constexpr int kChromeMenuOnly = 0;
+  return local_state->GetInteger(prefs::kRelaunchNotification) !=
+         kChromeMenuOnly;
+}
+
 void UpgradeDetector::NotifyUpgrade() {
   // An implementation will request that a notification be sent after dropping
   // back to the "none" annoyance level if the RelaunchNotificationPeriod

chrome/browser/upgrade_detector/upgrade_detector.h

@@ -141,6 +141,7 @@ class UpgradeDetector {
   // RelaunchNotificationPeriod policy setting, or a zero delta if unset or out
   // of range.
   static base::TimeDelta GetRelaunchNotificationPeriod();
+  static bool IsRelaunchNotificationPolicyEnabled();
 
   const base::Clock* clock() { return clock_; }
 

chrome/browser/upgrade_detector/upgrade_detector_chromeos.cc

@@ -127,9 +127,12 @@ UpgradeDetectorChromeos::UpgradeDetectorChromeos(
     // base::Unretained is safe here because |this| outlives the registrar.
     pref_change_registrar_.Add(
         prefs::kRelaunchHeadsUpPeriod,
-        base::BindRepeating(
-            &UpgradeDetectorChromeos::OnRelaunchHeadsUpPeriodPrefChanged,
-            base::Unretained(this)));
+        base::BindRepeating(&UpgradeDetectorChromeos::OnRelaunchPrefChanged,
+                            base::Unretained(this)));
+    pref_change_registrar_.Add(
+        prefs::kRelaunchNotification,
+        base::BindRepeating(&UpgradeDetectorChromeos::OnRelaunchPrefChanged,
+                            base::Unretained(this)));
   }
 }
 
@@ -240,18 +243,13 @@ void UpgradeDetectorChromeos::CalculateDeadlines() {
       std::max(high_deadline_ - heads_up_period, upgrade_detected_time());
 }
 
-void UpgradeDetectorChromeos::OnRelaunchHeadsUpPeriodPrefChanged() {
-  // Run OnThresholdPrefChanged using SequencedTaskRunner to avoid double
-  // NotifyUpgrade calls in case two polices are changed at one moment.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&UpgradeDetectorChromeos::OnThresholdPrefChanged,
-                     weak_factory_.GetWeakPtr()));
+void UpgradeDetectorChromeos::OnRelaunchNotificationPeriodPrefChanged() {
+  OnRelaunchPrefChanged();
 }
 
-void UpgradeDetectorChromeos::OnRelaunchNotificationPeriodPrefChanged() {
+void UpgradeDetectorChromeos::OnRelaunchPrefChanged() {
   // Run OnThresholdPrefChanged using SequencedTaskRunner to avoid double
-  // NotifyUpgrade calls in case two polices are changed at one moment.
+  // NotifyUpgrade calls in case multiple policies are changed at one moment.
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&UpgradeDetectorChromeos::OnThresholdPrefChanged,
@@ -320,7 +318,13 @@ void UpgradeDetectorChromeos::NotifyOnUpgrade() {
     set_upgrade_notification_stage(UPGRADE_ANNOYANCE_ELEVATED);
     next_delay = high_deadline_ - current_time;
   } else {
-    set_upgrade_notification_stage(UPGRADE_ANNOYANCE_NONE);
+    // If the relaunch notification policy is enabled, the user will be notified
+    // at a later time, so set the level to UPGRADE_ANNOYANCE_NONE. Otherwise,
+    // the user should be notified now, so set the level to
+    // UPGRADE_ANNOYANCE_LOW.
+    set_upgrade_notification_stage(IsRelaunchNotificationPolicyEnabled()
+                                       ? UPGRADE_ANNOYANCE_NONE
+                                       : UPGRADE_ANNOYANCE_LOW);
     next_delay = elevated_deadline_ - current_time;
   }
   const auto new_stage = upgrade_notification_stage();

chrome/browser/upgrade_detector/upgrade_detector_chromeos.h

@@ -68,9 +68,10 @@ class UpgradeDetectorChromeos : public UpgradeDetector,
   // Calculates |elevated_deadline_| and |high_deadline_|.
   void CalculateDeadlines();
 
-  // Handles a change to the browser.relaunch_heads_up_period Local State
-  // preference. Calls NotifyUpgrade if an upgrade is available.
-  void OnRelaunchHeadsUpPeriodPrefChanged();
+  // Handles a change to the browser.relaunch_heads_up_period or
+  // browser.relaunch_notification Local State preferences. Calls
+  // NotifyUpgrade() if an upgrade is available.
+  void OnRelaunchPrefChanged();
 
   // UpgradeDetector:
   void OnRelaunchNotificationPeriodPrefChanged() override;

chrome/browser/upgrade_detector/upgrade_detector_chromeos_unittest.cc

@@ -69,6 +69,9 @@ class UpgradeDetectorChromeosTest : public ::testing::Test {
       : utc_(icu::TimeZone::createTimeZone("Etc/GMT")),
         task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME),
         scoped_local_state_(TestingBrowserProcess::GetGlobal()) {
+    // By default, test with the relaunch policy enabled.
+    SetIsRelaunchNotificationPolicyEnabled(true /* enabled */);
+
     // Disable the detector's check to see if autoupdates are inabled.
     // Without this, tests put the detector into an invalid state by detecting
     // upgrades before the detection task completes.
@@ -102,6 +105,18 @@ class UpgradeDetectorChromeosTest : public ::testing::Test {
     fake_update_engine_client_->NotifyObserversThatStatusChanged(status);
   }
 
+  // Sets the browser.relaunch_notification preference in Local State to
+  // |value|.
+  void SetIsRelaunchNotificationPolicyEnabled(bool enabled) {
+    constexpr int kChromeMenuOnly = 0;     // Disabled.
+    constexpr int kRecommendedBubble = 1;  // Enabled.
+
+    scoped_local_state_.Get()->SetManagedPref(
+        prefs::kRelaunchNotification,
+        std::make_unique<base::Value>(enabled ? kRecommendedBubble
+                                              : kChromeMenuOnly));
+  }
+
   // Sets the browser.relaunch_notification_period preference in Local State to
   // |value|.
   void SetNotificationPeriodPref(base::TimeDelta value) {
@@ -145,6 +160,21 @@ class UpgradeDetectorChromeosTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(UpgradeDetectorChromeosTest);
 };
 
+TEST_F(UpgradeDetectorChromeosTest, PolicyNotEnabled) {
+  // Disable the relaunch notification policy as a whole.
+  SetIsRelaunchNotificationPolicyEnabled(false /* enabled */);
+
+  TestUpgradeDetectorChromeos upgrade_detector(GetMockClock(),
+                                               GetMockTickClock());
+  upgrade_detector.Init();
+
+  // The observer is expected to be notified that an upgrade is recommended.
+  ::testing::StrictMock<MockUpgradeObserver> mock_observer(&upgrade_detector);
+  EXPECT_CALL(mock_observer, OnUpgradeRecommended());
+
+  NotifyUpdateReadyToInstall();
+}
+
 TEST_F(UpgradeDetectorChromeosTest, TestHighAnnoyanceDeadline) {
   TestUpgradeDetectorChromeos upgrade_detector(GetMockClock(),
                                                GetMockTickClock());