Refactor histogram_persistence to be a class.

chrome/browser/chrome_browser_field_trials.cc

Index: chrome/browser/chrome_browser_field_trials.cc
diff --git a/chrome/browser/chrome_browser_field_trials.cc b/chrome/browser/chrome_browser_field_trials.cc
index 682271f04904f4c105a633e168a60741ceeeff5f..9a3f95ab032a850f807b87b6f7626b8d1346baf2 100644
--- a/chrome/browser/chrome_browser_field_trials.cc
+++ b/chrome/browser/chrome_browser_field_trials.cc
@@ -10,7 +10,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_base.h"
-#include "base/metrics/histogram_persistence.h"
+#include "base/metrics/persistent_histogram_allocator.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
@@ -33,12 +33,14 @@ void InstantiatePersistentHistograms() {
     // Create persistent/shared memory and allow histograms to be stored in it.
     // Memory that is not actualy used won't be physically mapped by the system.
     // BrowserMetrics usage peaked around 95% of 2MiB as of 2016-02-20.
-    base::SetPersistentHistogramMemoryAllocator(
-        new base::LocalPersistentMemoryAllocator(3 << 20,     // 3 MiB
-                                                 0x935DDD43,  // SHA1(B...M...)
-                                                 kAllocatorName));
-    base::GetPersistentHistogramMemoryAllocator()->CreateTrackingHistograms(
-        kAllocatorName);
+    base::PersistentHistogramAllocator::SetGlobalAllocator(
+        make_scoped_ptr(new base::PersistentHistogramAllocator(
+            make_scoped_ptr(new base::LocalPersistentMemoryAllocator(

[Alexei Svitkine (slow), 2016/03/07 16:40:43]

How about making a helper static method that takes the three params below and
hides the extra complexity of allocating the objects inside it?

Then, callers can be much cleaner, e.g.:

base::PersistentHistogramAllocator::InitGlobalAllocator(
    3 << 20,     // 3 MiB
    0x935DDD43,  // SHA1(BrowserMetrics)
    kAllocatorName);

[bcwhite, 2016/03/08 00:24:04]

This whole method _is_ a helper method.  :-)

I can't reduce it to parameters to a more general helper method, one that would
be useful outside of this file, because the type (e.g.
LocalPersistentMemoryAllocator) would also need to be passed... and a template
won't work because the parameters to different types may be different.

[Alexei Svitkine (slow), 2016/03/08 19:58:28]

There's like half a dozen places where you use this pattern, though I now see
that there's two patterns - one with a LocalPMA and other with just a PMA.
Still, I think it would be cleaner to hide some of the complexity inside the API
rather than on the call site.

So I suggest making two helper functions:

base::PersistentHistogramAllocator::InitGlobalAllocator() (for LocalPMA)
and:
base::PersistentHistogramAllocator::InitGlobalAllocatorOverMemory() (for PMA
over memory w/ extra params)

And simplifying the params as I suggest above - which would make the call sites
much cleaner.

[bcwhite, 2016/03/09 04:01:51]

Done.  Though personally I like the explicit creation of it.  I like to see what
is going on.

+                3 << 20,     // 3 MiB
+                0x935DDD43,  // SHA1(BrowserMetrics)
+                kAllocatorName)))));
+    base::PersistentHistogramAllocator::GetGlobalAllocator()
+        ->CreateTrackingHistograms(kAllocatorName);
   }
 }