Add DownloadStatus metric to FaviconHandler

components/favicon/core/favicon_handler.cc

Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 64c8961ae23ad760d06e826ee6e04bf06b7f1b2d..3c8a0ef969e6dbf7858af050a4a39bf83660615a 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "components/favicon/core/favicon_service.h"
 #include "components/favicon_base/favicon_util.h"
@@ -59,6 +60,31 @@ bool IsValid(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   return bitmap_result.is_valid();
 }
 
+void RecordDownloadAttemptsForHandlerType(
+    FaviconDriverObserver::NotificationIconType handler_type,
+    int attempts) {
+  switch (handler_type) {
+    case FaviconDriverObserver::NON_TOUCH_16_DIP:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.DownloadAttempts.Favicons", attempts);
+      return;
+    case FaviconDriverObserver::NON_TOUCH_LARGEST:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.DownloadAttempts.LargeIcons",
+                               attempts);
+      return;
+    case FaviconDriverObserver::TOUCH_LARGEST:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.DownloadAttempts.TouchIcons",
+                               attempts);
+      return;
+  }
+  NOTREACHED();
+}
+
+void RecordDownloadOutcome(FaviconHandler::DownloadOutcome outcome) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Favicons.DownloadOutcome", static_cast<int>(outcome),

[Ilya Sherman, 2017/04/13 21:24:43]

nit: The static_cast should no longer be necessary.  Are you getting a compile
error without it?

[fhorschig, 2017/04/18 15:27:18]

Removed. Nice to know that this works!

+      FaviconHandler::DownloadOutcome::DOWNLOAD_OUTCOME_COUNT);
+}
+
 // Returns true if |bitmap_results| is non-empty and:
 // - At least one of the bitmaps in |bitmap_results| is expired
 // OR
@@ -167,6 +193,7 @@ FaviconHandler::FaviconHandler(
       notification_icon_type_(favicon_base::INVALID_ICON),
       service_(service),
       delegate_(delegate),
+      num_download_requests_(0),
       current_candidate_index_(0u) {
   DCHECK(delegate_);
 }
@@ -199,6 +226,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   candidates_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
+  num_download_requests_ = 0;
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
@@ -311,6 +339,7 @@ void FaviconHandler::OnUpdateFaviconURL(
 
   download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
+  num_download_requests_ = 0;
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
@@ -359,12 +388,14 @@ void FaviconHandler::OnDidDownloadFavicon(
 
   if (bitmaps.empty() && http_status_code == 404) {
     DVLOG(1) << "Failed to Download Favicon:" << image_url;
+    RecordDownloadOutcome(DownloadOutcome::FAILED);
     if (service_)
       service_->UnableToDownloadFavicon(image_url);
   }
 
   bool request_next_icon = true;
   if (!bitmaps.empty()) {
+    RecordDownloadOutcome(DownloadOutcome::SUCCEEDED);
     float score = 0.0f;
     gfx::ImageSkia image_skia;
     if (download_largest_icon_) {
@@ -399,6 +430,9 @@ void FaviconHandler::OnDidDownloadFavicon(
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
+    // OnDidDownloadFavicon() can only be called after requesting a download, so
+    // |num_download_requests_| can never be 0.
+    RecordDownloadAttemptsForHandlerType(handler_type_, num_download_requests_);
     // We have either found the ideal candidate or run out of candidates.
     if (best_favicon_.candidate.icon_type != favicon_base::INVALID_ICON) {
       // No more icons to request, set the favicon from the candidate.
@@ -407,6 +441,7 @@ void FaviconHandler::OnDidDownloadFavicon(
     }
     // Clear download related state.
     current_candidate_index_ = candidates_.size();
+    num_download_requests_ = 0;
     best_favicon_ = DownloadedFavicon();
   }
 }
@@ -517,6 +552,8 @@ void FaviconHandler::OnFaviconData(const std::vector<
   if (has_expired_or_incomplete_result) {
     ScheduleDownload(current_candidate()->icon_url,
                      current_candidate()->icon_type);
+  } else if (num_download_requests_ > 0) {
+    RecordDownloadAttemptsForHandlerType(handler_type_, num_download_requests_);
   }
 }
 
@@ -527,10 +564,12 @@ void FaviconHandler::ScheduleDownload(const GURL& image_url,
   DCHECK(download_request_.IsCancelled()) << "More than one ongoing download";
   if (service_ && service_->WasUnableToDownloadFavicon(image_url)) {
     DVLOG(1) << "Skip Failed FavIcon: " << image_url;
+    RecordDownloadOutcome(DownloadOutcome::SKIPPED);
     OnDidDownloadFavicon(icon_type, 0, 0, image_url, std::vector<SkBitmap>(),
                          std::vector<gfx::Size>());
     return;
   }
+  ++num_download_requests_;
   download_request_.Reset(base::Bind(&FaviconHandler::OnDidDownloadFavicon,
                                      base::Unretained(this), icon_type));
   // A max bitmap size is specified to avoid receiving huge bitmaps in