[Doodle] Record UMA for DoodleConfig download outcome and time

components/doodle/doodle_service.h

Index: components/doodle/doodle_service.h
diff --git a/components/doodle/doodle_service.h b/components/doodle/doodle_service.h
index 3b519f8d06ede7eadd328a4e6fcdd6b6427ab955..9d6c7fcf4313969aa5b656655470e8625b8a25d2 100644
--- a/components/doodle/doodle_service.h
+++ b/components/doodle/doodle_service.h
@@ -11,6 +11,7 @@
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "base/time/clock.h"
+#include "base/time/tick_clock.h"
 #include "base/timer/timer.h"
 #include "components/doodle/doodle_fetcher.h"
 #include "components/doodle/doodle_types.h"
@@ -38,7 +39,8 @@ class DoodleService : public KeyedService {
   DoodleService(PrefService* pref_service,
                 std::unique_ptr<DoodleFetcher> fetcher,
                 std::unique_ptr<base::OneShotTimer> expiry_timer,
-                std::unique_ptr<base::Clock> clock);
+                std::unique_ptr<base::Clock> clock,
+                std::unique_ptr<base::TickClock> tick_clock);
   ~DoodleService() override;
 
   // KeyedService implementation.
@@ -61,13 +63,34 @@ class DoodleService : public KeyedService {
   void Refresh();
 
  private:
+  // Note: Keep in sync with the corresponding enum in histograms.xml. Never
+  // remove values, and only insert new values at the end.
+  enum DownloadOutcome {

[fhorschig, 2017/03/21 16:41:34]

Question: could we use enum class DownloadOutcome: int {} here?
If so, should we?
(assigning fixed ints could reduce the danger of someone reordering them btw.)

[Marc Treib, 2017/03/21 16:48:59]

Yes.
No, because enum class doesn't work with UMA_HISTOGRAM_ENUMERATION (we'd have to
static_cast to int and use UMA_HISTOGRAM_LINEAR_COUNTS.. ugh).
Sure, done.

+    OUTCOME_NEW_DOODLE,
+    OUTCOME_REVALIDATED_DOODLE,
+    OUTCOME_CHANGED_DOODLE,
+    OUTCOME_NO_DOODLE,
+    OUTCOME_EXPIRED,
+    OUTCOME_DOWNLOAD_ERROR,
+    OUTCOME_PARSING_ERROR,
+    // Insert new values here!
+    OUTCOME_COUNT
+  };
+
+  static bool DownloadOutcomeIsSuccess(DownloadOutcome outcome);

[fhorschig, 2017/03/21 16:41:34]

It's static because it couldn't access DownloadOutcome otherwise?

[Marc Treib, 2017/03/21 16:48:59]

It's a *member* because otherwise it couldn't see DownloadOutcome. It's static
because it doesn't need instance state.
</smartass> (scnr :P)

+  static void RecordDownloadMetrics(DownloadOutcome outcome,
+                                    base::TimeDelta download_time);
+
   // Callback for the fetcher.
-  void DoodleFetched(DoodleState state,
+  void DoodleFetched(base::TimeTicks start_time,
+                     DoodleState state,
                      base::TimeDelta time_to_live,
                      const base::Optional<DoodleConfig>& doodle_config);
 
-  void UpdateCachedConfig(base::TimeDelta time_to_live,
-                          const base::Optional<DoodleConfig>& doodle_config);
+  DownloadOutcome UpdateCachedConfig(
+      DoodleState state,
+      base::TimeDelta time_to_live,
+      const base::Optional<DoodleConfig>& doodle_config);
 
   // Callback for the expiry timer.
   void DoodleExpired();
@@ -79,6 +102,7 @@ class DoodleService : public KeyedService {
 
   std::unique_ptr<base::OneShotTimer> expiry_timer_;
   std::unique_ptr<base::Clock> clock_;
+  std::unique_ptr<base::TickClock> tick_clock_;
 
   // The result of the last network fetch.
   base::Optional<DoodleConfig> cached_config_;