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) {
+  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;
+  }
+  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 320 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_, current_candidate_index_ + 1);
     // 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();
     best_favicon_ = DownloadedFavicon();
   }
@@ skipping 84 matching lines @@
   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) {
     // 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.
+    RecordAttemptsForHandlerType(handler_type_, current_candidate_index_);

[pkotwicz, 2017/04/10 14:58:11]

Why are we recording Favicons.FaviconDownloadAttempts from here?

[fhorschig, 2017/04/10 15:05:21]

Because TestRecordDownloadAttemptsFinishedByCache would fail.

We have multiple candidates, but the best candidate fails to load.
Then the cached next candidate is used to complete the request.
I considered it important to capture these attempts as well.

[pkotwicz, 2017/04/10 17:26:10]

Sorry for the confusion earlier. I think that we need a dedicated variable which
counts the number of downloads: |num_download_requests_| (because that is what
the histogram is recording)

We should probably only record the metric if |num_download_requests_| is non
zero

Sorry for flip flopping

[fhorschig, 2017/04/11 12:07:20]

I agree, It's more explicit and future-proof in we ever skip candidates.
The recorded number is still exactly the same (incl. 404 as attempts).
Done. Added a check in |OnFaviconData| so |RecordAttempts...| does the action
it implies. Added comment to |OnDidDownload...|.
Don't worry.

     NotifyFaviconUpdated(favicon_bitmap_results);
   }
 
   if (!current_candidate() ||
       (has_results && !DoUrlsAndIconsMatch(current_candidate()->icon_url,
                                            current_candidate()->icon_type,
                                            favicon_bitmap_results))) {
     // The icon URLs have been updated since the favicon data was requested.
     return;
   }
@@ skipping 20 matching lines @@
   // 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