Serialize/Deserialize support in HistogramBase

content/browser/histogram_synchronizer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/histogram_synchronizer.h"
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/pickle.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "content/browser/histogram_controller.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/histogram_fetcher.h"
 #include "content/public/common/content_constants.h"
 
 using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
@@ skipping 244 matching lines @@
 void HistogramSynchronizer::OnHistogramDataCollected(
     int sequence_number,
     const std::vector<std::string>& pickled_histograms) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   RequestContext* request = RequestContext::GetRequestContext(sequence_number);
 
   for (std::vector<std::string>::const_iterator it = pickled_histograms.begin();
        it < pickled_histograms.end();
        ++it) {
-    base::Histogram::DeserializeHistogramInfo(*it);
+    Pickle pickle(it->data(), it->size());
+    PickleIterator iter(pickle);
+    base::HistogramBase* histogram =
+        base::HistogramBase::DeserializeHistogramInfo(&iter);
+    if (histogram) {

[Ilya Sherman, 2012/12/29 00:17:30]

Optional nit: IMO it's tidier to write "if (!histogram) continue" to reduce the
level of indentation.

[kaiwang, 2013/01/08 00:51:40]

Done.

+      if (histogram->flags() &
+          base::HistogramBase::kIPCSerializationSourceFlag) {
+        DVLOG(1) << "Single process mode, histogram observed and not copied: "
+                 << histogram->histogram_name();
+      } else {
+        histogram->AddSamplesFromPickle(&iter);

[Ilya Sherman, 2012/12/29 00:17:30]

Why isn't this done as part of DeserializeHistogramInfo() (which I would suggest
renaming to DeserializeHistogram())?  Would you ever want to deserialize just
the construction parameters, and not the samples?

[kaiwang, 2013/01/08 00:51:40]

IMO
1. histogram is in base/, so it should know nothing about inter process. 
2. We are actually not "deserializing" the histogram with samples, but find the
histogram and then add the diff. So it's a bit odd to put them into a same
function.

[Ilya Sherman, 2013/01/08 22:31:53]

But it already does know about inter-process communication -- the
kIPCSerializationSourceFlag is in the base namespace...
It just seems a fragile to rely on the fact that DeserializeHistogramInfo()
leaves the iterator in just the right place for you to be able to pick it up and
continue reading from it here.  If the two functions did not read from the same
"stream", I would be less concerned about their separation.

+      }
+    }
   }
 
   if (!request)
     return;
 
   // Delete request if we have heard back from all child processes.
   request->DecrementProcessesPending();
   request->DeleteIfAllDone();
 }
 
@@ skipping 50 matching lines @@
         kHistogramSynchronizerReservedSequenceNumber + 1;
   }
   DCHECK_NE(last_used_sequence_number_,
             kHistogramSynchronizerReservedSequenceNumber);
   if (requester == ASYNC_HISTOGRAMS)
     async_sequence_number_ = last_used_sequence_number_;
   return last_used_sequence_number_;
 }
 
 }  // namespace content