Primary histograms for InBrowserCleanerUI experiment

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc

Index: chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
diff --git a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
index 0ea7937d7e838017721cd74ace026eed07c8420c..2c3221197d2bacab3e7a1c973808f99d717decf1 100644
--- a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
+++ b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc
@@ -15,6 +15,7 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/task_scheduler/task_traits.h"
 #include "base/threading/thread_restrictions.h"
@@ -27,6 +28,7 @@
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/settings_resetter_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/srt_client_info_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
 #include "chrome/installer/util/scoped_token_privilege.h"
 #include "components/chrome_cleaner/public/constants/constants.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
@@ -100,8 +102,17 @@ ChromeCleanerController::IdleReason IdleReasonWhenConnectionClosedTooSoon(
              : ChromeCleanerController::IdleReason::kConnectionLost;
 }
 
+void RecordCleanerLogsAcceptanceHistogram(bool value) {

[csharp, 2017/07/07 17:32:51]

what about logs_uploaded instead of value?

[ftirelo, 2017/07/07 20:27:52]

Used logs_accepted instead.

+  UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.CleanerLogsAcceptance", value);
+}
+
 }  // namespace
 
+void RecordCleanupStartedHistogram(CleanupStartedHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.CleanupStarted", value,
+                            CLEANUP_STARTED_MAX);
+}
+
 ChromeCleanerControllerDelegate::ChromeCleanerControllerDelegate() = default;
 
 ChromeCleanerControllerDelegate::~ChromeCleanerControllerDelegate() = default;
@@ -231,12 +242,14 @@ void ChromeCleanerController::ReplyWithUserResponse(
     case UserResponse::kAcceptedWithLogs:
       acceptance = PromptAcceptance::ACCEPTED_WITH_LOGS;
       SetLogsEnabled(true);
+      RecordCleanerLogsAcceptanceHistogram(true);
       new_state = State::kCleaning;
       delegate_->TagForResetting(profile);
       break;
     case UserResponse::kAcceptedWithoutLogs:
       acceptance = PromptAcceptance::ACCEPTED_WITHOUT_LOGS;
       SetLogsEnabled(false);
+      RecordCleanerLogsAcceptanceHistogram(false);
       new_state = State::kCleaning;
       delegate_->TagForResetting(profile);
       break;
@@ -264,6 +277,7 @@ void ChromeCleanerController::Reboot() {
   if (state() != State::kRebootRequired)
     return;
 
+  UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.RebootInitiated", true);
   InitiateReboot();
 }
 
@@ -329,6 +343,8 @@ void ChromeCleanerController::OnChromeCleanerFetchedAndVerified(
   if (executable_path.empty()) {
     idle_reason_ = IdleReason::kScanningFailed;
     SetStateAndNotifyObservers(State::kIdle);
+    RecordPromptNotShownWithReasonHistogram(
+        NO_PROMPT_REASON_CLEANER_DOWNLOAD_FAILED);
     return;
   }
 
@@ -391,9 +407,12 @@ void ChromeCleanerController::OnPromptUser(
                                              PromptAcceptance::DENIED));
     idle_reason_ = IdleReason::kScanningFoundNothing;
     SetStateAndNotifyObservers(State::kIdle);
+    RecordPromptNotShownWithReasonHistogram(NO_PROMPT_REASON_NOTHING_FOUND);
     return;
   }
 
+  UMA_HISTOGRAM_COUNTS_1000("SoftwareReporter.NumberOfFilesToDelete",
+                            files_to_delete->size());

[csharp, 2017/07/07 17:32:51]

Is there any problem if files_to_delete->size() > 1000? I vaguely remember
hearing about problems on the back end if we gave values a lot higher than what
the count should have been

[ftirelo, 2017/07/07 20:27:52]

There is an overflow buffer for counts > 1000. Since less than 0.5% of file
lists are larger than that, we should be fine.

   files_to_delete_ = std::move(files_to_delete);
   prompt_user_callback_ = std::move(prompt_user_callback);
   SetStateAndNotifyObservers(State::kInfected);
@@ -407,6 +426,8 @@ void ChromeCleanerController::OnConnectionClosed() {
   if (state() == State::kScanning || state() == State::kInfected) {
     idle_reason_ = IdleReasonWhenConnectionClosedTooSoon(state());
     SetStateAndNotifyObservers(State::kIdle);
+    RecordPromptNotShownWithReasonHistogram(
+        NO_PROMPT_REASON_IPC_CONNECTION_BROKEN);

[csharp, 2017/07/07 17:32:51]

Is it possible for the connect to be closed after the user has seen the prompt?

[ftirelo, 2017/07/07 20:27:52]

Yes, if the cleaner binary crashes.

[csharp, 2017/07/07 20:53:33]

Should this be counted then, since in that case wouldn't we have already
recorded the prompt as being shown?

[ftirelo, 2017/07/07 21:22:07]

Added a new histogram for moment when IPC connection was lost and changed this
to only register non-prompt if state is kScanning.

     return;
   }
   // Nothing to do if OnConnectionClosed() is called in other states:
@@ -440,6 +461,7 @@ void ChromeCleanerController::OnCleanerProcessDone(
   }
 
   if (process_status.exit_code == kRebootRequiredExitCode) {
+    UMA_HISTOGRAM_BOOLEAN("SoftwareReporter.RebootRequired", true);
     SetStateAndNotifyObservers(State::kRebootRequired);
     return;
   }