Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(249)

Unified Diff: components/favicon/core/favicon_handler.cc

Issue 2799273002: Add support to process favicons from Web Manifests (Closed)
Patch Set: Browsertest comments addressed. Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « components/favicon/core/favicon_handler.h ('k') | components/favicon/core/favicon_handler_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index f811c350376f517a6888ab7222b2e98e9929e5cf..8f8622d7e79db019d6987d195f6867b0e84605b8 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -11,6 +11,7 @@
#include "base/bind.h"
#include "base/bind_helpers.h"
+#include "base/feature_list.h"
#include "base/memory/ref_counted_memory.h"
#include "base/metrics/histogram_macros.h"
#include "build/build_config.h"
@@ -23,6 +24,10 @@
#include "ui/gfx/image/image_util.h"
namespace favicon {
+
+const base::Feature kFaviconsFromWebManifest{"FaviconsFromWebManifest",
+ base::FEATURE_DISABLED_BY_DEFAULT};
+
namespace {
const int kNonTouchLargestIconSize = 192;
@@ -141,6 +146,11 @@ std::vector<int> GetDesiredPixelSizes(
return std::vector<int>();
}
+bool FaviconURLEquals(const FaviconURL& lhs, const FaviconURL& rhs) {
+ return lhs.icon_url == rhs.icon_url && lhs.icon_type == rhs.icon_type &&
+ lhs.icon_sizes == rhs.icon_sizes;
+}
+
} // namespace
////////////////////////////////////////////////////////////////////////////////
@@ -209,7 +219,10 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
initial_history_result_expired_or_incomplete_ = false;
redownload_icons_ = false;
got_favicon_from_history_ = false;
+ manifest_download_request_.Cancel();
image_download_request_.Cancel();
+ manifest_url_ = GURL();
+ non_manifest_original_candidates_.clear();
candidates_.clear();
notification_icon_url_ = GURL();
notification_icon_type_ = favicon_base::INVALID_ICON;
@@ -297,10 +310,111 @@ void FaviconHandler::NotifyFaviconUpdated(const GURL& icon_url,
void FaviconHandler::OnUpdateCandidates(
const GURL& page_url,
- const std::vector<FaviconURL>& candidates) {
+ const std::vector<FaviconURL>& candidates,
+ const GURL& manifest_url) {
if (page_url != url_)
return;
+ bool manifests_feature_enabled =
+ base::FeatureList::IsEnabled(kFaviconsFromWebManifest);
+
+ // |candidates| or |manifest_url| could have been modified via Javascript. If
+ // neither changed, ignore the call.
+ if ((!manifests_feature_enabled || manifest_url_ == manifest_url) &&
+ non_manifest_original_candidates_.size() == candidates.size() &&
+ std::equal(candidates.begin(), candidates.end(),
+ non_manifest_original_candidates_.begin(),
+ &FaviconURLEquals)) {
+ return;
+ }
+
+ non_manifest_original_candidates_ = candidates;
+ cancelable_task_tracker_for_candidates_.TryCancelAll();
+ manifest_download_request_.Cancel();
+ image_download_request_.Cancel();
+ num_image_download_requests_ = 0;
+ current_candidate_index_ = 0u;
+ best_favicon_ = DownloadedFavicon();
+
+ if (manifests_feature_enabled)
+ manifest_url_ = manifest_url;
+
+ // Check if the manifest was previously blacklisted (e.g. returned a 404) and
+ // ignore the manifest URL if that's the case.
+ if (!manifest_url_.is_empty() &&
+ service_->WasUnableToDownloadFavicon(manifest_url_)) {
+ DVLOG(1) << "Skip failed Manifest: " << manifest_url;
+ manifest_url_ = GURL();
+ }
+
+ // If no manifest available, proceed with the regular candidates only.
+ if (manifest_url_.is_empty()) {
+ OnGotFinalIconURLCandidates(candidates);
+ return;
+ }
+
+ // See if there is a cached favicon for the manifest. This will update the DB
+ // mappings only if the manifest URL is cached.
+ 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) {
+ // The database lookup for the page URL is guaranteed to be completed because
+ // the HistoryBackend uses a SequencedTaskRunner, and we also know that
+ // FetchFavicon() was called before OnUpdateCandidates().
+ DCHECK(got_favicon_from_history_);
+
+ 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 && (notification_icon_url_ != manifest_url_ ||
+ notification_icon_type_ != favicon_base::FAVICON)) {
+ // 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(
+ 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();
+
+ service_->UnableToDownloadFavicon(manifest_url_);
+ manifest_url_ = GURL();
+
+ 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_);
@@ -314,18 +428,7 @@ void FaviconHandler::OnUpdateCandidates(
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;
- }
-
- cancelable_task_tracker_for_candidates_.TryCancelAll();
- image_download_request_.Cancel();
candidates_ = std::move(sorted_candidates);
- num_image_download_requests_ = 0;
- 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.
@@ -419,9 +522,11 @@ void FaviconHandler::OnDidDownloadFavicon(
num_image_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. The
+ // manifest URL, if available, is used instead of the icon URL.
+ SetFavicon(manifest_url_.is_empty() ? best_favicon_.candidate.icon_url
+ : manifest_url_,
+ best_favicon_.image, best_favicon_.candidate.icon_type);
}
// Clear download related state.
current_candidate_index_ = candidates_.size();
@@ -439,6 +544,7 @@ const std::vector<GURL> FaviconHandler::GetIconURLs() const {
bool FaviconHandler::HasPendingTasksForTest() {
return !image_download_request_.IsCancelled() ||
+ !manifest_download_request_.IsCancelled() ||
cancelable_task_tracker_for_page_url_.HasTrackedTasks() ||
cancelable_task_tracker_for_candidates_.HasTrackedTasks();
}
@@ -476,32 +582,40 @@ 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, not the
+ // icon URL, and this lookup has happened earlier.
+ if (redownload_icons_ || !manifest_url_.is_empty()) {
// We have the mapping, but the favicon is out of date. Download it now.
ScheduleImageDownload(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_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.
- service_->UpdateFaviconMappingsAndFetch(
- url_, icon_url, icon_type, preferred_icon_size(),
- base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
- &cancelable_task_tracker_for_candidates_);
- }
+ 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.
+ service_->UpdateFaviconMappingsAndFetch(
+ url_, icon_url, icon_type, preferred_icon_size(), callback,
+ &cancelable_task_tracker_for_candidates_);
}
}
« no previous file with comments | « components/favicon/core/favicon_handler.h ('k') | components/favicon/core/favicon_handler_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698