Resolve first round of PR comments.

b/280094891

Change-Id: I1e833a6947065fb7f2e6fe398c2249ca72d28007

cobalt/browser/metrics/cobalt_enabled_state_provider.h

@@ -50,9 +50,8 @@ class CobaltEnabledStateProvider : public ::metrics::EnabledStateProvider {
   void SetReportingEnabled(bool is_reporting_enabled);
 
  private:
-  const PrefService* local_state_;
-  bool is_consent_given_;
-  bool is_reporting_enabled_;
+  bool is_consent_given_ = false;
+  bool is_reporting_enabled_ = false;
 };
 
 

cobalt/browser/metrics/cobalt_metrics_service_client.cc

@@ -44,7 +44,10 @@ namespace cobalt {
 namespace browser {
 namespace metrics {
 
-// How frequently to attempt uploading.
+// The interval in between upload attempts. That is, every interval in which
+// we package up the new, unlogged, metrics and attempt uploading them. In
+// Cobalt's case, this is the shortest interval in which we'll call the H5vcc
+// MetricEventHandlerWrapper.
 const int kStandardUploadIntervalSeconds = 5 * 60;  // 5 minutes.
 
 void CobaltMetricsServiceClient::SetOnUploadHandler(
@@ -77,6 +80,8 @@ void CobaltMetricsServiceClient::SetMetricsClientId(
   // TODO(b/286066035): What to do with client id here?
 }
 
+// TODO(b/286884542): Audit all stub implementations in this class and reaffirm
+// they're not needed and/or add a reasonable implementation.
 int32_t CobaltMetricsServiceClient::GetProduct() {
   // Note, Product is a Chrome concept and similar dimensions will get logged
   // elsewhere downstream. This value doesn't matter.
@@ -103,8 +108,8 @@ CobaltMetricsServiceClient::GetChannel() {
 }
 
 std::string CobaltMetricsServiceClient::GetVersionString() {
-  // All version strings will be attached to the GEL event sent by the web
-  // client, so the value doesn't matter much here.
+  // We assume the web client will log the Cobalt version along with its payload
+  // so this field is not that important.
   return "1.0";
 }
 
@@ -171,13 +176,13 @@ bool CobaltMetricsServiceClient::IsUMACellularUploadLogicEnabled() {
 
 bool CobaltMetricsServiceClient::SyncStateAllowsUkm() {
   // UKM currently not used. Value doesn't matter here.
-  return true;
+  return false;
 }
 
 bool CobaltMetricsServiceClient::SyncStateAllowsExtensionUkm() {
   // TODO(b/284467142): Revisit when enabling UKM.
   // UKM currently not used. Value doesn't matter here.
-  return true;
+  return false;
 }
 
 bool CobaltMetricsServiceClient::

cobalt/browser/metrics/cobalt_metrics_service_client.h

@@ -157,9 +157,6 @@ class CobaltMetricsServiceClient : public ::metrics::MetricsServiceClient {
       ::metrics::MetricsStateManager* state_manager, PrefService* local_state);
 
  private:
-  // Any prefs/state specific to Cobalt Metrics.
-  PrefService* local_state_;
-
   // The MetricsStateManager, must outlive the Metrics Service.
   ::metrics::MetricsStateManager* metrics_state_manager_;
 

cobalt/browser/metrics/cobalt_metrics_services_manager_client.cc

@@ -42,6 +42,7 @@ CobaltMetricsServicesManagerClient::CreateMetricsServiceClient() {
 std::unique_ptr<const base::FieldTrial::EntropyProvider>
 CobaltMetricsServicesManagerClient::CreateEntropyProvider() {
   // Cobalt doesn't use FieldTrials, so this is a noop.
+  NOTIMPLEMENTED();
   return nullptr;
 }
 

cobalt/h5vcc/h5vcc_metrics.cc

@@ -31,6 +31,7 @@ void H5vccMetrics::OnMetricEvent(
       static_cast<browser::metrics::CobaltMetricsServiceClient*>(
           browser::metrics::CobaltMetricsServicesManager::GetInstance()
               ->GetMetricsServiceClient());
+  DCHECK(client);
   client->SetOnUploadHandler(event_handler_.get());
 }
 
@@ -39,11 +40,15 @@ void H5vccMetrics::ToggleMetricsEnabled(bool isEnabled) {
       static_cast<browser::metrics::CobaltMetricsServicesManagerClient*>(
           browser::metrics::CobaltMetricsServicesManager::GetInstance()
               ->GetMetricsServicesManagerClient());
+  DCHECK(client);
+  is_enabled_ = isEnabled;
   client->GetEnabledStateProvider()->SetConsentGiven(isEnabled);
   client->GetEnabledStateProvider()->SetReportingEnabled(isEnabled);
   browser::metrics::CobaltMetricsServicesManager::GetInstance()
       ->UpdateUploadPermissions(isEnabled);
 }
 
+bool H5vccMetrics::GetMetricsEnabled() { return is_enabled_; }
+
 }  // namespace h5vcc
 }  // namespace cobalt

cobalt/h5vcc/h5vcc_metrics.h

@@ -39,6 +39,9 @@ class H5vccMetrics : public script::Wrappable {
 
   H5vccMetrics() {}
 
+  H5vccMetrics(const H5vccMetrics&) = delete;
+  H5vccMetrics& operator=(const H5vccMetrics&) = delete;
+
   // Binds an event handler that will be invoked every time Cobalt wants to
   // upload a metrics payload.
   void OnMetricEvent(
@@ -49,12 +52,14 @@ class H5vccMetrics : public script::Wrappable {
   // should never get called after that point.
   void ToggleMetricsEnabled(bool isEnabled);
 
+  // Returns current enabled state of metrics logging/reporting.
+  bool GetMetricsEnabled();
+
   DEFINE_WRAPPABLE_TYPE(H5vccMetrics);
 
  private:
   scoped_refptr<MetricEventHandlerWrapper> event_handler_;
-
-  DISALLOW_COPY_AND_ASSIGN(H5vccMetrics);
+  bool is_enabled_ = false;
 };
 
 }  // namespace h5vcc

cobalt/h5vcc/h5vcc_metrics.idl

@@ -21,12 +21,23 @@ interface H5vccMetrics {
   // every time Cobalt wants to upload a metrics payload. Only one event
   // handler callback can be registered at any time. Duplicate calls to
   // onMetricEvent will override any previously registered callback.
+  //
+  // Example usage in JS:
+  //
+  //   window.h5vcc.metrics.onMetricEvent((metricType, payload) => {
+  //     if (metricType == 'UMA') {
+  //       // log UMA payload here...
+  //     }
+  //   });
   void onMetricEvent(H5vccMetricEventHandler eventHandler);
 
   // API to explicitly enable or disable Cobalt metrics logging. Pass true
   // to turn on and false to disable. If disabled, the metric event handler
   // should never get called afterward.
   void toggleMetricsEnabled(boolean isEnabled);
+
+  // Returns the current enabled state of metrics reporting.
+  boolean getMetricsEnabled();
 };
 
 // Callback invoked when a new metric payload is ready to be published. The

cobalt/h5vcc/script_callback_wrapper.h

@@ -1,4 +1,4 @@
-// Copyright 2016 The Cobalt Authors. All Rights Reserved.
+// Copyright 2023 The Cobalt Authors. All Rights Reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.

components/crash/core/common/crash_key.h

@@ -20,6 +20,7 @@
 // Annotation interface. Because not all platforms use Crashpad yet, a
 // source-compatible interface is provided on top of the older Breakpad
 // storage mechanism.
+// TODO(b/286881972): Investigate enabling crashpad support in Cobalt/Telemetry.
 #if BUILDFLAG(USE_CRASHPAD_ANNOTATION) && !defined(STARBOARD)
 #include "third_party/crashpad/crashpad/client/annotation.h"  // nogncheck
 #endif

components/metrics_services_manager/metrics_services_manager.cc

@@ -94,7 +94,6 @@ void MetricsServicesManager::UpdatePermissions(bool current_may_record,
                                                bool current_consent_given,
                                                bool current_may_upload) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
 #if !defined(STARBOARD)
   // If the user has opted out of metrics, delete local UKM state. We Only check
   // consent for UKM.