lacros: Mojo API for metrics upload consent, part 2

Per discussions with product and legal, we want to continue to use a
single metrics reporting / crash upload consent on Chrome OS, for both
the OS and the lacros browser. Notes at go/lacros-crash-consent

This CL connects the browser settings toggle for metrics reporting
to the canonical OS metrics reporting pref over mojo. See design doc
at go/lacros-metrics-consent

Bug: 1148604
Test: added to unit_tests and lacros_chrome_browsertests
Change-Id: I38b20ba6d92c6e6b46be297a2120a964023cd195
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2582642Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835757}

chrome/browser/chromeos/crosapi/metrics_reporting_ash.cc

@@ -8,14 +8,41 @@
 
 #include "base/bind.h"
 #include "base/check.h"
+#include "chrome/browser/chromeos/settings/stats_reporting_controller.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chromeos/crosapi/mojom/metrics_reporting.mojom.h"
 #include "components/metrics/metrics_pref_names.h"
 #include "components/prefs/pref_service.h"
 
 namespace crosapi {
+namespace {
+
+// Delegate for production, which uses the real reporting subsystem.
+class DelegateImpl : public MetricsReportingAsh::Delegate {
+ public:
+  DelegateImpl() = default;
+  DelegateImpl(const DelegateImpl&) = delete;
+  DelegateImpl& operator=(const DelegateImpl&) = delete;
+  ~DelegateImpl() override = default;
+
+  // MetricsReportingAsh::Delegate:
+  void SetMetricsReportingEnabled(bool enabled) override {
+    // Use primary profile because Lacros does not support multi-signin.
+    Profile* profile = ProfileManager::GetPrimaryUserProfile();
+    // Chrome OS uses this wrapper around the underlying metrics pref.
+    chromeos::StatsReportingController::Get()->SetEnabled(profile, enabled);
+  }
+};
+
+}  // namespace
 
 MetricsReportingAsh::MetricsReportingAsh(PrefService* local_state)
-    : local_state_(local_state) {
+    : MetricsReportingAsh(std::make_unique<DelegateImpl>(), local_state) {}
+
+MetricsReportingAsh::MetricsReportingAsh(std::unique_ptr<Delegate> delegate,
+                                         PrefService* local_state)
+    : delegate_(std::move(delegate)), local_state_(local_state) {
+  DCHECK(delegate_);
   DCHECK(local_state_);
   pref_change_registrar_.Init(local_state_);
   // base::Unretained() is safe because PrefChangeRegistrar removes all
@@ -45,8 +72,7 @@ void MetricsReportingAsh::AddObserver(
 void MetricsReportingAsh::SetMetricsReportingEnabled(
     bool enabled,
     SetMetricsReportingEnabledCallback callback) {
-  // TODO(https://crbug.com/1148604): Implement this.
-  NOTIMPLEMENTED();
+  delegate_->SetMetricsReportingEnabled(enabled);
   std::move(callback).Run();
 }
 

chrome/browser/chromeos/crosapi/metrics_reporting_ash.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_CHROMEOS_CROSAPI_METRICS_REPORTING_ASH_H_
 #define CHROME_BROWSER_CHROMEOS_CROSAPI_METRICS_REPORTING_ASH_H_
 
+#include <memory>
+
 #include "chromeos/crosapi/mojom/metrics_reporting.mojom.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -19,9 +21,17 @@ namespace crosapi {
 // This class must only be used from the main thread.
 class MetricsReportingAsh : public mojom::MetricsReporting {
  public:
-  // |local_state| is injected for testing. In production it is the global
-  // local state PrefService, owned by g_browser_process.
+  // Allows stubbing out the metrics reporting subsystem.
+  class Delegate {
+   public:
+    virtual ~Delegate() = default;
+    virtual void SetMetricsReportingEnabled(bool enabled) = 0;
+  };
+  // Constructor for production. Uses the real metrics reporting subsystem.
   explicit MetricsReportingAsh(PrefService* local_state);
+  // Constructor for testing.
+  MetricsReportingAsh(std::unique_ptr<Delegate> delegate,
+                      PrefService* local_state);
   MetricsReportingAsh(const MetricsReportingAsh&) = delete;
   MetricsReportingAsh& operator=(const MetricsReportingAsh&) = delete;
   ~MetricsReportingAsh() override;
@@ -42,6 +52,8 @@ class MetricsReportingAsh : public mojom::MetricsReporting {
   // Returns whether metrics reporting is enabled.
   bool IsMetricsReportingEnabled() const;
 
+  std::unique_ptr<Delegate> delegate_;
+
   // In production, owned by g_browser_process, which outlives this object.
   PrefService* const local_state_;
 

chrome/browser/chromeos/crosapi/metrics_reporting_ash_unittest.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/browser/chromeos/crosapi/metrics_reporting_ash.h"
 
+#include <memory>
+
+#include "base/optional.h"
+#include "base/run_loop.h"
 #include "base/test/task_environment.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -34,15 +38,41 @@ class TestObserver : public mojom::MetricsReportingObserver {
   mojo::Receiver<mojom::MetricsReportingObserver> receiver_{this};
 };
 
-TEST(MetricsReportingAshTest, Basics) {
-  base::test::TaskEnvironment task_environment;
+class TestDelegate : public MetricsReportingAsh::Delegate {
+ public:
+  TestDelegate() = default;
+  TestDelegate(const TestDelegate&) = delete;
+  TestDelegate& operator=(const TestDelegate&) = delete;
+  ~TestDelegate() override = default;
+
+  // MetricsReportingAsh::Delegate:
+  void SetMetricsReportingEnabled(bool enabled) override { enabled_ = enabled; }
 
+  // Public because this is test code.
+  base::Optional<bool> enabled_;
+};
+
+class MetricsReportingAshTest : public testing::Test {
+ public:
+  MetricsReportingAshTest()
+      : scoped_testing_local_state_(TestingBrowserProcess::GetGlobal()),
+        local_state_(scoped_testing_local_state_.Get()) {}
+  MetricsReportingAshTest(const MetricsReportingAshTest&) = delete;
+  MetricsReportingAshTest& operator=(const MetricsReportingAshTest&) = delete;
+  ~MetricsReportingAshTest() override = default;
+
+  // Public because this is test code.
+  base::test::TaskEnvironment task_environment_;
+  ScopedTestingLocalState scoped_testing_local_state_;
+  PrefService* const local_state_;
+};
+
+TEST_F(MetricsReportingAshTest, Basics) {
   // Simulate metrics reporting enabled.
-  ScopedTestingLocalState local_state(TestingBrowserProcess::GetGlobal());
-  local_state.Get()->SetBoolean(metrics::prefs::kMetricsReportingEnabled, true);
+  local_state_->SetBoolean(metrics::prefs::kMetricsReportingEnabled, true);
 
   // Construct the object under test.
-  MetricsReportingAsh metrics_reporting_ash(local_state.Get());
+  MetricsReportingAsh metrics_reporting_ash(local_state_);
   mojo::Remote<mojom::MetricsReporting> metrics_reporting_remote;
   metrics_reporting_ash.BindReceiver(
       metrics_reporting_remote.BindNewPipeAndPassReceiver());
@@ -57,14 +87,32 @@ TEST(MetricsReportingAshTest, Basics) {
 
   // Disabling metrics reporting in ash fires the observer with the new value.
   observer.metrics_enabled_.reset();
-  local_state.Get()->SetBoolean(metrics::prefs::kMetricsReportingEnabled,
-                                false);
+  local_state_->SetBoolean(metrics::prefs::kMetricsReportingEnabled, false);
   observer.receiver_.FlushForTesting();
   ASSERT_TRUE(observer.metrics_enabled_.has_value());
   EXPECT_FALSE(observer.metrics_enabled_.value());
+}
+
+TEST_F(MetricsReportingAshTest, SetMetricsReportingEnabled) {
+  // Simulate metrics reporting disabled.
+  local_state_->SetBoolean(metrics::prefs::kMetricsReportingEnabled, false);
+
+  // Construct the object with a test delegate.
+  auto delegate = std::make_unique<TestDelegate>();
+  TestDelegate* test_delegate = delegate.get();
+  MetricsReportingAsh metrics_reporting_ash(std::move(delegate), local_state_);
+  mojo::Remote<mojom::MetricsReporting> metrics_reporting_remote;
+  metrics_reporting_ash.BindReceiver(
+      metrics_reporting_remote.BindNewPipeAndPassReceiver());
 
-  // TODO(https://crbug.com/1148604): Test SetMetricsReportingConsent() once
-  // that function is implemented.
+  // Calling SetMetricsReportingEnabled() over mojo calls through to the metrics
+  // reporting subsystem.
+  base::RunLoop run_loop;
+  metrics_reporting_remote->SetMetricsReportingEnabled(true,
+                                                       run_loop.QuitClosure());
+  run_loop.Run();
+  EXPECT_TRUE(test_delegate->enabled_.has_value());
+  EXPECT_TRUE(test_delegate->enabled_.value());
 }
 
 }  // namespace

chrome/browser/lacros/metrics_reporting_lacros_browsertest.cc

@@ -69,8 +69,16 @@ IN_PROC_BROWSER_TEST_F(MetricsReportingLacrosBrowserTest, Basics) {
   ASSERT_TRUE(observer2.metrics_enabled_.has_value());
   EXPECT_EQ(ash_metrics_enabled, observer2.metrics_enabled_.value());
 
-  // TODO(https://crbug.com/1148604): Test SetMetricsReportingConsent() once
-  // that function is implemented.
+  // Exercise SetMetricsReportingEnabled() and ensure its callback is called.
+  base::RunLoop run_loop3;
+  metrics_reporting->SetMetricsReportingEnabled(!ash_metrics_enabled,
+                                                run_loop3.QuitClosure());
+  run_loop3.Run();
+
+  base::RunLoop run_loop4;
+  metrics_reporting->SetMetricsReportingEnabled(ash_metrics_enabled,
+                                                run_loop4.QuitClosure());
+  run_loop4.Run();
 }
 
 }  // namespace

chrome/browser/ui/BUILD.gn

@@ -2772,6 +2772,8 @@ static_library("ui") {
       "window_sizer/window_sizer_chromeos.h",
     ]
     deps += [
+      "//chromeos/crosapi/mojom",
+      "//chromeos/lacros",
       "//chromeos/ui/base",
       "//chromeos/ui/frame",
     ]

chrome/browser/ui/webui/settings/metrics_reporting_handler.cc

@@ -19,6 +19,11 @@
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/web_ui.h"
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+#include "chromeos/crosapi/mojom/metrics_reporting.mojom.h"  // nogncheck
+#include "chromeos/lacros/lacros_chrome_service_impl.h"
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+
 namespace settings {
 
 MetricsReportingHandler::MetricsReportingHandler() {}
@@ -90,6 +95,30 @@ void MetricsReportingHandler::HandleSetMetricsReportingEnabled(
   bool enabled;
   CHECK(args->GetBoolean(0, &enabled));
   ChangeMetricsReportingState(enabled);
+
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+  // To match the pre-Lacros settings UX, the metrics reporting toggle in Lacros
+  // browser settings controls both browser metrics reporting and OS metrics
+  // reporting. See https://crbug.com/1148604.
+  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
+  // Service may be null in tests.
+  if (!lacros_chrome_service)
+    return;
+  // The metrics reporting API was added in Chrome OS 89.
+  if (!lacros_chrome_service->IsMetricsReportingAvailable()) {
+    LOG(WARNING) << "MetricsReporting API not available";
+    return;
+  }
+  // Bind the remote here instead of the constructor because this function
+  // is rarely called, so we usually don't need the remote.
+  if (!metrics_reporting_remote_.is_bound()) {
+    lacros_chrome_service->BindMetricsReporting(
+        metrics_reporting_remote_.BindNewPipeAndPassReceiver());
+  }
+  // Set metrics reporting state in ash-chrome.
+  metrics_reporting_remote_->SetMetricsReportingEnabled(enabled,
+                                                        base::DoNothing());
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
 }
 
 void MetricsReportingHandler::OnPolicyChanged(const base::Value* previous,

chrome/browser/ui/webui/settings/metrics_reporting_handler.h

@@ -16,6 +16,11 @@
 #include "components/policy/core/common/policy_service.h"
 #include "components/prefs/pref_member.h"
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+#include "chromeos/crosapi/mojom/metrics_reporting.mojom.h"  // nogncheck
+#include "mojo/public/cpp/bindings/remote.h"
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+
 namespace base {
 class DictionaryValue;
 }
@@ -65,6 +70,11 @@ class MetricsReportingHandler : public SettingsPageUIHandler {
   // enabled or managed.
   std::unique_ptr<policy::PolicyChangeRegistrar> policy_registrar_;
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+  // The metrics reporting interface in ash-chrome.
+  mojo::Remote<crosapi::mojom::MetricsReporting> metrics_reporting_remote_;
+#endif  // BUILDFLAG(IS_CHROMEOS_LACROS)
+
   DISALLOW_COPY_AND_ASSIGN(MetricsReportingHandler);
 };