[1/4] Allow registration of power monitor observers pre-initialization

This CL is allowing the registration of PowerObservers before the
PowerMonitor is initialized.

The PowerMonitor has a PowerSource and a ObserverList. The call to
PowerMonitor::Initialize is used to set a PowerSource that will
trigger the notifications. It is thus valid to add observers before
the power source is set.

TBR=miu@chromium.org

Bug: 1074332
Change-Id: Ia238b7caf52f80a7de01329908c4979d81341634
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2202877
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771591}

base/power_monitor/power_monitor.cc

@@ -25,12 +25,8 @@ bool PowerMonitor::IsInitialized() {
   return GetInstance()->source_.get() != nullptr;
 }
 
-bool PowerMonitor::AddObserver(PowerObserver* obs) {
-  PowerMonitor* power_monitor = GetInstance();
-  if (!IsInitialized())
-    return false;
-  power_monitor->observers_->AddObserver(obs);
-  return true;
+void PowerMonitor::AddObserver(PowerObserver* obs) {
+  GetInstance()->observers_->AddObserver(obs);
 }
 
 void PowerMonitor::RemoveObserver(PowerObserver* obs) {
@@ -47,9 +43,8 @@ bool PowerMonitor::IsOnBatteryPower() {
 }
 
 void PowerMonitor::ShutdownForTesting() {
-  PowerMonitor::GetInstance()->observers_->AssertEmpty();
   GetInstance()->source_ = nullptr;
-  g_is_process_suspended.store(false);
+  g_is_process_suspended.store(false, std::memory_order_relaxed);
 }
 
 bool PowerMonitor::IsProcessSuspended() {

base/power_monitor/power_monitor.h

@@ -40,12 +40,10 @@ class BASE_EXPORT PowerMonitor {
   // from which it was registered.
   // Must not be called from within a notification callback.
   //
-  // AddObserver() fails and returns false if PowerMonitor::Initialize() has not
-  // been invoked. Failure should only happen in unit tests, where the
-  // PowerMonitor is generally not initialized. It is safe to call
-  // RemoveObserver with a PowerObserver that was not successfully added as an
+  // It is safe to add observers before the PowerMonitor is initialized. It is
+  // safe to call RemoveObserver with a PowerObserver that was not added as an
   // observer.
-  static bool AddObserver(PowerObserver* observer);
+  static void AddObserver(PowerObserver* observer);
   static void RemoveObserver(PowerObserver* observer);
 
   // Is the computer currently on battery power. May only be called if the

base/power_monitor/power_monitor_unittest.cc

@@ -13,12 +13,15 @@ namespace base {
 
 class PowerMonitorTest : public testing::Test {
  protected:
-  PowerMonitorTest() {
+  PowerMonitorTest() = default;
+
+  void TearDown() override { PowerMonitor::ShutdownForTesting(); }
+
+  void PowerMonitorInitialize() {
     power_monitor_source_ = new PowerMonitorTestSource();
     PowerMonitor::Initialize(
         std::unique_ptr<PowerMonitorSource>(power_monitor_source_));
   }
-  ~PowerMonitorTest() override { PowerMonitor::ShutdownForTesting(); }
 
   PowerMonitorTestSource* source() { return power_monitor_source_; }
 
@@ -34,9 +37,11 @@ class PowerMonitorTest : public testing::Test {
 TEST_F(PowerMonitorTest, PowerNotifications) {
   const int kObservers = 5;
 
+  PowerMonitorInitialize();
+
   PowerMonitorTestObserver observers[kObservers];
   for (auto& index : observers)
-    EXPECT_TRUE(PowerMonitor::AddObserver(&index));
+    PowerMonitor::AddObserver(&index);
 
   // Sending resume when not suspended should have no effect.
   source()->GenerateResumeEvent();
@@ -84,7 +89,9 @@ TEST_F(PowerMonitorTest, PowerNotifications) {
 
 TEST_F(PowerMonitorTest, ThermalThrottling) {
   PowerMonitorTestObserver observer;
-  EXPECT_TRUE(PowerMonitor::AddObserver(&observer));
+  PowerMonitor::AddObserver(&observer);
+
+  PowerMonitorInitialize();
 
   constexpr PowerObserver::DeviceThermalState kThermalStates[] = {
       PowerObserver::DeviceThermalState::kUnknown,
@@ -100,4 +107,32 @@ TEST_F(PowerMonitorTest, ThermalThrottling) {
 
   PowerMonitor::RemoveObserver(&observer);
 }
+
+TEST_F(PowerMonitorTest, AddObserverBeforeAndAfterInitialization) {
+  PowerMonitorTestObserver observer1;
+  PowerMonitorTestObserver observer2;
+
+  // An observer is added before the PowerMonitor initialization.
+  PowerMonitor::AddObserver(&observer1);
+
+  PowerMonitorInitialize();
+
+  // An observer is added after the PowerMonitor initialization.
+  PowerMonitor::AddObserver(&observer2);
+
+  // Simulate suspend/resume notifications.
+  source()->GenerateSuspendEvent();
+  EXPECT_EQ(observer1.suspends(), 1);
+  EXPECT_EQ(observer2.suspends(), 1);
+  EXPECT_EQ(observer1.resumes(), 0);
+  EXPECT_EQ(observer2.resumes(), 0);
+
+  source()->GenerateResumeEvent();
+  EXPECT_EQ(observer1.resumes(), 1);
+  EXPECT_EQ(observer2.resumes(), 1);
+
+  PowerMonitor::RemoveObserver(&observer1);
+  PowerMonitor::RemoveObserver(&observer2);
+}
+
 }  // namespace base

base/util/timer/wall_clock_timer.cc

@@ -52,8 +52,10 @@ void WallClockTimer::OnResume() {
 }
 
 void WallClockTimer::AddObserver() {
-  if (!observer_added_)
-    observer_added_ = base::PowerMonitor::AddObserver(this);
+  if (!observer_added_) {
+    base::PowerMonitor::AddObserver(this);
+    observer_added_ = true;
+  }
 }
 
 void WallClockTimer::RemoveObserver() {

gpu/ipc/service/gpu_watchdog_thread_v2.cc

@@ -284,7 +284,8 @@ void GpuWatchdogThreadImplV2::OnAddPowerObserver() {
   DCHECK(watchdog_thread_task_runner_->BelongsToCurrentThread());
   DCHECK(base::PowerMonitor::IsInitialized());
 
-  is_power_observer_added_ = base::PowerMonitor::AddObserver(this);
+  base::PowerMonitor::AddObserver(this);
+  is_power_observer_added_ = true;
 }
 
 // Running on the watchdog thread.

media/cast/sender/h264_vt_encoder.cc

@@ -181,22 +181,17 @@ H264VideoToolboxEncoder::H264VideoToolboxEncoder(
             weak_factory_.GetWeakPtr(), cast_environment_));
 
     // Register for power state changes.
-    if (base::PowerMonitor::AddObserver(this)) {
-      VLOG(1) << "Registered for power state changes.";
-    } else {
-      DLOG(WARNING) << "No power monitor. Process suspension will invalidate "
-                       "the encoder.";
-    }
+    base::PowerMonitor::AddObserver(this);
+    VLOG(1) << "Registered for power state changes.";
   }
 }
 
 H264VideoToolboxEncoder::~H264VideoToolboxEncoder() {
   DestroyCompressionSession();
 
-  // If video_frame_factory_ is not null, the encoder registered for power state
-  // changes in the ctor and it must now unregister.
-  if (video_frame_factory_)
-    base::PowerMonitor::RemoveObserver(this);
+  // Unregister the power observer. It is valid to remove an observer that was
+  // not added.
+  base::PowerMonitor::RemoveObserver(this);
 }
 
 void H264VideoToolboxEncoder::ResetCompressionSession() {