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..524fa77d316f7d88b72e9e722626c2d9355c9e5a 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -195,7 +195,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;
@@ -244,7 +246,8 @@ void FaviconHandler::SetFavicon(const GURL& icon_url,
                                 const gfx::Image& image,
                                 favicon_base::IconType icon_type) {
   if (service_ && ShouldSaveFavicon())
-    service_->SetFavicons(url_, icon_url, icon_type, image);
+    service_->SetFavicons(url_, manifest_url_.value_or(icon_url), icon_type,
+                          image);
 
   NotifyFaviconUpdated(icon_url, icon_type, image);
 }
@@ -273,23 +276,87 @@ void FaviconHandler::NotifyFaviconUpdated(const GURL& icon_url,
   if (image.IsEmpty())
     return;
 
+  // If a manifest is being processed, it's URL overrides the icon URL.
+  const GURL& notification_icon_url = manifest_url_.value_or(icon_url);
+
   gfx::Image image_with_adjusted_colorspace = image;
   favicon_base::SetFaviconColorSpace(&image_with_adjusted_colorspace);
 
-  delegate_->OnFaviconUpdated(url_, handler_type_, icon_url,
-                              icon_url != notification_icon_url_,
+  delegate_->OnFaviconUpdated(url_, handler_type_, notification_icon_url,
+                              notification_icon_url != notification_icon_url_,
                               image_with_adjusted_colorspace);
 
-  notification_icon_url_ = icon_url;
+  notification_icon_url_ = notification_icon_url;
   notification_icon_type_ = icon_type;
 }
 
-void FaviconHandler::OnUpdateFaviconURL(
+void FaviconHandler::OnUpdateCandidates(
     const GURL& page_url,
-    const std::vector<FaviconURL>& candidates) {
+    const std::vector<FaviconURL>& candidates,
+    const base::Optional<GURL>& manifest_url) {
+  DCHECK_EQ(page_url, url_);
   if (page_url != url_)
     return;
 
+  // TODO/DONOTSUBMIT(mastiz): Not sure if this is reachable at all: perhaps
+  // manifest URL can be specified via Javascript?

[pkotwicz, 2017/04/11 03:47:07]

Yes, the manifest URL can be changed via JavaScript. The browser side code (e.g.
InstallableManager) currently assumes that this never happens.

[mastiz, 2017/04/11 13:31:01]

Ack, thanks. Added a new test.

+  if (manifest_url_ && manifest_url_ == manifest_url)
+    return;
+
+  manifest_url_ = manifest_url;
+
+  // If no manifest available, proceed with the regular candidates only.
+  if (!manifest_url.has_value()) {
+    OnGotFinalIconURLCandidates(candidates);
+    return;
+  }
+
+  if (redownload_icons_) {

[pkotwicz, 2017/04/11 03:47:07]

Do we need to wait till OnFaviconDataForInitialURLFromFaviconService() is called
before proceeding?

[mastiz, 2017/04/11 13:31:01]

redownload_icons_ can only be true only after
OnFaviconDataForInitialURLFromFaviconService(). Added a comment.

[pkotwicz, 2017/04/12 22:10:06]

I'd recommend getting rid of this if() statement and always fetching data for
the manifest URL.

Otherwise there are too many cases to think about:

OnFaviconDataForInitialURLFromFaviconService()
OnUpdateCandidates()
OnGotFinalIconURLCandidates()

OnUpdateCandidates()
OnFaviconDataForInitialURLFromFaviconService()
OnGotFinalIconURLCandidates()

OnUpdateCandidates()
OnGotFinalIconURLCandidates()
OnFaviconDataForInitialURLFromFaviconService()

[mastiz, 2017/04/20 18:06:33]

Done.

+    ScheduleManifestDownload();
+  } else {
+    // See if there is a cached favicon for the manifest.
+    GetFaviconAndUpdateMappingsUnlessIncognito(
+        /*icon_url=*/*manifest_url, favicon_base::FAVICON,
+        base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService,
+                   base::Unretained(this)));
+  }
+}
+
+void FaviconHandler::OnFaviconDataForManifestFromFaviconService(
+    const std::vector<favicon_base::FaviconRawBitmapResult>&
+        favicon_bitmap_results) {
+  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.
+    NotifyFaviconUpdated(favicon_bitmap_results);
+  }
+
+  if (has_expired_or_incomplete_result)
+    ScheduleManifestDownload();
+}
+
+void FaviconHandler::ScheduleManifestDownload() {
+  manifest_download_request_.Reset(base::Bind(
+      &FaviconHandler::OnGotFinalIconURLCandidates, base::Unretained(this)));
+  // If we download a manifest, make need to bypass the lookups in the favicon
+  // cache because the manifest's URL is used as icon URL anyway.
+  redownload_icons_ = true;
+  delegate_->DownloadManifest(*manifest_url_,
+                              manifest_download_request_.callback());

[pkotwicz, 2017/04/11 03:47:07]

We need to take into account that the Web Manifest might not have any icons.

For instance:
https://www.washingtonpost.com/pb/resources/json/desktop-notifications-manife...

[mastiz, 2017/04/11 13:31:01]

Done, implemented one proposal for this.

Unless you have better ideas, I had to cache this in our in-memory 404 table for
icons. I believe a cleaner solution would involve changes in the history
backend.

[pkotwicz, 2017/04/12 22:10:06]

This is OK for now 

+}
+
+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 =
       GetDesiredPixelSizes(handler_type_);
@@ -309,7 +376,7 @@ void FaviconHandler::OnUpdateFaviconURL(
     return;
   }
 
-  download_request_.Cancel();
+  image_download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
@@ -355,7 +422,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 +486,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();
 }
 
@@ -459,34 +527,42 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
 }
 
 void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
-  GURL icon_url = current_candidate()->icon_url;
-  favicon_base::IconType icon_type = current_candidate()->icon_type;
-
+  const GURL icon_url = current_candidate()->icon_url;
+  const favicon_base::IconType icon_type = current_candidate()->icon_type;
   if (redownload_icons_) {
     // We have the mapping, but the favicon is out of date. Download it now.
-    ScheduleDownload(icon_url, icon_type);
-  } else if (service_) {
-    // We don't know the favicon, but we may have previously downloaded the
-    // favicon for another page that shares the same favicon. Ask for the
-    // favicon given the favicon URL.
-    if (delegate_->IsOffTheRecord()) {
-      service_->GetFavicon(
-          icon_url, icon_type, preferred_icon_size(),
-          base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
-          &cancelable_task_tracker_);
-    } else {
-      // Ask the history service for the icon. This does two things:
-      // 1. Attempts to fetch the favicon data from the database.
-      // 2. If the favicon exists in the database, this updates the database to
-      //    include the mapping between the page url and the favicon url.
-      // This is asynchronous. The history service will call back when done.
-      // TODO(pkotwicz): pass in all of |image_urls_| to
-      // UpdateFaviconMappingsAndFetch().
-      service_->UpdateFaviconMappingsAndFetch(
-          url_, {icon_url}, icon_type, preferred_icon_size(),
-          base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
-          &cancelable_task_tracker_);
-    }
+    ScheduleFaviconDownload(icon_url, icon_type);
+  } else {
+    GetFaviconAndUpdateMappingsUnlessIncognito(
+        icon_url, icon_type,
+        base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)));
+  }
+}
+
+void FaviconHandler::GetFaviconAndUpdateMappingsUnlessIncognito(
+    const GURL& icon_url,
+    favicon_base::IconType icon_type,
+    const favicon_base::FaviconResultsCallback& callback) {
+  if (!service_)
+    return;
+
+  // We don't know the favicon, but we may have previously downloaded the
+  // favicon for another page that shares the same favicon. Ask for the
+  // favicon given the favicon URL.
+  if (delegate_->IsOffTheRecord()) {
+    service_->GetFavicon(icon_url, icon_type, preferred_icon_size(), callback,
+                         &cancelable_task_tracker_);
+  } else {
+    // Ask the history service for the icon. This does two things:
+    // 1. Attempts to fetch the favicon data from the database.
+    // 2. If the favicon exists in the database, this updates the database to
+    //    include the mapping between the page url and the favicon url.
+    // This is asynchronous. The history service will call back when done.
+    // TODO(pkotwicz): pass in all of |image_urls_| to
+    // UpdateFaviconMappingsAndFetch().
+    service_->UpdateFaviconMappingsAndFetch(url_, {icon_url}, icon_type,
+                                            preferred_icon_size(), callback,
+                                            &cancelable_task_tracker_);
   }
 }
 
@@ -511,34 +587,37 @@ void FaviconHandler::OnFaviconData(const std::vector<
                                            current_candidate()->icon_type,
                                            favicon_bitmap_results))) {
     // The icon URLs have been updated since the favicon data was requested.
+    NOTREACHED();  // DONOTSUBMIT
     return;
   }
 
   if (has_expired_or_incomplete_result) {
-    ScheduleDownload(current_candidate()->icon_url,
-                     current_candidate()->icon_type);
+    ScheduleFaviconDownload(current_candidate()->icon_url,
+                            current_candidate()->icon_type);
   }
 }
 
-void FaviconHandler::ScheduleDownload(const GURL& image_url,
-                                      favicon_base::IconType icon_type) {
+void FaviconHandler::ScheduleFaviconDownload(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);
 }