Add browser_test to check that NaCl is logging UMA samples correctly.

chrome/test/nacl/nacl_browsertest.cc

Index: chrome/test/nacl/nacl_browsertest.cc
diff --git a/chrome/test/nacl/nacl_browsertest.cc b/chrome/test/nacl/nacl_browsertest.cc
index 8e5cf3f20f19dfb024e448ce1d0dbaf2786daf43..65c6ee81ba8171a85ad39b7044a0e5790f853ec7 100644
--- a/chrome/test/nacl/nacl_browsertest.cc
+++ b/chrome/test/nacl/nacl_browsertest.cc
@@ -4,6 +4,8 @@
 
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/statistics_recorder.h"
 #include "base/path_service.h"
 #include "base/timer.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
@@ -12,13 +14,16 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/dom_operation_notification_details.h"
+#include "content/public/browser/histogram_fetcher.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "native_client/src/trusted/service_runtime/nacl_error_code.h"
 #include "net/base/net_util.h"
+#include "ppapi/native_client/src/trusted/plugin/plugin_error.h"
 #include "webkit/plugins/webplugininfo.h"
 
 namespace {
@@ -312,6 +317,100 @@ IN_PROC_BROWSER_TEST_F(NaClBrowserTestGLibc, SimpleLoadTest) {
   RunLoadTest(FILE_PATH_LITERAL("nacl_load_test.html"));
 }
 
+class HistogramHelper {

[Mark Seaborn, 2012/08/21 19:41:37]

Should this all be in a separate file to be more obviously findable?  The UMA
test is fairly big.

[Nick Bray (chromium), 2012/08/24 18:17:48]

I agree, but I was planning to do structural reforms later.  The bigger question
is how to set things up so we can easily instantiate multiple variants of the
same test (newlib, glibc, pnacl) and be able to define tests in different files.
 This will require some polishing.

+ public:
+  HistogramHelper() {}
+
+  // Each child process may have its own histogram data, make sure this data
+  // gets accumulated into the browser process before we examine the histograms.
+  void Fetch() {
+    base::Closure callback = base::Bind(
+        &HistogramHelper::FetchCallback,

[jar (doing other things), 2012/08/23 00:35:39]

nit: move 328 to end of 327.  For function calls, no reason to  wrap like this. 
You can also align 329 start with the paren.

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+        base::Unretained(this));
+
+    content::FetchHistogramsAsynchronously(
+        MessageLoop::current(), callback,

[Mark Seaborn, 2012/08/21 19:41:37]

Should you be creating a new message loop object so that you're not modifying a
global when you call Quit()?

[Nick Bray (chromium), 2012/08/24 18:17:48]

As far as I know, this is idiomatic for browser_tests.

+        base::TimeDelta::FromMilliseconds(60000));

[Mark Seaborn, 2012/08/21 19:41:37]

Comment: "give up after 60s, which anyway is longer than the brower tests'
current timeout of ???s".

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+    content::RunMessageLoop();
+  }
+
+  // We know the exact number of samples in a bucket, and that no other bucket
+  // should have samples.
+  void ExpectUniqueSample(const std::string &name, size_t bucket,

[Mark Seaborn, 2012/08/21 19:41:37]

"&" spacing style.  'bucket' -> 'bucket_id', perhaps?

[jar (doing other things), 2012/08/23 00:35:39]

nit: For function definitions... one arg per line (align at paren is best).

[Mark Seaborn, 2012/08/24 19:30:42]

Ping.

[Nick Bray (chromium), 2012/08/24 21:13:56]

Done.

+                          base::Histogram::Count count) {

[Mark Seaborn, 2012/08/21 19:41:37]

'count' -> 'expected_count', just for clarity?  Same below.

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+    base::Histogram* histogram = base::StatisticsRecorder::FindHistogram(name);
+    ASSERT_NE((base::Histogram*)NULL, histogram) << "Histogram \"" << name <<

[jar (doing other things), 2012/08/23 00:35:39]

nit: avoid C style cast (here and below). Best is static_cast, but you may need
reinterpret_cast here.

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+        "\" does not exist.";
+
+    base::Histogram::SampleSet samples;
+    histogram->SnapshotSample(&samples);
+    CheckBucketCount(name, bucket, count, samples);
+    CheckTotalCount(name, count, samples);
+  }
+
+  // We don't know the values of the samples, but we know how many there are.
+  void ExpectTotalCount(const std::string &name, base::Histogram::Count count) {

[Mark Seaborn, 2012/08/21 19:41:37]

"&" spacing style.

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+    base::Histogram* histogram = base::StatisticsRecorder::FindHistogram(name);
+    ASSERT_NE((base::Histogram*)NULL, histogram) << "Histogram \"" << name <<
+        "\" does not exist.";
+
+    base::Histogram::SampleSet samples;
+    histogram->SnapshotSample(&samples);
+    CheckTotalCount(name, count, samples);
+  }
+
+ private:
+  void FetchCallback() {
+    MessageLoopForUI::current()->Quit();
+  }
+
+  void CheckBucketCount(
+      const std::string &name,
+      size_t bucket,
+      base::Histogram::Count count,
+      base::Histogram::SampleSet& samples) {

[jar (doing other things), 2012/08/23 00:35:39]

nit: prefer aligning args with paren in definition.

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+    EXPECT_EQ(count, samples.counts(bucket)) << "Histogram \"" << name <<
+        "\" does not have the right number of samples (" << count <<
+        ") in the expected bucket (" << bucket << ").";
+  }
+
+  void CheckTotalCount(
+      const std::string &name,
+      base::Histogram::Count count,
+      base::Histogram::SampleSet& samples) {
+    EXPECT_EQ(count, samples.TotalCount()) << "Histogram \"" << name <<
+        "\" does not have the right total number of samples (" << count << ").";
+  }
+};
+
+// TODO(ncbray) create multiple variants via macros.

[Mark Seaborn, 2012/08/21 19:41:37]

Multiple variants to do what?

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+IN_PROC_BROWSER_TEST_F(NaClBrowserTestNewlib, UMALoadTest) {
+  // Sanity check.
+  ASSERT_TRUE(base::StatisticsRecorder::IsActive());
+
+  // Load a NaCl module to generate UMA data.
+  RunLoadTest(FILE_PATH_LITERAL("nacl_load_test.html"));
+
+  // Make sure histograms from child processes have been accumulated in the
+  // browser brocess.
+  HistogramHelper histograms;
+  histograms.Fetch();
+
+  // Did the plugin report success?
+  histograms.ExpectUniqueSample("NaCl.LoadStatus.Plugin",
+                                plugin::ERROR_LOAD_SUCCESS, 1);
+
+  // Did the sel_ldr report success?
+  histograms.ExpectUniqueSample("NaCl.LoadStatus.SelLdr",
+                                LOAD_OK, 1);
+
+  // Make sure we have other important histograms.
+  histograms.ExpectTotalCount("NaCl.Perf.StartupTime.LoadModule", 1);
+  histograms.ExpectTotalCount("NaCl.Perf.StartupTime.Total", 1);
+  histograms.ExpectTotalCount("NaCl.Perf.Size.Manifest", 1);
+  histograms.ExpectTotalCount("NaCl.Perf.Size.Nexe", 1);

[Mark Seaborn, 2012/08/21 19:41:37]

Can you add a comment saying something like:

This test is a replacement for native_client/tests/pyauto_nacl/nacl_uma.py. 
TODO(ncbray): Convert the rest of that script, including [insert examples or
comprehensive list :-) ...]

[Nick Bray (chromium), 2012/08/24 18:17:48]

Done.

+}
+
 #endif  // !(defined(ADDRESS_SANITIZER) && defined(OS_LINUX))
 
 }  // namespace anonymous