Add attempt count metric to FaviconHandler

components/favicon/core/favicon_handler.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 "components/favicon/core/favicon_handler.h"
 
 #include <algorithm>
 #include <cmath>
 #include <utility>
 #include <vector>
 
 #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"
 #include "components/favicon_base/select_favicon_frames.h"
 #include "skia/ext/image_operations.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_util.h"
 
 namespace favicon {
@@ skipping 27 matching lines @@
 // Return true if |bitmap_result| is expired.
 bool IsExpired(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   return bitmap_result.expired;
 }
 
 // Return true if |bitmap_result| is valid.
 bool IsValid(const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   return bitmap_result.is_valid();
 }
 
+void RecordAttemptsForHandlerType(

[pkotwicz, 2017/04/12 02:48:20]

Maybe rename this method to: RecordDownloadAttemptsForHandlerType()

[fhorschig, 2017/04/12 08:32:39]

Done.

+    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();
+}
+
 // Returns true if |bitmap_results| is non-empty and:
 // - At least one of the bitmaps in |bitmap_results| is expired
 // OR
 // - |bitmap_results| is missing favicons for |desired_size_in_dip| and one of
 //   the scale factors in favicon_base::GetFaviconScales().
 bool HasExpiredOrIncompleteResult(
     int desired_size_in_dip,
     const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results) {
   if (bitmap_results.empty())
     return false;
@@ skipping 88 matching lines @@
       got_favicon_from_history_(false),
       initial_history_result_expired_or_incomplete_(false),
       redownload_icons_(false),
       icon_types_(FaviconHandler::GetIconTypesFromHandlerType(handler_type)),
       download_largest_icon_(
           handler_type == FaviconDriverObserver::NON_TOUCH_LARGEST ||
           handler_type == FaviconDriverObserver::TOUCH_LARGEST),
       notification_icon_type_(favicon_base::INVALID_ICON),
       service_(service),
       delegate_(delegate),
+      num_download_requests_(0),
       current_candidate_index_(0u) {
   DCHECK(delegate_);
 }
 
 FaviconHandler::~FaviconHandler() {
 }
 
 // static
 int FaviconHandler::GetIconTypesFromHandlerType(
     FaviconDriverObserver::NotificationIconType handler_type) {
@@ skipping 12 matching lines @@
 
   url_ = url;
 
   initial_history_result_expired_or_incomplete_ = false;
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
   download_request_.Cancel();
   candidates_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
+  num_download_requests_ = 0;
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well WebContents) when the favicon url is
   // available.
   if (service_) {
     service_->GetFaviconForPageURL(
         url_, icon_types_, preferred_icon_size(),
         base::Bind(
@@ skipping 92 matching lines @@
                    &FaviconCandidate::CompareScore);
 
   if (candidates_.size() == sorted_candidates.size() &&
       std::equal(sorted_candidates.begin(), sorted_candidates.end(),
                  candidates_.begin())) {
     return;
   }
 
   download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
+  num_download_requests_ = 0;
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
   if (current_candidate() && got_favicon_from_history_)
     OnGotInitialHistoryDataAndIconURLCandidates();
 }
 
 // static
@@ skipping 68 matching lines @@
       downloaded_favicon.candidate.score = score;
       request_next_icon = !UpdateFaviconCandidate(downloaded_favicon);
     }
   }
 
   if (request_next_icon && current_candidate_index_ + 1 < candidates_.size()) {
     // Process the next candidate.
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
+    // OnDidDownloadFavicon can only be called after invoking a request, so
+    // |num_download_requests_| can never be 0.

[pkotwicz, 2017/04/12 02:48:21]

Nit: OnDidDownloadFavicon -> OnDidDownloadFavicon()

"invoking a request" -> "requesting a download"

[fhorschig, 2017/04/12 08:32:39]

Done.

+    RecordAttemptsForHandlerType(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.
       SetFavicon(best_favicon_.candidate.icon_url, best_favicon_.image,
                  best_favicon_.candidate.icon_type);
     }
     // Clear download related state.
     current_candidate_index_ = candidates_.size();
+    num_download_requests_ = 0;
     best_favicon_ = DownloadedFavicon();
   }
 }
 
 const std::vector<GURL> FaviconHandler::GetIconURLs() const {
   std::vector<GURL> icon_urls;
   for (const FaviconCandidate& candidate : candidates_)
     icon_urls.push_back(candidate.icon_url);
   return icon_urls;
 }
@@ skipping 72 matching lines @@
 
 void FaviconHandler::OnFaviconData(const std::vector<
     favicon_base::FaviconRawBitmapResult>& favicon_bitmap_results) {
   bool has_results = !favicon_bitmap_results.empty();
   bool has_valid_result = HasValidResult(favicon_bitmap_results);
   bool has_expired_or_incomplete_result =
       !has_valid_result || HasExpiredOrIncompleteResult(preferred_icon_size(),
                                                         favicon_bitmap_results);
 
   if (has_valid_result) {
+    if (num_download_requests_ > 0) {
+      RecordAttemptsForHandlerType(handler_type_, current_candidate_index_);

[pkotwicz, 2017/04/12 02:48:21]

Don't you want to pass |num_download_requests_| here?

You should record this in the else{} statement of
if(has_expired_or_incomplete_result){}
If the icon is expired the current code records the UMA metric twice

[fhorschig, 2017/04/12 08:32:39]

Yes, done.
Done. (It hasn't happened yet because num_download_requests_ was usually 0 at
this
point. There is a test verifying that now.)

+    }
     // There is a valid favicon. Notify any observers. It is useful to notify
     // the observers even if the favicon is expired or incomplete (incorrect
     // size) because temporarily showing the user an expired favicon or
     // streched favicon is preferable to showing the user the default favicon.
     NotifyFaviconUpdated(favicon_bitmap_results);
   }
 
   if (!current_candidate() ||
       (has_results && !DoUrlsAndIconsMatch(current_candidate()->icon_url,
                                            current_candidate()->icon_type,
@@ skipping 12 matching lines @@
                                       favicon_base::IconType icon_type) {
   DCHECK(image_url.is_valid());
   // Note that CancelableCallback starts cancelled.
   DCHECK(download_request_.IsCancelled()) << "More than one ongoing download";
   if (service_ && service_->WasUnableToDownloadFavicon(image_url)) {
     DVLOG(1) << "Skip Failed FavIcon: " << image_url;
     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
   // OnDidDownloadFavicon(). See FaviconDriver::StartDownload()
   // for more details about the max bitmap size.
   const int download_id =
       delegate_->DownloadImage(image_url, GetMaximalIconSize(handler_type_),
                                download_request_.callback());
   DCHECK_NE(download_id, 0);
 }
 
 }  // namespace favicon