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 complet
mastiz
2017/04/20 18:06:33
This happens in upper layers: the manifest URL is
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
|
+ 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 al
pkotwicz
2017/04/24 14:42:15
Perhaps FaviconHandler::OnDidDownloadManifest() sh
mastiz
2017/04/27 13:54:50
How would that distinguish a 404 from 503?
pkotwicz
2017/05/01 04:56:53
I suggest doing this:
if (status_code == 404 || (
mastiz
2017/05/04 10:57:53
Would that work for a manifest that returns a 200
pkotwicz
2017/05/04 17:28:24
Black listing anything other than 5xx sounds reaso
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 n
mastiz
2017/05/12 13:31:32
This was done previously but I've reverted it (in
pkotwicz
2017/05/12 15:37:41
Fair enough. You will need to delete the tests whi
mastiz
2017/05/15 14:06:59
404 behavior hasn't changed, only 5xx. That is, an
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 pag
mastiz
2017/04/20 18:06:33
If we were to always load and parse manifests, thi
pkotwicz
2017/04/24 14:42:15
It might not be that nice because:
- Web Manifest
mastiz
2017/04/27 13:54:50
Acknowledged, makes sense. We'd need to see how th
|
+ |
+ 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); |
} |