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..7f86a235cd485a4b3043a12c48305ff540619381 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,115 @@ 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;
 

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

Shouldn't the TOUCH_LARGEST FaviconHandler completely ignore the Web Manifest.

It is weird that both the TOUCH_LARGEST and NON_TOUCH_LARGEST FaviconHandlers
will download the Web Manifest

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

This happens in upper layers: the manifest URL is provided only to one of the
two handlers.

[pkotwicz, 2017/04/24 14:42:14]

I see. This is done in FaviconDriverImpl

+  // |candidates| or |manifest_url| could have been modified via Javascript.
+  if (manifest_url_ && manifest_url_ == manifest_url)
+    return;
+
+  manifest_url_ = manifest_url;
+  non_manifest_original_candidates_ = candidates;
+
+  // Check if the manifest previously returned a 404 (or a 200 but contained no
+  // icons), and ignore the manifest URL if that's the case.
+  if (manifest_url_ && service_ &&
+      service_->WasUnableToDownloadFavicon(*manifest_url_)) {
+    DVLOG(1) << "Skip failed Manifest: " << *manifest_url_;
+    manifest_url_.reset();
+  }
+
+  // If no manifest available, proceed with the regular candidates only.
+  if (!manifest_url_) {
+    OnGotFinalIconURLCandidates(candidates);
+    return;
+  }
+
+  if (redownload_icons_) {
+    // Note that |redownload_icons_| can only be true after the database lookup
+    // for the page URL is completed.
+    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::OnDidDownloadManifest, base::Unretained(this)));
+  delegate_->DownloadManifest(*manifest_url_,
+                              manifest_download_request_.callback());
+}
+
+void FaviconHandler::OnDidDownloadManifest(
+    int status_code,
+    const std::vector<FaviconURL>& candidates) {
+  // Mark manifest download as finished.
+  manifest_download_request_.Cancel();
+
+  if (!candidates.empty()) {
+    DCHECK(status_code == 200);

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

Why the DCHECK() ?

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

To add clarity, but I realize you're not a big fan of CHECKs so removed.

+    OnGotFinalIconURLCandidates(candidates);

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

You want to return here?

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

Correct, done (btw tests were failing due to this).

+  }
+
+  // If either the downloading of the manifest failed, OR the manifest contains
+  // no icons, proceed with the list of icons listed in the HTML.
+  DVLOG(1) << "Could not fetch Manifest icons from " << *manifest_url_
+           << ", falling back to inlined ones, which are "
+           << non_manifest_original_candidates_.size();
+
+  if (service_ && (status_code == 404 || status_code == 200))

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

Why is this if() checking the status code?

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

We shouldn't call UnableToDownloadFavicon() for all HTTP response codes like
503. Similar logic exists for images, in OnDidDownloadFavicon.

[pkotwicz, 2017/04/24 14:42:15]

Perhaps FaviconHandler::OnDidDownloadManifest() should take a manifest as an
argument and you can check
!manifest->IsEmpty() && manifest->icons.empty()
instead of checking for |status_code| == 200

It is super weird that you are trying to infer whether the Web Manifest download
succeeded based on the status code

[mastiz, 2017/04/27 13:54:50]

How would that distinguish a 404 from 503?
I don't find this weird at all. Can you elaborate why you find this weird? It's
analogous to image download, and I think we have legit reasons to be interested
in HTTP status code (please refer to earlier question).

[pkotwicz, 2017/05/01 04:56:53]

I suggest doing this:

if (status_code == 404 || (!manifest->IsEmpty() && manifest->icons.empty()) {
}

If the HTTP status code is 503, content::Manifest::IsEmpty() should return true

I think that checking for status code 200 weird. It is weird because you are
checking for status code == 200 as a round about way for:
- the Web Manifest was fetched correctly
- the Web Manifest has no icon URLs

[mastiz, 2017/05/04 10:57:53]

Would that work for a manifest that returns a 200 but is empty or contains
invalid JSON? We wouldn't blacklist it, then. I think verifying non-emptiness is
as weird as the 200 comparison, for the purpose of inferring whether a manifest
was fetched correctly.

Perhaps we should blacklist anything other than 5xx? The only reason why we want
this is because we assume some errors are temporary.

[pkotwicz, 2017/05/04 17:28:24]

Black listing anything other than 5xx sounds reasonable

[mastiz, 2017/05/10 10:03:51]

Done.

[pkotwicz, 2017/05/12 06:13:28]

Did you forget to upload a patch set. There does not seem to be any 5xx handling
in this CL anymore. (And the UnknownManifestWithoutIconsAndKnownRegularIcons
test is failing now)

[mastiz, 2017/05/12 13:31:32]

This was done previously but I've reverted it (in fact removed all HTTP status
code handling) for a later patch, dependent on crbug.com/720289. Meanwhile, the
idea is to treat 5xx just like 404s.

[pkotwicz, 2017/05/12 15:37:41]

Fair enough. You will need to delete the tests which are testing for this
behavior. For instance:
FaviconHandlerManifestsEnabledTest.IgnoreManifestWithPrior404

[mastiz, 2017/05/15 14:06:59]

404 behavior hasn't changed, only 5xx. That is, any failed manifest download
will be blacklisted.

[pkotwicz, 2017/05/16 06:36:34]

Ok, I see now

+    service_->UnableToDownloadFavicon(*manifest_url_);

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

If we end up download the Web Manifest on each page load, we can always download
the Web Manifest here. (Because it will likely  already have been cached)

I think that Dominick has landed an experiment which fetches the Web Manifest on
page load:
https://chromium.googlesource.com/chromium/src/+/c2d4ff6157a4d06bd06089b872e0....

It is possible that we don't need to cache Web Manifests the way we cache
favicons. A default favicon name is assumed if the page does not specify one. So
we are likely to get a 404 when we download a favicon. However, a default Web
Manifest name is not assumed if the page does not specify one.

I don't think that I am qualified to determine whether "We should download Web
Manifests on page load". The Android "Emerging Markets" team can likely answer
this question.

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

If we were to always load and parse manifests, this whole patch might need to be
reverted. Upper layers could simply feed in a merged list of favicons.
Meanwhile, I think it's responsible to implement the caching presented here.

[pkotwicz, 2017/04/24 14:42:15]

It might not be that nice because:
- Web Manifest downloads take non zero time.
- We want to avoid a flash of the default icon
- I am unsure whether downloading the Web Manifest triggers the spinner on
Desktop

I think that we will likely still need to vend the icon URLs and the manifest
URL to FaviconHandler prior to download the Web Manifest (even if we always
downloading the Web Manifest)

[mastiz, 2017/04/27 13:54:50]

Acknowledged, makes sense. We'd need to see how that compares to icon download
itself, and if it's negligible, a simpler solution would be desirable.

+
+  manifest_url_.reset();
+  OnGotFinalIconURLCandidates(non_manifest_original_candidates_);
+}
+
+void FaviconHandler::OnGotFinalIconURLCandidates(
+    const std::vector<FaviconURL>& candidates) {
   std::vector<FaviconCandidate> sorted_candidates;
   const std::vector<int> desired_pixel_sizes =
       GetDesiredPixelSizes(handler_type_);
@@ -309,7 +404,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 +450,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 +514,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 +555,45 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
 }
 
 void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
-  GURL icon_url = current_candidate()->icon_url;
-  favicon_base::IconType icon_type = current_candidate()->icon_type;
-
-  if (redownload_icons_) {
+  const GURL icon_url = current_candidate()->icon_url;
+  const favicon_base::IconType icon_type = current_candidate()->icon_type;
+  // If the icons listed in a manifest are being processed, skip the cache
+  // lookup for |icon_url| since the manifest's URL is used for caching anyway,
+  // and this lookup has happened earlier.
+  if (redownload_icons_ || manifest_url_) {
     // 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 +618,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);
 }