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(
+    FaviconDriverObserver::NotificationIconType handler_type,
+    int attempts) {
+  // The name of the histogram must not be a variable.
+  switch (handler_type) {
+    case FaviconDriverObserver::NON_TOUCH_16_DIP:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.FaviconDownloadAttempts", attempts);
+      return;
+    case FaviconDriverObserver::NON_TOUCH_LARGEST:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.LargeIconDownloadAttempts", attempts);
+      return;
+    case FaviconDriverObserver::TOUCH_LARGEST:
+      UMA_HISTOGRAM_COUNTS_100("Favicons.TouchIconDownloadAttempts", attempts);
+      return;

[pkotwicz, 2017/04/04 13:04:37]

Can we use the same histogram for Favicons.LargeIconDownloadAttempts and
Favicons.TouchIconDownloadAttempts

mastiz@ is trying to consolidate FaviconHandlers for type
FaviconDriverObserver::NON_TOUCH_LARGEST and
FaviconDriverObserver::TOUCH_LARGEST

in https://codereview.chromium.org/2795763004/

[fhorschig, 2017/04/10 09:55:50]

Done.

[fhorschig, 2017/04/10 13:59:41]

I was informed that we might have a large delay on that effort
and we could potentially lose the recorded data if that change
turns out to be pushed back. We discussed that it might make
more sense to consolidate the metrics later, which is still
wasy to do.
Therefore, I readded the differentiation for now. 
WDYT?

+  }
+  NOTREACHED();
+  return;

[pkotwicz, 2017/04/04 13:04:37]

Nit: The return here is redundant

[fhorschig, 2017/04/10 09:55:50]

Gone.

+}
+
 // 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),
+      request_attempts_(0),
       current_candidate_index_(0u) {
   DCHECK(delegate_);
 }
 
 FaviconHandler::~FaviconHandler() {
 }
 
 // static
 int FaviconHandler::GetIconTypesFromHandlerType(
     FaviconDriverObserver::NotificationIconType handler_type) {
@@ skipping 10 matching lines @@
 void FaviconHandler::FetchFavicon(const GURL& url) {
   cancelable_task_tracker_.TryCancelAll();
 
   url_ = url;
 
   initial_history_result_expired_or_incomplete_ = false;
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
   download_request_.Cancel();
   candidates_.clear();
+  request_attempts_ = 0;
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
   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(
@@ skipping 94 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);
+  request_attempts_ = 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 {
+    RecordAttemptsForHandlerType(handler_type_, request_attempts_);

[pkotwicz, 2017/04/04 13:04:38]

Can you store |current_candidate_index_ + 1| instead?

[fhorschig, 2017/04/10 09:55:50]

Done. This captures 404s as attempts as well.

     // 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();
+    request_attempts_ = 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 106 matching lines @@
   // 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;
   }
   download_request_.Reset(base::Bind(&FaviconHandler::OnDidDownloadFavicon,
                                      base::Unretained(this), icon_type));
+  ++request_attempts_;
   // 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