lacros: Only call ChangeMetricsReportingState() for actual changes

ChangeMetricsReportingState() has side effects that occur even if the
reporting state didn't change, including recording an UMA metric and
clearing some stability data.

Change MetricsRecordingObserver to only call the method when the
state actually changed, rather than unconditionally on startup and
on change.

Bug: 1148604
Test: added to unit_tests
Change-Id: Ia510bcb99c5b8dd0ad23e7ade0c8595ad8d86cfa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580841Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834972}

chrome/browser/chrome_browser_main_parts_lacros.cc

@@ -25,7 +25,8 @@ int ChromeBrowserMainPartsLacros::PreEarlyInitialization() {
   // for updates. Create it here because local state is required to check for
   // policy overrides.
   DCHECK(g_browser_process->local_state());
-  metrics_reporting_observer_ = std::make_unique<MetricsReportingObserver>();
+  metrics_reporting_observer_ = std::make_unique<MetricsReportingObserver>(
+      g_browser_process->local_state());
   metrics_reporting_observer_->Init();
 
   return content::RESULT_CODE_NORMAL_EXIT;

chrome/browser/lacros/metrics_reporting_observer.cc

@@ -4,11 +4,16 @@
 
 #include "chrome/browser/lacros/metrics_reporting_observer.h"
 
-#include "base/bind.h"
+#include "base/check.h"
 #include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chromeos/lacros/lacros_chrome_service_impl.h"
+#include "components/metrics/metrics_pref_names.h"
+#include "components/prefs/pref_service.h"
 
-MetricsReportingObserver::MetricsReportingObserver() = default;
+MetricsReportingObserver::MetricsReportingObserver(PrefService* local_state)
+    : local_state_(local_state) {
+  DCHECK(local_state_);
+}
 
 MetricsReportingObserver::~MetricsReportingObserver() = default;
 
@@ -20,7 +25,7 @@ void MetricsReportingObserver::Init() {
   }
 
   // Set the initial state.
-  ChangeMetricsReportingState(
+  UpdateMetricsReportingState(
       lacros_service->init_params()->ash_metrics_enabled);
 
   // Add this object as an observer. The observer will fire with the current
@@ -32,5 +37,23 @@ void MetricsReportingObserver::Init() {
 }
 
 void MetricsReportingObserver::OnMetricsReportingChanged(bool enabled) {
+  UpdateMetricsReportingState(enabled);
+}
+
+void MetricsReportingObserver::UpdateMetricsReportingState(bool enabled) {
+  // ChangeMetricsReportingState() unconditionally records its own UMA metrics
+  // and clears stability metrics. Only call it if the enabled state actually
+  // changed.
+  if (enabled == IsMetricsReportingEnabled())
+    return;
+
+  DoChangeMetricsReportingState(enabled);
+}
+
+void MetricsReportingObserver::DoChangeMetricsReportingState(bool enabled) {
   ChangeMetricsReportingState(enabled);
 }
+
+bool MetricsReportingObserver::IsMetricsReportingEnabled() const {
+  return local_state_->GetBoolean(metrics::prefs::kMetricsReportingEnabled);
+}

chrome/browser/lacros/metrics_reporting_observer.h

@@ -9,13 +9,16 @@
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
+class PrefService;
+
 // Observes ash-chrome for changes in metrics reporting consent state. The UX
 // goal is to have a single "shared" metrics reporting state across the OS and
 // browser. Ash owns the canonical state, so lacros observes it for changes.
 class MetricsReportingObserver
     : public crosapi::mojom::MetricsReportingObserver {
  public:
-  MetricsReportingObserver();
+  // |local_state| is the "Local State" (non-profile) preferences store.
+  explicit MetricsReportingObserver(PrefService* local_state);
   MetricsReportingObserver(const MetricsReportingObserver&) = delete;
   MetricsReportingObserver& operator=(const MetricsReportingObserver&) = delete;
   ~MetricsReportingObserver() override;
@@ -26,6 +29,20 @@ class MetricsReportingObserver
   void OnMetricsReportingChanged(bool enabled) override;
 
  private:
+  friend class TestMetricsReportingObserver;
+
+  // Updates the metrics reporting if it has changed from the previous state.
+  void UpdateMetricsReportingState(bool enabled);
+
+  // Changes the metrics reporting state. Virtual for testing.
+  virtual void DoChangeMetricsReportingState(bool enabled);
+
+  // Returns whether metrics reporting is enabled.
+  bool IsMetricsReportingEnabled() const;
+
+  // Local state (non-profile) preferences.
+  PrefService* const local_state_;
+
   // Mojo connection to ash.
   mojo::Remote<crosapi::mojom::MetricsReporting> metrics_reporting_remote_;
 

chrome/browser/lacros/metrics_reporting_observer_unittest.cc

+// Copyright 2020 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/lacros/metrics_reporting_observer.h"
+
+#include "base/test/task_environment.h"
+#include "chrome/test/base/scoped_testing_local_state.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "components/metrics/metrics_pref_names.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// MetricsReportingObserver that avoids calling ChangeMetricsReportingState().
+class TestMetricsReportingObserver : public MetricsReportingObserver {
+ public:
+  explicit TestMetricsReportingObserver(PrefService* local_state)
+      : MetricsReportingObserver(local_state) {}
+  TestMetricsReportingObserver(const TestMetricsReportingObserver&) = delete;
+  TestMetricsReportingObserver& operator=(const TestMetricsReportingObserver&) =
+      delete;
+  ~TestMetricsReportingObserver() override = default;
+
+  // MetricsReportingObserver:
+  void DoChangeMetricsReportingState(bool enabled) override {
+    metrics_reporting_enabled_ = enabled;
+    ++change_metrics_reporting_count_;
+  }
+
+  base::Optional<bool> metrics_reporting_enabled_;
+  int change_metrics_reporting_count_ = 0;
+};
+
+TEST(MetricsReportingObserverTest, ChangesOnlyApplyOnce) {
+  base::test::TaskEnvironment task_environment;
+
+  // Simulate starting with metrics reporting starting enabled.
+  ScopedTestingLocalState local_state(TestingBrowserProcess::GetGlobal());
+  local_state.Get()->SetBoolean(metrics::prefs::kMetricsReportingEnabled, true);
+
+  // Construct object under test.
+  TestMetricsReportingObserver observer(local_state.Get());
+
+  // Receiving metrics reporting enabled from ash does not call
+  // ChangeMetricsReportingState() because metrics are already enabled.
+  observer.OnMetricsReportingChanged(true);
+  EXPECT_FALSE(observer.metrics_reporting_enabled_.has_value());
+  EXPECT_EQ(0, observer.change_metrics_reporting_count_);
+
+  // However, receiving metrics reporting disabled from ash does call
+  // ChangeMetricsReportingState() because the value changed.
+  observer.OnMetricsReportingChanged(false);
+  EXPECT_TRUE(observer.metrics_reporting_enabled_.has_value());
+  EXPECT_FALSE(observer.metrics_reporting_enabled_.value());
+  EXPECT_EQ(1, observer.change_metrics_reporting_count_);
+}

chrome/test/BUILD.gn

@@ -4905,6 +4905,7 @@ test("unit_tests") {
     assert(enable_native_notifications)
     sources += [
       "../browser/lacros/lacros_chrome_service_delegate_impl_unittest.cc",
+      "../browser/lacros/metrics_reporting_observer_unittest.cc",
       "../browser/notifications/notification_platform_bridge_lacros_unittest.cc",
       "../common/chrome_paths_lacros_unittest.cc",
     ]