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 738fa28cd48797d373e57c5d96ccd3101ceec7b4..6f3948b4d16faf90bd5fbef232738f9f2f0f9a87 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -220,14 +220,17 @@ int FaviconHandler::GetIconTypesFromHandlerType(
 }
 
 void FaviconHandler::FetchFavicon(const GURL& url) {
-  cancelable_task_tracker_.TryCancelAll();
+  cancelable_task_tracker_for_page_url_.TryCancelAll();
+  cancelable_task_tracker_for_candidates_.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;
@@ -242,7 +245,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
       url_, icon_types_, preferred_icon_size(),
       base::Bind(&FaviconHandler::OnFaviconDataForInitialURLFromFaviconService,
                  base::Unretained(this)),
-      &cancelable_task_tracker_);
+      &cancelable_task_tracker_for_page_url_);
 }
 
 bool FaviconHandler::UpdateFaviconCandidate(
@@ -273,6 +276,8 @@ bool FaviconHandler::UpdateFaviconCandidate(
 void FaviconHandler::SetFavicon(const GURL& icon_url,
                                 const gfx::Image& image,
                                 favicon_base::IconType icon_type) {
+  LOG(INFO) << "MIKEL FaviconHandler::SetFavicon() type " << icon_type << " url "
+            << icon_url;
   if (ShouldSaveFavicon())
     service_->SetFavicons(url_, icon_url, icon_type, image);
 
@@ -282,9 +287,6 @@ void FaviconHandler::SetFavicon(const GURL& icon_url,
 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(),
@@ -303,23 +305,105 @@ 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;
 
+  // |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_->WasUnableToDownloadFavicon(*manifest_url_)) {
+    DVLOG(1) << "Skip failed Manifest: " << *manifest_url_;
+    manifest_url_.reset();
+  }
+
+  if (manifest_url_) {
+    cancelable_task_tracker_for_candidates_.TryCancelAll();

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

- Can this be moved outside of the if() statement. That way you don't need to
call it in OnGotFinalIconURLCandidates()

- Shouldn't you also cancel |manifest_download_request_| here?

- I am tempted to recommend creating an inner subclass which deals with fetching
manifest data from the database and downloading the manifest. This would be in
nice in the sense that the code here could call: 
manifest_fetcher_.reset()

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

I don't think this is desirable because OnGotFinalIconURLCandidates() will
ignore the call if the candidates didn't change. In that case, there's no need
to start from scratch.

In fact, cancelling here alone would abort the ongoing processing and not start
a new one, which is a bug. UpdateSameIconURLsWhileProcessingShouldBeNoop()
doesn't catch it, so I introduced a new dedicated test.
Let's defer this discussion to when the rest of the comments are addressed.

More broadly, I would love we could refactor the code to a throw-away class that
does the actual fetching of manifest and also icons, but I'm not 100% convinced
the result would be much simpler.

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

Isn't this because |cancelable_task_tracker_for_candidates_| is used for both:
- getting the icon for the Web Manifest URL from history
- getting the icon for a candidate URL from history
I think that these two tasks should use different CancellableTaskTrackers to
make things simpler

OnUpdateCandidates() should cancel any in progress tasks to compute the
candidates passed to OnGotFinalIconURLCandidates()

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

I'm not really sure why splitting would be any simpler. Due to the code in
GetFaviconAndUpdateMappingsUnlessIncognito() requiring the tracker, what you
propose will lead to duplicate code (for incognito-handling, which is the kind
of subtle code you'd rather not duplicate) or code complexity (inject tracker as
parameter).

Higher-level, IIUC, you're proposing that we don't cancel image lookups while we
trigger the manifest's lookup. This adds yet another possible state in our state
machine to think about: should the lookup completion trigger
NotifyFaviconUpdated()? Should it process further candidates?

I think it's simpler to make explicit that up to one DB lookup is ongoing at all
times for candidate icons and manifest. This seems more natural with a single
tracker.

WDYT?

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

I think that splitting the trackers would fix "Bug #2" that I have outlined in
my other comment.

Passing a tracker to GetFaviconAndUpdateMappingsUnlessIncognito() is reasonable.

The way I think about things is that state is reset in
FaviconHandler::OnGotFinalIconURLCandidates() and the processing between
FaviconHandler::OnUpdateCandidates() and
FaviconHandler::OnGotFinalIconURLCandidates() is self contained (hence my
earlier proposal for moving this processing to a dedicated class) I am not
suggesting that you move the processing to a dedicated class in this CL

I am fine if you find a way of addressing the bug while cancelling both the
"manifest URL" and the "icon URL" database lookups in OnUpdateCandidates()

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

I might have achieved this, PTAL.

I've traded simplicity for a small behavioral change (the only change not
guarded by the feature flag): the no-op handling for candidate equality is now
done based on the original candidate favicons (non-filtered, non-sorted). Let me
know if this is something you wouldn't like to change.

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

This change is reasonable. Its a neat solution to the bug

+    // 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)));
+  } else {
+    // If no manifest available, proceed with the regular candidates only.
+    OnGotFinalIconURLCandidates(candidates);
+  }
+}
+
+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) {
+    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()) {
+    OnGotFinalIconURLCandidates(candidates);
+    return;
+  }
+
+  // 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 (status_code == 404 || status_code == 200)
+    service_->UnableToDownloadFavicon(*manifest_url_);
+
+  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_);
@@ -339,7 +423,8 @@ void FaviconHandler::OnUpdateFaviconURL(
     return;
   }
 
-  download_request_.Cancel();
+  cancelable_task_tracker_for_candidates_.TryCancelAll();
+  image_download_request_.Cancel();
   candidates_ = std::move(sorted_candidates);
   num_download_requests_ = 0;
   current_candidate_index_ = 0u;
@@ -386,7 +471,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;
@@ -436,9 +521,10 @@ void FaviconHandler::OnDidDownloadFavicon(
     RecordDownloadAttemptsForHandlerType(handler_type_, num_download_requests_);
     // 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);
+      // No more icons to request, set the favicon from the candidate. Note that
+      // manifest URLs override icon URLs if available.
+      SetFavicon(manifest_url_.value_or(best_favicon_.candidate.icon_url),
+                 best_favicon_.image, best_favicon_.candidate.icon_type);
     }
     // Clear download related state.
     current_candidate_index_ = candidates_.size();
@@ -455,8 +541,10 @@ const std::vector<GURL> FaviconHandler::GetIconURLs() const {
 }
 
 bool FaviconHandler::HasPendingTasksForTest() {
-  return !download_request_.IsCancelled() ||
-         cancelable_task_tracker_.HasTrackedTasks();
+  return !manifest_download_request_.IsCancelled() ||
+         !image_download_request_.IsCancelled() ||
+         cancelable_task_tracker_for_page_url_.HasTrackedTasks() ||
+         cancelable_task_tracker_for_candidates_.HasTrackedTasks();
 }
 
 bool FaviconHandler::ShouldSaveFavicon() {
@@ -479,15 +567,20 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
   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))) {
-    // 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 (has_valid_result) {
+    if (!current_candidate() ||
+        DoUrlsAndIconsMatch(current_candidate()->icon_url,
+                            current_candidate()->icon_type,
+                            favicon_bitmap_results) ||
+        (manifest_url_ &&
+         DoUrlsAndIconsMatch(*manifest_url_, favicon_base::FAVICON,
+                             favicon_bitmap_results))) {
+      // 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())
@@ -495,34 +588,42 @@ 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);
+    ScheduleFaviconDownload(icon_url, icon_type);
   } else {
-    // 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_);
-    }
+    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) {
+  // 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_for_candidates_);
+  } 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_for_candidates_);
   }
 }
 
@@ -547,22 +648,24 @@ 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.
-    return;
+    // TODO(mastiz) / DONOTSUBMIT: Remove all this 'if' block.
+    NOTREACHED();

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

Can you move the removal of this if() statement and the introduction of the
second CancelableTaskTracker into a separate CL (which will likely land before
this CL)?

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

Yes, will do, together with a few changes in tests. Let's settle on the few open
high-level points and I'll start splitting the changes.

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

Done.

   }
 
   if (has_expired_or_incomplete_result) {
-    ScheduleDownload(current_candidate()->icon_url,
-                     current_candidate()->icon_type);
+    ScheduleFaviconDownload(current_candidate()->icon_url,
+                            current_candidate()->icon_type);
   } else if (num_download_requests_ > 0) {
     RecordDownloadAttemptsForHandlerType(handler_type_, num_download_requests_);
   }
 }
 
-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_->WasUnableToDownloadFavicon(image_url)) {
     DVLOG(1) << "Skip Failed FavIcon: " << image_url;
     RecordDownloadOutcome(DownloadOutcome::SKIPPED);
@@ -571,14 +674,15 @@ void FaviconHandler::ScheduleDownload(const GURL& image_url,
     return;
   }
   ++num_download_requests_;
-  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);
 }