Add support to process favicons from Web Manifests

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>
@@ skipping 177 matching lines @@
 }
 
 void FaviconHandler::FetchFavicon(const GURL& url) {
   cancelable_task_tracker_.TryCancelAll();
 
   url_ = 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;
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well WebContents) when the favicon url is
   // available.
   if (service_) {
@@ skipping 28 matching lines @@
                best_favicon_.candidate.score;
   } else {
     return best_favicon_.candidate.score == 1;
   }
 }
 
 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);
 }
 
 void FaviconHandler::NotifyFaviconUpdated(
     const std::vector<favicon_base::FaviconRawBitmapResult>&
         favicon_bitmap_results) {
   if (favicon_bitmap_results.empty())
     return;
 
   gfx::Image resized_image = favicon_base::SelectFaviconFramesFromPNGs(
       favicon_bitmap_results,
       favicon_base::GetFaviconScales(),
       preferred_icon_size());
   // The history service sends back results for a single icon URL and icon
   // type, so it does not matter which result we get |icon_url| and |icon_type|
   // from.
   const GURL icon_url = favicon_bitmap_results[0].icon_url;
   favicon_base::IconType icon_type = favicon_bitmap_results[0].icon_type;
   NotifyFaviconUpdated(icon_url, icon_type, resized_image);
 }
 
 void FaviconHandler::NotifyFaviconUpdated(const GURL& icon_url,
                                           favicon_base::IconType icon_type,
                                           const gfx::Image& image) {
   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_);
   for (const FaviconURL& candidate : candidates) {
     if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_)) {
       sorted_candidates.push_back(
           FaviconCandidate::FromFaviconURL(candidate, desired_pixel_sizes));
     }
   }
 
   std::stable_sort(sorted_candidates.begin(), sorted_candidates.end(),
                    &FaviconCandidate::CompareScore);
 
   if (candidates_.size() == sorted_candidates.size() &&
       std::equal(sorted_candidates.begin(), sorted_candidates.end(),
                  candidates_.begin())) {
     return;
   }
 
-  download_request_.Cancel();
+  image_download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
   current_candidate_index_ = 0u;
   best_favicon_ = DownloadedFavicon();
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
   if (current_candidate() && got_favicon_from_history_)
     OnGotInitialHistoryDataAndIconURLCandidates();
 }
 
@@ skipping 25 matching lines @@
 }
 
 void FaviconHandler::OnDidDownloadFavicon(
     favicon_base::IconType icon_type,
     int id,
     int http_status_code,
     const GURL& image_url,
     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;
     if (service_)
       service_->UnableToDownloadFavicon(image_url);
   }
 
   bool request_next_icon = true;
   if (!bitmaps.empty()) {
     float score = 0.0f;
@@ skipping 27 matching lines @@
 
   if (request_next_icon && current_candidate_index_ + 1 < candidates_.size()) {
     // Process the next candidate.
     ++current_candidate_index_;
     DownloadCurrentCandidateOrAskFaviconService();
   } else {
     // 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);

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

If the |icon_url| is from the Web Manifest, we have to use the manifest URL as
the icon URL

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

This is implemented inside SetFavicon and NotifyFaviconUpdated. The second has
two callers so I slightly preferred this alternative, but I'm fine either way
(it's more important that tests cover all these cases, which I believe they do).

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

I think that it is clearer if you pass the correct value to SetFavicon() and
NotifyFaviconUpdated().

From looking at the code, it looks like you don't need to apply the
transformation to any of the NotifyFaviconUpdated() calls. In the places that
NotifyFaviconUpdated() is called, the "icon URL" should already be the "manifest
URL"

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

Done.

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

It looks like there is still the manifest_url_.value_or(icon_url) statement in
NotifyFaviconUpdated()

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

There's at least one codepath (via OnFaviconDataForManifestFromFaviconService)
where this is needed. What would you suggest otherwise? We can avoid this
overload of NotifyFaviconUpdated() but that'd lead to some duplicated code (call
to SelectFaviconFramesFromPNGs()).

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

When NotifyFaviconUpdated() is called as a result of
OnFaviconDataForManifestFromFaviconService() |icon_url| should be equal to
|manifest_url|

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

You're right, thanks. Done.

     }
     // Clear download related state.
     current_candidate_index_ = candidates_.size();
     best_favicon_ = DownloadedFavicon();
   }
 }
 
 const std::vector<GURL> FaviconHandler::GetIconURLs() const {
   std::vector<GURL> icon_urls;
   for (const FaviconCandidate& candidate : candidates_)
     icon_urls.push_back(candidate.icon_url);
   return icon_urls;
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
-  return !download_request_.IsCancelled() ||
+  return !manifest_download_request_.IsCancelled() ||
+         !image_download_request_.IsCancelled() ||
          cancelable_task_tracker_.HasTrackedTasks();
 }
 
 bool FaviconHandler::ShouldSaveFavicon() {
   if (!delegate_->IsOffTheRecord())
     return true;
 
   // Always save favicon if the page is bookmarked.
   return delegate_->IsBookmarked(url_);
 }
 
 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);
   initial_history_result_expired_or_incomplete_ =
       !has_valid_result ||
       HasExpiredOrIncompleteResult(preferred_icon_size(),
                                    favicon_bitmap_results);
   redownload_icons_ = initial_history_result_expired_or_incomplete_ &&
                       !favicon_bitmap_results.empty();
 
   if (has_valid_result && (!current_candidate() ||
                            DoUrlsAndIconsMatch(current_candidate()->icon_url,
                                                current_candidate()->icon_type,
                                                favicon_bitmap_results))) {

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

We should trigger this if() statement if the "database icon URL" is the Web
Manifest URL

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

This situation would cause current_candidate() == null in all cases, wouldn't
it? It's also exercised by at least one test.

     // The db knows the favicon (although it may be out of date) and the entry
     // doesn't have an icon. Set the favicon now, and if the favicon turns out
     // to be expired (or the wrong url) we'll fetch later on. This way the
     // user doesn't see a flash of the default favicon.
     NotifyFaviconUpdated(favicon_bitmap_results);
   }
 
   if (current_candidate())
     OnGotInitialHistoryDataAndIconURLCandidates();
 }
 
 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_);
+  }
+}
+
 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);
   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 (!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.
+    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);
 }
 
 }  // namespace favicon