Safety check Chrome cleaner: log metrics only when user runs a SC

Bug: 1087263
Change-Id: Ie404c10b503f7951007ec6eba445fdffe8ab2a3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2513655Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Commit-Queue: Rainhard Findling <rainhard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823550}

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

@@ -473,6 +473,12 @@ void SafetyCheckHandler::CheckChromeCleaner() {
     // current state.
     safe_browsing::ChromeCleanerController::GetInstance()->AddObserver(this);
   }
+  // Log the current status into metrics.
+  if (chrome_cleaner_status_ != ChromeCleanerStatus::kHidden &&
+      chrome_cleaner_status_ != ChromeCleanerStatus::kChecking) {
+    base::UmaHistogramEnumeration("Settings.SafetyCheck.ChromeCleanerResult",
+                                  chrome_cleaner_status_);
+  }
 }
 #endif
 
@@ -550,11 +556,6 @@ void SafetyCheckHandler::OnChromeCleanerCheckResult(
       GetStringForChromeCleaner(result.status, result.cct_completion_time,
                                 timestamp_delegate_->GetSystemTime()));
   FireWebUIListener(kChromeCleanerEvent, event);
-  if (result.status != ChromeCleanerStatus::kHidden &&
-      result.status != ChromeCleanerStatus::kChecking) {
-    base::UmaHistogramEnumeration("Settings.SafetyCheck.ChromeCleanerResult",
-                                  result.status);
-  }
   chrome_cleaner_status_ = result.status;
   CompleteParentIfChildrenCompleted();
 }

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

@@ -139,6 +139,20 @@ class SafetyCheckHandler
   base::string16 GetStringForChromeCleanerRan();
   base::string16 GetStringForChromeCleanerRan(base::Time cct_completion_time,
                                               base::Time system_time);
+
+  // safe_browsing::ChromeCleanerController::Observer overrides.
+  void OnIdle(
+      safe_browsing::ChromeCleanerController::IdleReason idle_reason) override;
+  void OnReporterRunning() override;
+  void OnScanning() override;
+  void OnInfected(bool is_powered_by_partner,
+                  const safe_browsing::ChromeCleanerScannerResults&
+                      scanner_results) override;
+  void OnCleaning(bool is_powered_by_partner,
+                  const safe_browsing::ChromeCleanerScannerResults&
+                      scanner_results) override;
+  void OnRebootRequired() override;
+  void OnRebootFailed() override;
 #endif
 
  protected:
@@ -270,22 +284,6 @@ class SafetyCheckHandler
       password_manager::InsecureCredentialsManager::CredentialsView credentials)
       override;
 
-#if defined(OS_WIN) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
-  // safe_browsing::ChromeCleanerController::Observer overrides.
-  void OnIdle(
-      safe_browsing::ChromeCleanerController::IdleReason idle_reason) override;
-  void OnReporterRunning() override;
-  void OnScanning() override;
-  void OnInfected(bool is_powered_by_partner,
-                  const safe_browsing::ChromeCleanerScannerResults&
-                      scanner_results) override;
-  void OnCleaning(bool is_powered_by_partner,
-                  const safe_browsing::ChromeCleanerScannerResults&
-                      scanner_results) override;
-  void OnRebootRequired() override;
-  void OnRebootFailed() override;
-#endif
-
   // SettingsPageUIHandler implementation.
   void OnJavascriptAllowed() override;
   void OnJavascriptDisallowed() override;

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

@@ -1572,6 +1572,36 @@ TEST_F(SafetyCheckHandlerTest, CheckChromeCleaner_DisabledByAdmin) {
       "href=\"https://support.google.com/chrome?p=your_administrator\">Your "
       "administrator</a> has disabled Browser's check for harmful software");
 }
+
+TEST_F(SafetyCheckHandlerTest, CheckChromeCleaner_ObserverUpdateLogging) {
+  safe_browsing::ChromeCleanerControllerImpl::ResetInstanceForTesting();
+  safe_browsing::ChromeCleanerControllerImpl::GetInstance()->SetIdleForTesting(
+      safe_browsing::ChromeCleanerController::IdleReason::
+          kReporterFoundNothing);
+  // We expect a user triggering a safety check to log the Chrome cleaner
+  // result.
+  safety_check_->PerformSafetyCheck();
+  histogram_tester_.ExpectBucketCount(
+      "Settings.SafetyCheck.ChromeCleanerResult",
+      SafetyCheckHandler::ChromeCleanerStatus::kNoUwsFoundWithTimestamp, 1);
+  // Subsequent Chrome cleaner status updates without the user running safety
+  // check again should not trigger logging.
+  safety_check_->OnIdle(safe_browsing::ChromeCleanerController::IdleReason::
+                            kReporterFoundNothing);
+  safety_check_->OnReporterRunning();
+  safety_check_->OnScanning();
+  safety_check_->OnRebootRequired();
+  safety_check_->OnRebootFailed();
+  histogram_tester_.ExpectBucketCount(
+      "Settings.SafetyCheck.ChromeCleanerResult",
+      SafetyCheckHandler::ChromeCleanerStatus::kNoUwsFoundWithTimestamp, 1);
+  histogram_tester_.ExpectBucketCount(
+      "Settings.SafetyCheck.ChromeCleanerResult",
+      SafetyCheckHandler::ChromeCleanerStatus::kRebootRequired, 0);
+  histogram_tester_.ExpectBucketCount(
+      "Settings.SafetyCheck.ChromeCleanerResult",
+      SafetyCheckHandler::ChromeCleanerStatus::kScanningForUws, 0);
+}
 #endif
 
 TEST_F(SafetyCheckHandlerTest, CheckParentRanDisplayString) {