Add support to process favicons from Web Manifests

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..0e0ded5e8bf2baeeeef416f6c2ff1ac3e8cfcb15 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -102,11 +102,14 @@ bool HasExpiredOrIncompleteResult(
   return false;
 }
 
-// Returns true if at least one of |bitmap_results| is valid.
-bool HasValidResult(
+// Finds a valid bitmap in |bitmap_results| or returns nullptr otherwise.
+const favicon_base::FaviconRawBitmapResult* FindValidResult(
     const std::vector<favicon_base::FaviconRawBitmapResult>& bitmap_results) {
-  return std::find_if(bitmap_results.begin(), bitmap_results.end(), IsValid) !=
-      bitmap_results.end();
+  for (const favicon_base::FaviconRawBitmapResult& result : bitmap_results) {
+    if (IsValid(result))
+      return &result;
+  }
+  return nullptr;
 }
 
 std::vector<int> GetDesiredPixelSizes(
@@ -195,7 +198,9 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   initial_history_result_expired_or_incomplete_ = false;
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
-  download_request_.Cancel();
+  manifest_download_request_.Cancel();
+  image_download_request_.Cancel();
+  manifest_url_.reset();
   candidates_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
@@ -243,8 +248,13 @@ bool FaviconHandler::UpdateFaviconCandidate(
 void FaviconHandler::SetFavicon(const GURL& icon_url,
                                 const gfx::Image& image,
                                 favicon_base::IconType icon_type) {
-  if (service_ && ShouldSaveFavicon())
+  if (service_ && ShouldSaveFavicon()) {
     service_->SetFavicons(url_, icon_url, icon_type, image);
+    // If the icon list was coming from a Web Manifest, let's also store
+    // the association using the manifest's URL as page URL.
+    if (manifest_url_)
+      service_->SetFavicons(*manifest_url_, icon_url, icon_type, image);

[pkotwicz, 2017/04/07 14:13:04]

- We will need to add expiry logic specifically for manifest URLs

- We avoid some of these problems if we use the "manifest URL" as the "icon URL"
instead of the "page URL". For instance, we would not need to call SetFavicons()
twice here. When we use the "manifest URL" as the "page URL" we have to call
FaviconService::SetFavicons() twice. We need to store the "page URL" <-> "image
data" association for the sake of the bookmark page and the history page.

[mastiz, 2017/04/10 15:34:12]

The first argument above is convincing to go for icon URLs, and it'd be good to
document that in the Design Doc. I'll change the implementation.

+  }
 
   NotifyFaviconUpdated(icon_url, icon_type, image);
 }
@@ -284,11 +294,32 @@ void FaviconHandler::NotifyFaviconUpdated(const GURL& icon_url,
   notification_icon_type_ = icon_type;
 }
 
-void FaviconHandler::OnUpdateFaviconURL(
+void FaviconHandler::OnUpdateCandidates(
     const GURL& page_url,
-    const std::vector<FaviconURL>& candidates) {
-  if (page_url != url_)
+    const std::vector<FaviconURL>& candidates,
+    const base::Optional<GURL>& manifest_url) {
+  DCHECK_EQ(page_url, url_);
+
+  manifest_url_ = manifest_url;
+
+  // If no manifest available, proceed with the regular candidates only.
+  if (!manifest_url.has_value()) {
+    OnGotFinalIconURLCandidates(candidates);
     return;
+  }
+
+  // See if there is a cached favicon for the manifest.
+  service_->GetFaviconForPageURL(
+      *manifest_url, icon_types_, preferred_icon_size(),
+      base::Bind(&FaviconHandler::OnFaviconDataForManifestURLFromFaviconService,
+                 base::Unretained(this)),
+      &cancelable_task_tracker_);
+}
+
+void FaviconHandler::OnGotFinalIconURLCandidates(
+    const std::vector<FaviconURL>& candidates) {
+  // Mark manifest download as finished, if there was one.
+  manifest_download_request_.Cancel();
 
   std::vector<FaviconCandidate> sorted_candidates;
   const std::vector<int> desired_pixel_sizes =
@@ -309,7 +340,7 @@ void FaviconHandler::OnUpdateFaviconURL(
     return;
   }
 
-  download_request_.Cancel();
+  image_download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
@@ -347,6 +378,39 @@ void FaviconHandler::OnGotInitialHistoryDataAndIconURLCandidates() {
   DownloadCurrentCandidateOrAskFaviconService();
 }
 
+void FaviconHandler::OnFaviconDataForManifestURLFromFaviconService(
+    const std::vector<favicon_base::FaviconRawBitmapResult>&
+        favicon_bitmap_results) {
+  DCHECK(manifest_download_request_.IsCancelled());
+
+  const favicon_base::FaviconRawBitmapResult* valid_result =
+      FindValidResult(favicon_bitmap_results);
+
+  if (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.
+    NotifyFaviconUpdated(favicon_bitmap_results);
+  }
+
+  if (valid_result && !HasExpiredOrIncompleteResult(preferred_icon_size(),
+                                                    favicon_bitmap_results)) {
+    // Register a direct mapping between the page URL and the icons listed in
+    // the manifest.
+    if (service_ && ShouldSaveFavicon()) {
+      service_->MergeFavicon(url_, {valid_result->icon_url},
+                             valid_result->icon_type, valid_result->bitmap_data,
+                             valid_result->pixel_size);

[pkotwicz, 2017/04/07 14:13:04]

Why are you calling FaviconService::MergeFavicon() instead of
FaviconService::SetFavicons()

If we stored the "manifest URL" as the "icon URL" we would be able to use
FaviconService::UpdateFaviconMappingsAndFetch() to establish the "page URL" <->
"manifest URL" mapping

[mastiz, 2017/04/10 15:34:12]

I could explain why but this is a moot point anyway, unless we get back to using
manifest URLs as page URLs.

+    }
+  } else {
+    manifest_download_request_.Reset(base::Bind(
+        &FaviconHandler::OnGotFinalIconURLCandidates, base::Unretained(this)));
+    delegate_->DownloadManifest(*manifest_url_,
+                                manifest_download_request_.callback());
+  }
+}
+
 void FaviconHandler::OnDidDownloadFavicon(
     favicon_base::IconType icon_type,
     int id,
@@ -355,7 +419,7 @@ void FaviconHandler::OnDidDownloadFavicon(
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& original_bitmap_sizes) {
   // Mark download as finished.
-  download_request_.Cancel();
+  image_download_request_.Cancel();
 
   if (bitmaps.empty() && http_status_code == 404) {
     DVLOG(1) << "Failed to Download Favicon:" << image_url;
@@ -419,7 +483,8 @@ const std::vector<GURL> FaviconHandler::GetIconURLs() const {
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
-  return !download_request_.IsCancelled() ||
+  return !manifest_download_request_.IsCancelled() ||
+         !image_download_request_.IsCancelled() ||
          cancelable_task_tracker_.HasTrackedTasks();
 }
 
@@ -435,7 +500,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
     const std::vector<favicon_base::FaviconRawBitmapResult>&
         favicon_bitmap_results) {
   got_favicon_from_history_ = true;
-  bool has_valid_result = HasValidResult(favicon_bitmap_results);
+  bool has_valid_result = FindValidResult(favicon_bitmap_results) != nullptr;
   initial_history_result_expired_or_incomplete_ =
       !has_valid_result ||
       HasExpiredOrIncompleteResult(preferred_icon_size(),
@@ -493,17 +558,25 @@ void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
 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);
+  const favicon_base::FaviconRawBitmapResult* valid_result =
+      FindValidResult(favicon_bitmap_results);
   bool has_expired_or_incomplete_result =
-      !has_valid_result || HasExpiredOrIncompleteResult(preferred_icon_size(),
-                                                        favicon_bitmap_results);
+      !valid_result || HasExpiredOrIncompleteResult(preferred_icon_size(),
+                                                    favicon_bitmap_results);
 
-  if (has_valid_result) {
+  if (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.
     NotifyFaviconUpdated(favicon_bitmap_results);
+    // If a manifest is being used, let's also store the association using the
+    // manifest's URL as page URL.
+    if (manifest_url_ && service_ && ShouldSaveFavicon()) {
+      service_->MergeFavicon(*manifest_url_, {valid_result->icon_url},
+                             valid_result->icon_type, valid_result->bitmap_data,
+                             valid_result->pixel_size);
+    }
   }
 
   if (!current_candidate() ||
@@ -524,21 +597,23 @@ void FaviconHandler::ScheduleDownload(const GURL& image_url,
                                       favicon_base::IconType icon_type) {
   DCHECK(image_url.is_valid());
   // Note that CancelableCallback starts cancelled.
-  DCHECK(download_request_.IsCancelled()) << "More than one ongoing download";
+  DCHECK(image_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));
+  image_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());
+                               image_download_request_.callback());
   DCHECK_NE(download_id, 0);
 }