Simplify FaviconHandler::OnFaviconData()

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 <vector>
 
@@ skipping 622 matching lines @@
     }
   }
 }
 
 void FaviconHandler::OnFaviconData(const std::vector<
     favicon_base::FaviconRawBitmapResult>& favicon_bitmap_results) {
   if (PageChangedSinceFaviconWasRequested())
     return;
 
   bool has_results = !favicon_bitmap_results.empty();
   bool has_expired_or_incomplete_result = HasExpiredOrIncompleteResult(

[Roger McFarlane (Chromium), 2015/04/27 15:38:30]

has_results && HasExpired...?

See ONFaviconDataForInitialURFromFaviconService()

[pkotwicz, 2015/04/27 18:26:36]

I have changed HasExpiredOrIncompleteResult() to return true if the passed in
FaviconRawBitmapResult vector is empty

       preferred_icon_size(), favicon_bitmap_results);

[pkotwicz, 2015/04/13 04:55:18]

Yes, storing this in |favicon_expired_or_incomplete_| would be more correct, but
that's for another time

   bool has_valid_result = HasValidResult(favicon_bitmap_results);
+  history_results_ = favicon_bitmap_results;

[Roger McFarlane (Chromium), 2015/04/13 17:45:37]

Do you need to save the results if they're not valid?

How independent are these booleans?

[pkotwicz, 2015/04/25 01:14:48]

It is unclear what the correct behavior is.
- The only user of |history_results_| is ProcessCurrentUrl(). Thus
|history_results_| and |favicon_expired_or_incomplete_| should be updated at the
same time.
This matters when OnUpdateFaviconURL() gets called after OnFaviconData() has
been called for a particular page URL
- It would be nice if FaviconHandler (ProcessCurrentUrl() in particular())
depended less on state from FaviconDriver. To that end it would probably be nice
to update |history_results_| only when HasValidResult() returns true.

[pkotwicz, 2015/04/25 01:19:41]

The reason that it might be nice to only update |history_results_| when
HasValidResult() returns true is that this is "kind of" when
NotifyFaviconAvailable() is called.

Figuring out why the logic for when NotifyFaviconAvailable() is called in
OnFaviconDataForInitialURLFromFaviconService() and OnFaviconData() is different
would be useful

 
-  if (has_results && handler_type_ == FAVICON && !download_largest_icon_) {
-    if (has_valid_result) {
-      // There is a favicon, set it now. If expired we'll download the current
-      // one again, but at least the user will get some icon instead of the
-      // default and most likely the current one is fine anyway.
-      NotifyFaviconAvailable(favicon_bitmap_results);
-    }
-    if (has_expired_or_incomplete_result) {
-      // The favicon is out of date. Request the current one.
-      ScheduleDownload(driver_->GetActiveURL(),
-                       driver_->GetActiveFaviconURL(),
-                       favicon_base::FAVICON);
-    }
-  } else if (current_candidate() &&
-      (!has_results || has_expired_or_incomplete_result ||
-       !(DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results)))) {
-    // We don't know the favicon, it is out of date or its type is not same as
-    // one got from page. Request the current one.

[pkotwicz, 2015/04/13 04:55:18]

The results should be empty if the type is different than the one that we got
from the page. In particular, we pass in just one type to
UpdateFaviconMappingsAndFetch() and GetFaviconFromFaviconService()

+  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.
+    NotifyFaviconAvailable(favicon_bitmap_results);
+  }
+
+  if (!current_candidate() ||
+      (has_results &&
+       !DoUrlsAndIconsMatch(*current_candidate(), favicon_bitmap_results))) {
+    // The icon URLs have been updated since the favicon data was requested.
+    return;
+  }
+
+  if (!has_results || has_expired_or_incomplete_result) {
     ScheduleDownload(driver_->GetActiveURL(),
                      current_candidate()->icon_url,
                      current_candidate()->icon_type);
   }
-  history_results_ = favicon_bitmap_results;
-
-  if (has_valid_result &&
-      (handler_type_ != FAVICON || download_largest_icon_)) {
-    NotifyFaviconAvailable(favicon_bitmap_results);
-  }
 }
 
 void FaviconHandler::ScheduleDownload(const GURL& url,
                                      const GURL& image_url,
                                      favicon_base::IconType 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 = DownloadFavicon(image_url,
                                           GetMaximalIconSize(icon_type));
@@ skipping 14 matching lines @@
     gfx::Size largest =
         image_url.icon_sizes[GetLargestSizeIndex(image_url.icon_sizes)];
     image_url.icon_sizes.clear();
     image_url.icon_sizes.push_back(largest);
   }
   std::stable_sort(image_urls_.begin(), image_urls_.end(),
                    CompareIconSize);
 }
 
 }  // namespace favicon