Fix the Chrome OS ResourceReporter

chrome/browser/chromeos/resource_reporter/resource_reporter.h

Index: chrome/browser/chromeos/resource_reporter/resource_reporter.h
diff --git a/chrome/browser/chromeos/resource_reporter/resource_reporter.h b/chrome/browser/chromeos/resource_reporter/resource_reporter.h
index 763740a3fa2d124e4953c47a6f913d3d95d5602d..7cfb7da60b00f6e8d73b0600fbc75c8a5cb41e62 100644
--- a/chrome/browser/chromeos/resource_reporter/resource_reporter.h
+++ b/chrome/browser/chromeos/resource_reporter/resource_reporter.h
@@ -8,7 +8,6 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include <map>
 #include <string>
 #include <vector>
 
@@ -16,9 +15,9 @@
 #include "base/macros.h"
 #include "base/memory/memory_pressure_listener.h"
 #include "base/memory/singleton.h"
-#include "base/time/time.h"
 #include "chrome/browser/task_manager/task_manager_observer.h"
 #include "components/metrics/metrics_service.h"
+#include "components/prefs/pref_registry_simple.h"
 #include "components/rappor/sample.h"
 
 namespace chromeos {
@@ -66,14 +65,15 @@ class ResourceReporter : public task_manager::TaskManagerObserver {
   // The singleton instance.
   static ResourceReporter* GetInstance();
 
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
   // Start / stop observing the task manager and the memory pressure events.
   void StartMonitoring();
   void StopMonitoring();
 
   // task_manager::TaskManagerObserver:
-  void OnTaskAdded(task_manager::TaskId id) override;
-  void OnTaskToBeRemoved(task_manager::TaskId id) override;
-  void OnTasksRefreshed(const task_manager::TaskIdList& task_ids) override;
+  void OnTasksRefreshedWithBackgroundCalculations(
+      const task_manager::TaskIdList& task_ids) override;
 
  private:
   friend struct base::DefaultSingletonTraits<ResourceReporter>;
@@ -129,9 +129,9 @@ class ResourceReporter : public task_manager::TaskManagerObserver {
     NUM_RANGES            = 7,
   };
 
-  // The maximum number of top consumer tasks of each resource that we're
-  // interested in reporting.
-  static const size_t kTopConsumersCount;
+  // The CPU and memory thresholds beyond which the tasks will be reported.
+  static const double kTaskCpuThresholdForReporting;
+  static const int64_t kTaskMemoryThresholdForReporting;
 
   ResourceReporter();
 
@@ -154,51 +154,35 @@ class ResourceReporter : public task_manager::TaskManagerObserver {
   const TaskRecord* SampleTaskByCpu() const;
   const TaskRecord* SampleTaskByMemory() const;
 
+  // Does the actual recording of Rappor and UMA samples.
+  void ReportSamples();
+
   // The callback function that will be invoked on memory pressure events.
   using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
   void OnMemoryPressure(MemoryPressureLevel memory_pressure_level);
 
-  // We'll use this to watch for memory pressure events so that we can trigger
-  // Rappor sampling at at the critical memory pressure level.
+  // Monitor memory pressure events.
   std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
 
-  // Contains the collected data about the currently running tasks from the most
-  // recent task manager refresh.
-  std::map<task_manager::TaskId, std::unique_ptr<TaskRecord>> task_records_;
-
-  // Contains the top |kTopConsumerCount| CPU consumer tasks sorted in a
-  // descending order by their CPU usage.
-  std::vector<TaskRecord*> task_records_by_cpu_;
+  // Contains the data about the most CPU and memory intensive tasks collected
+  // at a critical memory pressure event.
+  std::vector<TaskRecord> task_records_;
 
-  // Contains the top |kTopConsumerCount| memory consumer tasks sorted in a
-  // descending order by their memory usage.
-  std::vector<TaskRecord*> task_records_by_memory_;
-
-  // The time at which the previous critical memory pressure event was received
-  // at which we recorded Rappor samples. This is used to limit generating a
-  // Rappor report to once per |kMinimumTimeBetweenReportsInMS|.
-  // This is needed to avoid generating a lot of samples that can lower the
-  // Rappor privacy guarantees.
-  base::TimeTicks last_memory_pressure_event_time_;
+  // The task manager used for unit tests.
+  task_manager::TaskManagerInterface* task_manager_for_testing_;

[ncarter (slow), 2016/10/07 19:04:16]

A for_testing_ data member should be a last resort, since it'll result in
codeblocks in the .cc file that are unreachable in unittests. Two ways I see to
switch this to dependency injection:

(1) is to pass the TaskManagerInterface in the ctor. That's awkward to
impossible with a singleton, so I'd suggest instead:

(2) Pass a TaskManagerInterface* to StartMonitoring.

Then this member can be, like, task_manager_to_observe_, with a comment. Then
ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() just gets the real TM
singleton instance, and passes it on.

[afakhry, 2016/10/12 16:37:36]

That's a great suggestion actually! Done.
Thanks!

 
   // The range that includes the number of CPU cores in the current system.
   const CpuCoresNumberRange system_cpu_cores_range_;
 
   // The most recent reading for the browser and GPU processes to be reported as
   // UMA histograms when the system is under critical memory pressure.
-  double last_browser_process_cpu_ = 0.0;
-  double last_gpu_process_cpu_ = 0.0;
-  int64_t last_browser_process_memory_ = 0;
-  int64_t last_gpu_process_memory_ = 0;
+  double last_browser_process_cpu_;
+  double last_gpu_process_cpu_;
+  int64_t last_browser_process_memory_;
+  int64_t last_gpu_process_memory_;
 
   // Tracks whether monitoring started or not.
-  bool is_monitoring_ = false;
-
-  // True after we've seen a critical memory pressure event.
-  bool have_seen_first_memory_pressure_event_ = false;
-
-  // True after the first task manager OnTasksRefreshed() event is received.
-  bool have_seen_first_task_manager_refresh_ = false;
+  bool is_monitoring_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceReporter);
 };