[CloudOpenMetrics] Disable CloudOpenMetrics for multiple files

CloudOpenMetrics was only defined for single files in the cloud
open/upload flow. When the user selects multiple files, metrics like
TaskResult will be logged once for each file. This would lead to the
kIncorrectlyLoggedMultipleTimes state.

Disable CloudOpenMetrics for when multiple files are selected for the
cloud open/upload flow, leave defining this as a TODO.

Bug: b/300861997
Change-Id: Icae8275cb870f2d05b13d2bdf3fa0a3f8d59bbad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4988562
Reviewed-by: Alexander Bolodurin <alexbn@chromium.org>
Commit-Queue: Cassy Chun-Crogan <cassycc@google.com>
Cr-Commit-Position: refs/heads/main@{#1216792}

chrome/browser/ash/file_manager/file_tasks.cc

@@ -809,7 +809,7 @@ bool ExecuteFileTask(Profile* profile,
     const bool started = ExecuteWebDriveOfficeTask(
         profile, task, file_urls, modal_parent,
         std::make_unique<ash::cloud_upload::CloudOpenMetrics>(
-            ash::cloud_upload::CloudProvider::kGoogleDrive));
+            ash::cloud_upload::CloudProvider::kGoogleDrive, file_urls.size()));
     if (done) {
       if (started) {
         std::move(done).Run(
@@ -829,7 +829,7 @@ bool ExecuteFileTask(Profile* profile,
     const bool started = ExecuteOpenInOfficeTask(
         profile, task, file_urls, modal_parent,
         std::make_unique<ash::cloud_upload::CloudOpenMetrics>(
-            ash::cloud_upload::CloudProvider::kOneDrive));
+            ash::cloud_upload::CloudProvider::kOneDrive, file_urls.size()));
     if (done) {
       if (started) {
         std::move(done).Run(

chrome/browser/ash/file_manager/file_tasks_browsertest.cc

@@ -663,7 +663,7 @@ IN_PROC_BROWSER_TEST_P(FileTasksBrowserTest, FallbackSucceedsWithQuickOffice) {
       profile, CreateWebDriveOfficeTask(), {test_url}, nullptr,
       ash::office_fallback::FallbackReason::kOffline,
       std::make_unique<ash::cloud_upload::CloudOpenMetrics>(
-          ash::cloud_upload::CloudProvider::kOneDrive)));
+          ash::cloud_upload::CloudProvider::kOneDrive, /*file_count=*/1)));
 }
 
 IN_PROC_BROWSER_TEST_P(FileTasksBrowserTest, FallbackFailsNoQuickOffice) {
@@ -695,7 +695,7 @@ IN_PROC_BROWSER_TEST_P(FileTasksBrowserTest, FallbackFailsNoQuickOffice) {
       profile, CreateWebDriveOfficeTask(), {test_url}, nullptr,
       ash::office_fallback::FallbackReason::kOffline,
       std::make_unique<ash::cloud_upload::CloudOpenMetrics>(
-          ash::cloud_upload::CloudProvider::kOneDrive)));
+          ash::cloud_upload::CloudProvider::kOneDrive, /*file_count=*/1)));
 }
 #endif  // BUILDFLAG(GOOGLE_CHROME_BRANDING)
 
@@ -1113,7 +1113,7 @@ class DriveTest : public TestAccountBrowserTest {
     // Path of test file relative to the DriveFs mount point.
     relative_test_file_path = base::FilePath("/").AppendASCII(test_file_name_);
     cloud_open_metrics_ = std::make_unique<ash::cloud_upload::CloudOpenMetrics>(
-        ash::cloud_upload::CloudProvider::kGoogleDrive);
+        ash::cloud_upload::CloudProvider::kGoogleDrive, /*file_count=*/1);
     cloud_open_metrics_weak_ptr_ = cloud_open_metrics_->GetWeakPtr();
   }
 
@@ -1473,7 +1473,7 @@ class OneDriveTest : public TestAccountBrowserTest,
     // The path in ODFS is the relative path with "/" prefixed.
     test_path_within_odfs_ = base::FilePath("/").Append(relative_test_path_);
     cloud_open_metrics_ = std::make_unique<ash::cloud_upload::CloudOpenMetrics>(
-        ash::cloud_upload::CloudProvider::kOneDrive);
+        ash::cloud_upload::CloudProvider::kOneDrive, /*file_count=*/1);
     cloud_open_metrics_weak_ptr_ = cloud_open_metrics_->GetWeakPtr();
   }
 

chrome/browser/ui/webui/ash/cloud_upload/cloud_open_metrics.cc

@@ -56,8 +56,10 @@ std::ostream& operator<<(std::ostream& os, const Metric<MetricType>& metric) {
   return os;
 }
 
-CloudOpenMetrics::CloudOpenMetrics(CloudProvider cloud_provider)
-    : cloud_provider_(cloud_provider),
+CloudOpenMetrics::CloudOpenMetrics(CloudProvider cloud_provider,
+                                   size_t file_count)
+    : multiple_files_(file_count > 1),
+      cloud_provider_(cloud_provider),
       copy_error_(cloud_provider_ == CloudProvider::kGoogleDrive
                       ? kGoogleDriveCopyErrorMetricName
                       : kOneDriveCopyErrorMetricName),
@@ -79,8 +81,15 @@ CloudOpenMetrics::CloudOpenMetrics(CloudProvider cloud_provider)
                          ? kGoogleDriveUploadResultMetricName
                          : kOneDriveUploadResultMetricName) {}
 
-// TODO(b/300861997): Dump without crashing if there was an inconsistency.
+// Check metric consistency and update metric states as required. Log the
+// companion metrics with the final metric states. Dump without crashing if an
+// inconsistency was found.
 CloudOpenMetrics::~CloudOpenMetrics() {
+  if (multiple_files_) {
+    // TODO(b/242685536): Define CloudOpenMetrics for multiple files.
+    return;
+  }
+
   bool google_drive = cloud_provider_ == CloudProvider::kGoogleDrive;
   ExpectLogged(task_result_);
   if (task_result_.logged()) {
@@ -538,6 +547,10 @@ base::WeakPtr<CloudOpenMetrics> CloudOpenMetrics::GetWeakPtr() {
 
 template <typename MetricType>
 void CloudOpenMetrics::PrintDebugInformation(Metric<MetricType>& metric) {
+  if (multiple_files_) {
+    // TODO(b/242685536): Define CloudOpenMetrics for multiple files.
+    return;
+  }
   inconsistency_found_ = true;
   LOG(ERROR) << "Inconsistent metric found: " << metric;
   LOG(ERROR) << "Metrics: " << std::endl

chrome/browser/ui/webui/ash/cloud_upload/cloud_open_metrics.h

@@ -122,7 +122,7 @@ inline void Metric<MetricType>::LogMetric(MetricType new_value) {
 // a SafeRef.
 class CloudOpenMetrics {
  public:
-  explicit CloudOpenMetrics(CloudProvider cloud_provider);
+  explicit CloudOpenMetrics(CloudProvider cloud_provider, size_t file_count);
   ~CloudOpenMetrics();
 
   // Not copyable. Create a SafeRef instead.
@@ -164,6 +164,7 @@ class CloudOpenMetrics {
 
  private:
   // Print debug information about the detected inconsistency and every metric.
+  // Record that an inconsistency was found.
   template <typename MetricType>
   void PrintDebugInformation(Metric<MetricType>& metric);
 
@@ -183,6 +184,7 @@ class CloudOpenMetrics {
   void SetWrongValueLogged(Metric<MetricType>& metric);
 
   bool inconsistency_found_ = false;
+  bool multiple_files_;
   CloudProvider cloud_provider_;
   Metric<base::File::Error> copy_error_;
   Metric<base::File::Error> move_error_;

chrome/browser/ui/webui/ash/cloud_upload/cloud_open_metrics_unittest.cc

@@ -129,7 +129,8 @@ int CloudOpenMetricsTest::number_of_dump_calls_ = 0;
 // is logged.
 TEST_F(CloudOpenMetricsTest, TaskResultLogged) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kOpened);
   }
   histogram_.ExpectUniqueSample(kGoogleDriveTaskResultMetricStateMetricName,
@@ -139,7 +140,10 @@ TEST_F(CloudOpenMetricsTest, TaskResultLogged) {
 // Tests that the TaskResult companion metric is set correctly when TaskResult
 // is not logged.
 TEST_F(CloudOpenMetricsTest, TaskResultNotLogged) {
-  { CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive); }
+  {
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
+  }
   histogram_.ExpectUniqueSample(kGoogleDriveTaskResultMetricStateMetricName,
                                 MetricState::kIncorrectlyNotLogged, 1);
   ASSERT_EQ(1, CloudOpenMetricsTest::number_of_dump_calls());
@@ -149,7 +153,8 @@ TEST_F(CloudOpenMetricsTest, TaskResultNotLogged) {
 // is logged twice.
 TEST_F(CloudOpenMetricsTest, TaskResultLoggedTwice) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kOpened);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFailedToOpen);
   }
@@ -159,13 +164,23 @@ TEST_F(CloudOpenMetricsTest, TaskResultLoggedTwice) {
   ASSERT_EQ(1, CloudOpenMetricsTest::number_of_dump_calls());
 }
 
+// Tests that no DumpWithoutCrashing calls were made and the TaskResult
+// companion metric is not logged when when TaskResult is not logged but
+// multiple files were selected.
+TEST_F(CloudOpenMetricsTest, MultipleFilesSelected) {
+  { CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive, 2); }
+  histogram_.ExpectTotalCount(kGoogleDriveTaskResultMetricStateMetricName, 0);
+  ASSERT_EQ(0, CloudOpenMetricsTest::number_of_dump_calls());
+}
+
 // Tests that the TransferRequired, UploadResult and OpenErrors companion
 // metrics are set correctly when TaskResult is logged as kFallbackQuickOffice
 // and they are logged consistently.
 TEST_F(CloudOpenMetricsTest,
        MetricsConsistentWhenTaskResultIsFallbackQuickOffice) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFallbackQuickOffice);
     cloud_open_metrics.LogGoogleDriveOpenError(OfficeDriveOpenErrors::kOffline);
   }
@@ -183,7 +198,8 @@ TEST_F(CloudOpenMetricsTest,
 TEST_F(CloudOpenMetricsTest,
        MetricsInconsistentWhenTaskResultIsFallbackQuickOffice) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFallbackQuickOffice);
     cloud_open_metrics.LogTransferRequired(
         OfficeFilesTransferRequired::kNotRequired);
@@ -205,7 +221,8 @@ TEST_F(CloudOpenMetricsTest,
 TEST_F(CloudOpenMetricsTest,
        MetricsConsistentWhenTaskResultIsCancelledAtConfirmation) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(
         OfficeTaskResult::kCancelledAtConfirmation);
     cloud_open_metrics.LogSourceVolume(
@@ -228,7 +245,8 @@ TEST_F(CloudOpenMetricsTest,
 TEST_F(CloudOpenMetricsTest,
        MetricsInconsistentWhenTaskResultIsCancelledAtConfirmation) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(
         OfficeTaskResult::kCancelledAtConfirmation);
     cloud_open_metrics.LogUploadResult(
@@ -251,7 +269,8 @@ TEST_F(CloudOpenMetricsTest,
 // is logged as kFailedToOpen and it is logged consistently.
 TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenTaskResultIsFailedToOpen) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFailedToOpen);
     cloud_open_metrics.LogOneDriveOpenError(
         OfficeOneDriveOpenErrors::kConversionToODFSUrlError);
@@ -264,7 +283,8 @@ TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenTaskResultIsFailedToOpen) {
 // is logged as kFailedToOpen and it is logged inconsistently.
 TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenTaskResultIsFailedToOpen) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFailedToOpen);
     cloud_open_metrics.LogOneDriveOpenError(OfficeOneDriveOpenErrors::kSuccess);
   }
@@ -278,7 +298,8 @@ TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenTaskResultIsFailedToOpen) {
 // logged consistently.
 TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenTaskResultIsOpened) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kOpened);
     cloud_open_metrics.LogOneDriveOpenError(OfficeOneDriveOpenErrors::kSuccess);
     cloud_open_metrics.LogTransferRequired(
@@ -297,7 +318,8 @@ TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenTaskResultIsOpened) {
 // logged inconsistently.
 TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenTaskResultIsOpened) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kOpened);
     cloud_open_metrics.LogUploadResult(OfficeFilesUploadResult::kSuccess);
     cloud_open_metrics.LogTransferRequired(OfficeFilesTransferRequired::kCopy);
@@ -316,7 +338,8 @@ TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenTaskResultIsOpened) {
 // logged consistently.
 TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenTaskResultIsMoved) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kMoved);
     cloud_open_metrics.LogGoogleDriveOpenError(OfficeDriveOpenErrors::kSuccess);
     cloud_open_metrics.LogUploadResult(OfficeFilesUploadResult::kSuccess);
@@ -335,7 +358,8 @@ TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenTaskResultIsMoved) {
 // logged inconsistently.
 TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenTaskResultIsMoved) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kMoved);
     cloud_open_metrics.LogGoogleDriveOpenError(
         OfficeDriveOpenErrors::kNoMetadata);
@@ -356,7 +380,8 @@ TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenTaskResultIsMoved) {
 TEST_F(CloudOpenMetricsTest,
        MetricsConsistentWhenTransferRequiredIsNotRequired) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTransferRequired(
         OfficeFilesTransferRequired::kNotRequired);
     cloud_open_metrics.LogGoogleDriveOpenError(
@@ -377,7 +402,8 @@ TEST_F(CloudOpenMetricsTest,
 TEST_F(CloudOpenMetricsTest,
        MetricsInconsistentWhenTransferRequiredIsNotRequired) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTransferRequired(
         OfficeFilesTransferRequired::kNotRequired);
     cloud_open_metrics.LogUploadResult(
@@ -400,7 +426,8 @@ TEST_F(CloudOpenMetricsTest,
 TEST_F(CloudOpenMetricsTest,
        MetricsConsistentWhenTransferRequiredIsCopyAndTaskResultIsFailedToOpen) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTransferRequired(OfficeFilesTransferRequired::kCopy);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFailedToOpen);
     cloud_open_metrics.LogUploadResult(
@@ -421,7 +448,8 @@ TEST_F(
     CloudOpenMetricsTest,
     MetricsInconsistentWhenTransferRequiredIsCopyAndTaskResultIsFailedToOpen) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogTransferRequired(OfficeFilesTransferRequired::kCopy);
     cloud_open_metrics.LogTaskResult(OfficeTaskResult::kFailedToOpen);
     cloud_open_metrics.LogSourceVolume(OfficeFilesSourceVolume::kGoogleDrive);
@@ -438,7 +466,8 @@ TEST_F(
 TEST_F(CloudOpenMetricsTest,
        MetricsConsistentWhenUploadResultIsCopyOperationError) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogUploadResult(
         OfficeFilesUploadResult::kCopyOperationError);
     cloud_open_metrics.LogCopyError(
@@ -453,7 +482,8 @@ TEST_F(CloudOpenMetricsTest,
 TEST_F(CloudOpenMetricsTest,
        MetricsInconsistentWhenUploadResultIsCopyOperationError) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogUploadResult(
         OfficeFilesUploadResult::kCopyOperationError);
   }
@@ -466,7 +496,8 @@ TEST_F(CloudOpenMetricsTest,
 // is logged and UploadResult is logged consistently.
 TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenMoveErrorIsLogged) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogMoveError(base::File::Error::FILE_ERROR_NO_SPACE);
     cloud_open_metrics.LogUploadResult(
         OfficeFilesUploadResult::kMoveOperationError);
@@ -479,7 +510,8 @@ TEST_F(CloudOpenMetricsTest, MetricsConsistentWhenMoveErrorIsLogged) {
 // is logged and UploadResult is logged inconsistently.
 TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenMoveErrorIsLogged) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogMoveError(base::File::Error::FILE_ERROR_NO_SPACE);
   }
   histogram_.ExpectUniqueSample(kOneDriveUploadResultMetricStateMetricName,
@@ -491,7 +523,8 @@ TEST_F(CloudOpenMetricsTest, MetricsInconsistentWhenMoveErrorIsLogged) {
 // no dump without crashing.
 TEST_F(CloudOpenMetricsTest, NoDumpWhenAllMetricsAreConsistentForOpenFlow) {
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kOneDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogSourceVolume(
         OfficeFilesSourceVolume::kMicrosoftOneDrive);
     cloud_open_metrics.LogTransferRequired(
@@ -521,7 +554,8 @@ TEST_F(CloudOpenMetricsTest, NoDumpWhenAllMetricsAreConsistentForOpenFlow) {
 TEST_F(CloudOpenMetricsTest, NoDumpWhenAllMetricsAreConsistentForMoveFlow) {
   ASSERT_EQ(0, CloudOpenMetricsTest::number_of_dump_calls());
   {
-    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive);
+    CloudOpenMetrics cloud_open_metrics(CloudProvider::kGoogleDrive,
+                                        /*file_count=*/1);
     cloud_open_metrics.LogSourceVolume(
         OfficeFilesSourceVolume::kMicrosoftOneDrive);
     cloud_open_metrics.LogTransferRequired(OfficeFilesTransferRequired::kMove);