Add ExitFunnel to prepare for instrumenting browser exits.

components/browser_watcher/watcher_metrics_provider_win_unittest.cc

Index: components/browser_watcher/watcher_metrics_provider_win_unittest.cc
diff --git a/components/browser_watcher/watcher_metrics_provider_win_unittest.cc b/components/browser_watcher/watcher_metrics_provider_win_unittest.cc
index 9002a171536660b2033955e549ef6ff5f841b989..9e4e31a45f1d15357bebafc8a12eb721bb7f320d 100644
--- a/components/browser_watcher/watcher_metrics_provider_win_unittest.cc
+++ b/components/browser_watcher/watcher_metrics_provider_win_unittest.cc
@@ -56,6 +56,15 @@ class WatcherMetricsProviderWinTest : public testing::Test {
     return key.GetValueCount();
   }
 
+  void AddExitFunnelEvent(int pid, const base::char16* name, int64 value) {
+    base::string16 key_name =
+        base::StringPrintf(L"%ls\\%d-%d", kRegistryPath, pid, pid);

[erikwright (departed), 2014/12/11 15:00:46]

Why dup the writing? You're testing (and overly coupling your tests to)
implementation.

You should have a reader and a writer. And the public contract of the two is
that what I write with the Writer can be read from the Reader. Your tests
shouldn't concern themselves with how that data is stored under the hood.

[Sigurður Ásgeirsson, 2014/12/12 16:21:40]

There's really not enough code there to worry about reuse. Also, making the
writing reusable for testing means to allow specifying the time written.

+
+    base::win::RegKey key(HKEY_CURRENT_USER, key_name.c_str(), KEY_WRITE);
+    ASSERT_EQ(key.WriteValue(name, &value, sizeof(value), REG_QWORD),
+              ERROR_SUCCESS);
+  }
+
  protected:
   registry_util::RegistryOverrideManager override_manager_;
   base::HistogramTester histogram_tester_;
@@ -103,4 +112,23 @@ TEST_F(WatcherMetricsProviderWinTest, DoesNotReportOwnProcessId) {
   EXPECT_EQ(ExitCodeRegistryPathValueCount(), 1);
 }
 
+TEST_F(WatcherMetricsProviderWinTest, RecordsExitFunnel) {
+  // Record an exit funnel - note the recorded times are in microseconds,
+  // but the reporting is in milliseconds, hence the times 1000.
+  AddExitFunnelEvent(100, L"One", 1000 * 1000);
+  AddExitFunnelEvent(100, L"Two", 1010 * 1000);
+  AddExitFunnelEvent(100, L"Three", 990 * 1000);
+
+  WatcherMetricsProviderWin provider(kRegistryPath);
+
+  provider.ProvideStabilityMetrics(NULL);

[erikwright (departed), 2014/12/11 15:00:46]

Testing that the histograms are actually written is good, but it's going to be
inconvenient to get really thorough coverage of your reading code if that's the
only way to do it.

Take the reading code out of the WatcherMetricsProvider, test the hell out of
it, and then you can easily get high coverage of the WatcherMetricsProvider by
making the simple verification that it does in fact send histograms.

[Sigurður Ásgeirsson, 2014/12/12 16:21:40]

I added some testing to verify that the metrics provider can read what the exit
funnel writes. I'd like to ship this as-is, rather than sinking further work
into refining it. This code won't live long, and we only need confidence that
it's not reporting rubbish.

+  histogram_tester_.ExpectUniqueSample("Stability.ExitFunnel.Three", 0, 1);
+  histogram_tester_.ExpectUniqueSample("Stability.ExitFunnel.One", 10, 1);
+  histogram_tester_.ExpectUniqueSample("Stability.ExitFunnel.Two", 20, 1);
+
+  // Make sure the subkey is deleted on reporting.
+  base::win::RegistryKeyIterator it(HKEY_CURRENT_USER, kRegistryPath);
+  ASSERT_EQ(it.SubkeyCount(), 0);
+}
+
 }  // namespace browser_watcher