Skip to content

Commit

Permalink
Simplify FaviconHandler
Browse files Browse the repository at this point in the history 
This CL:
- Removes the |is_active_favicon| parameter from NotifyFaviconAvailable() since
  its value can be computed from member variables
- Refactors FaviconHandler::OnDidDownloadFavicon() to exit early if the URL for
  which the favicon was downloaded is unexpected
- Changes FaviconHandler::FetchFavicon() to reset as much state as possible.
- Changes the signature of FaviconHandler::ScheduleDownload() to void because
  its return value is never used
- Removes unused member variable
  FaviconHandler::waiting_for_favicon_service_data_

BUG=None
TEST=None

Review URL: https://codereview.chromium.org/1084473002

Cr-Commit-Position: refs/heads/master@{#326042}
  • Loading branch information
@pkotwicz
pkotwicz authored and Commit bot committed Apr 21, 2015
1 parent dcf344f commit e002b6e
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 57 deletions.
94 changes: 47 additions & 47 deletions components/favicon/core/favicon_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,10 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
url_ = url;

favicon_expired_or_incomplete_ = got_favicon_from_history_ = false;
download_requests_.clear();
image_urls_.clear();
history_results_.clear();
best_favicon_candidate_ = FaviconCandidate();

// 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
Expand Down Expand Up @@ -298,19 +301,12 @@ void FaviconHandler::SetFavicon(const GURL& url,
if (ShouldSaveFavicon(url))
SetHistoryFavicons(url, icon_url, icon_type, image);

if (!UrlMatches(url, url_) || PageChangedSinceFaviconWasRequested())
return;

NotifyFaviconAvailable(
icon_url,
image,
icon_type == favicon_base::FAVICON && !download_largest_icon_);
NotifyFaviconAvailable(icon_url, image);
}

void FaviconHandler::NotifyFaviconAvailable(
const std::vector<favicon_base::FaviconRawBitmapResult>&
favicon_bitmap_results,
bool is_active_favicon) {
favicon_bitmap_results) {
gfx::Image resized_image = favicon_base::SelectFaviconFramesFromPNGs(
favicon_bitmap_results,
favicon_base::GetFaviconScales(),
Expand All @@ -319,23 +315,27 @@ void FaviconHandler::NotifyFaviconAvailable(
// not matter which result we get the |icon_url| from.
const GURL icon_url = favicon_bitmap_results.empty() ?
GURL() : favicon_bitmap_results[0].icon_url;
NotifyFaviconAvailable(icon_url, resized_image, is_active_favicon);
NotifyFaviconAvailable(icon_url, resized_image);
}

void FaviconHandler::NotifyFaviconAvailable(const GURL& icon_url,
const gfx::Image& image,
bool is_active_favicon) {
const gfx::Image& image) {
gfx::Image image_with_adjusted_colorspace = image;
favicon_base::SetFaviconColorSpace(&image_with_adjusted_colorspace);

bool is_active_favicon =
(handler_type_ == FAVICON && !download_largest_icon_);

driver_->OnFaviconAvailable(
image_with_adjusted_colorspace, icon_url, is_active_favicon);
}

void FaviconHandler::OnUpdateFaviconURL(
const std::vector<FaviconURL>& candidates) {
download_requests_.clear();
image_urls_.clear();
best_favicon_candidate_ = FaviconCandidate();

for (const FaviconURL& candidate : candidates) {
if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_))
image_urls_.push_back(candidate);
Expand Down Expand Up @@ -393,14 +393,19 @@ void FaviconHandler::OnDidDownloadFavicon(
DownloadRequest download_request = i->second;
download_requests_.erase(i);

if (current_candidate() &&
DoUrlAndIconMatch(*current_candidate(),
image_url,
download_request.icon_type)) {
bool request_next_icon = true;
if (PageChangedSinceFaviconWasRequested() ||
!current_candidate() ||
!DoUrlAndIconMatch(*current_candidate(),
image_url,
download_request.icon_type)) {
return;
}

bool request_next_icon = true;
if (!bitmaps.empty()) {
float score = 0.0f;
gfx::ImageSkia image_skia;
if (download_largest_icon_ && !bitmaps.empty()) {
if (download_largest_icon_) {
int index = -1;
// Use the largest bitmap if FaviconURL doesn't have sizes attribute.
if (current_candidate()->icon_sizes.empty()) {
Expand All @@ -424,30 +429,27 @@ void FaviconHandler::OnDidDownloadFavicon(
gfx::Image image(image_skia);
// The downloaded icon is still valid when there is no FaviconURL update
// during the downloading.
if (!bitmaps.empty()) {
request_next_icon = !UpdateFaviconCandidate(
download_request.url, image_url, image, score,
download_request.icon_type);
}
}
if (request_next_icon && !PageChangedSinceFaviconWasRequested() &&
image_urls_.size() > 1) {
// Remove the first member of image_urls_ and process the remaining.
image_urls_.erase(image_urls_.begin());
ProcessCurrentUrl();
} else 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_.url,
best_favicon_candidate_.image_url,
best_favicon_candidate_.image,
best_favicon_candidate_.icon_type);
// Reset candidate.
image_urls_.clear();
download_requests_.clear();
best_favicon_candidate_ = FaviconCandidate();
request_next_icon = !UpdateFaviconCandidate(
download_request.url, image_url, image, score,
download_request.icon_type);
}
}

if (request_next_icon && image_urls_.size() > 1) {
// Remove the first member of image_urls_ and process the remaining.
image_urls_.erase(image_urls_.begin());
ProcessCurrentUrl();
} else 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_.url,
best_favicon_candidate_.image_url,
best_favicon_candidate_.image,
best_favicon_candidate_.icon_type);
// Reset candidate.
image_urls_.clear();
download_requests_.clear();
best_favicon_candidate_ = FaviconCandidate();
}
}

bool FaviconHandler::HasPendingTasksForTest() {
Expand Down Expand Up @@ -569,7 +571,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
// 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.
NotifyFaviconAvailable(favicon_bitmap_results, true);
NotifyFaviconAvailable(favicon_bitmap_results);
} else {
// If |favicon_bitmap_results| does not have any valid results, treat the
// favicon as if it's expired.
Expand Down Expand Up @@ -599,7 +601,7 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
// renderer to download the icon.

if (has_valid_result && (handler_type_ != FAVICON || download_largest_icon_))
NotifyFaviconAvailable(favicon_bitmap_results, false);
NotifyFaviconAvailable(favicon_bitmap_results);
}

void FaviconHandler::DownloadFaviconOrAskFaviconService(
Expand Down Expand Up @@ -647,7 +649,7 @@ void FaviconHandler::OnFaviconData(const std::vector<
// There is a favicon, set it now. If expired we'll download the current
// one again, but at least the user will get some icon instead of the
// default and most likely the current one is fine anyway.
NotifyFaviconAvailable(favicon_bitmap_results, true);
NotifyFaviconAvailable(favicon_bitmap_results);
}
if (has_expired_or_incomplete_result) {
// The favicon is out of date. Request the current one.
Expand All @@ -668,11 +670,11 @@ void FaviconHandler::OnFaviconData(const std::vector<

if (has_valid_result &&
(handler_type_ != FAVICON || download_largest_icon_)) {
NotifyFaviconAvailable(favicon_bitmap_results, false);
NotifyFaviconAvailable(favicon_bitmap_results);
}
}

int FaviconHandler::ScheduleDownload(const GURL& url,
void FaviconHandler::ScheduleDownload(const GURL& url,
const GURL& image_url,
favicon_base::IconType icon_type) {
// A max bitmap size is specified to avoid receiving huge bitmaps in
Expand All @@ -686,8 +688,6 @@ int FaviconHandler::ScheduleDownload(const GURL& url,
download_requests_[download_id] =
DownloadRequest(url, image_url, icon_type);
}

return download_id;
}

void FaviconHandler::SortAndPruneImageUrls() {
Expand Down
15 changes: 5 additions & 10 deletions components/favicon/core/favicon_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,9 @@ class FaviconHandler {

// Schedules a download for the specified entry. This adds the request to
// download_requests_.
int ScheduleDownload(const GURL& url,
const GURL& image_url,
favicon_base::IconType icon_type);
void ScheduleDownload(const GURL& url,
const GURL& image_url,
favicon_base::IconType icon_type);

// Updates |favicon_candidate_| and returns true if it is an exact match.
bool UpdateFaviconCandidate(const GURL& url,
Expand All @@ -233,11 +233,9 @@ class FaviconHandler {
// FaviconDriver::NotifyFaviconAvailable() for |is_active_favicon| in detail.
void NotifyFaviconAvailable(
const std::vector<favicon_base::FaviconRawBitmapResult>&
favicon_bitmap_results,
bool is_active_favicon);
favicon_bitmap_results);
void NotifyFaviconAvailable(const GURL& icon_url,
const gfx::Image& image,
bool is_active_favicon);
const gfx::Image& image);

// Return the current candidate if any.
favicon::FaviconURL* current_candidate() {
Expand Down Expand Up @@ -266,9 +264,6 @@ class FaviconHandler {
// URL of the page we're requesting the favicon for.
GURL url_;

// Whether we are waiting for data from the FaviconService.
bool waiting_for_favicon_service_data_;

// Whether we got data back for the initial request to the FaviconService.
bool got_favicon_from_history_;

Expand Down

0 comments on commit e002b6e

Please sign in to comment.