Leak reports collect information about the memory usage

chrome/browser/metrics/metrics_memory_details.cc

Index: chrome/browser/metrics/metrics_memory_details.cc
diff --git a/chrome/browser/metrics/metrics_memory_details.cc b/chrome/browser/metrics/metrics_memory_details.cc
index f8ccb556074e1419b88758d1fd1ffa04f65a2f80..46b9728c00a9715bf8d363107a6d57200c037283 100644
--- a/chrome/browser/metrics/metrics_memory_details.cc
+++ b/chrome/browser/metrics/metrics_memory_details.cc
@@ -56,17 +56,23 @@ bool MemoryGrowthTracker::UpdateSample(base::ProcessId pid,
 }
 
 MetricsMemoryDetails::MetricsMemoryDetails(
-    const base::Closure& callback,
-    MemoryGrowthTracker* memory_growth_tracker)
-    : callback_(callback), memory_growth_tracker_(memory_growth_tracker) {
-  memory_growth_tracker_ = memory_growth_tracker;
-}
+    const base::Closure& callback, MemoryGrowthTracker* memory_growth_tracker)

[Alexei Svitkine (slow), 2016/10/12 21:46:12]

Nit: I think convention is 1 param per line if first param is on a new line. But
if this is what "git cl format" generates, I'm ok with this too.

[mwlodar, 2016/10/12 22:45:39]

Done.

+    : callback_(callback), memory_growth_tracker_(memory_growth_tracker),
+      generate_histograms_(true) {}
+
+MetricsMemoryDetails::MetricsMemoryDetails(
+    const base::Closure& callback, MemoryGrowthTracker* memory_growth_tracker,
+    bool generate_histograms)
+    : callback_(callback), memory_growth_tracker_(memory_growth_tracker),
+      generate_histograms_(generate_histograms) {}

[Alexei Svitkine (slow), 2016/10/12 21:46:12]

Hmm, instead of a separate constructor, why not just make a setter?

[mwlodar, 2016/10/12 22:45:39]

Done.

 
 MetricsMemoryDetails::~MetricsMemoryDetails() {
 }
 
 void MetricsMemoryDetails::OnDetailsAvailable() {
-  UpdateHistograms();
+  if (generate_histograms_)
+    UpdateHistograms();
+  AnalyzeMemoryGrowth();
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback_);
 }
 
@@ -120,15 +126,6 @@ void MetricsMemoryDetails::UpdateHistograms() {
                                           sample / 1024);
             UMA_HISTOGRAM_MEMORY_LARGE_MB("Memory.Renderer.Committed",
                                           committed / 1024);
-            int diff;
-            if (memory_growth_tracker_ &&
-                memory_growth_tracker_->UpdateSample(
-                    browser.processes[index].pid, sample, &diff)) {
-              if (diff < 0)
-                UMA_HISTOGRAM_MEMORY_KB("Memory.RendererShrinkIn30Min", -diff);
-              else
-                UMA_HISTOGRAM_MEMORY_KB("Memory.RendererGrowthIn30Min", diff);
-            }
             renderer_count++;
             continue;
         }
@@ -311,3 +308,21 @@ void MetricsMemoryDetails::UpdateSwapHistograms() {
   }
 }
 #endif  // defined(OS_CHROMEOS)
+
+void MetricsMemoryDetails::AnalyzeMemoryGrowth() {
+  const ProcessData& browser = *ChromeBrowser();
+  for (size_t index = 0; index < browser.processes.size(); index++) {

[Alexei Svitkine (slow), 2016/10/12 21:46:12]

Nit: use a C++ for loop:

for (const auto& process_entry : GetBrowser()->processes) {

}

[mwlodar, 2016/10/12 22:45:39]

Done.

+    int sample = static_cast<int>(browser.processes[index].working_set.priv);
+
+    int diff;
+    if (memory_growth_tracker_ && memory_growth_tracker_->UpdateSample(
+          browser.processes[index].pid, sample, &diff)) {
+      if (generate_histograms_) {

[Alexei Svitkine (slow), 2016/10/12 21:46:12]

Nit: Merge this if with the one you have above.

[mwlodar, 2016/10/12 22:45:39]

Done. This part is non-trivial though and I added a comment to warn against
changing the order of conditions.

+        if (diff < 0)
+          UMA_HISTOGRAM_MEMORY_KB("Memory.RendererShrinkIn30Min", -diff);
+        else
+          UMA_HISTOGRAM_MEMORY_KB("Memory.RendererGrowthIn30Min", diff);
+      }
+    }
+  }
+}