New issue
Advanced search Search tips

Issue 712466 link

Starred by 1 user
Status: Fixed
Owner:
pkotw...@chromium.org
Closed: May 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 698671



Sign in to add a comment

Make FaviconService::GetRawFaviconForPageURL() have a more intuitive behavior when multiple icon types are passed in

Project Member Reported by pkotw...@chromium.org, Apr 18 2017 
This came up in the code review for
LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache()

If FaviconService::GetRawFaviconForPageURL() is called with:
desired_size_in_pixel = 0
icon_types = FAVICON | TOUCH_ICON | TOUCH_PRECOMPOSED_ICON

The page_url is mapped to both a TOUCH_ICON and a FAVICON
The bitmap of type FAVICON is larger than the bitmap of type TOUCH_ICON

The FaviconService::GetRawFaviconForPageURL() call will return the TOUCH_ICON even though it is the smaller of the two icons.

This is counterintuitive and unfortunate. It would be nice if FaviconService::GetRawFaviconForPageURL() returned the largest icon when a parameter of |desired_size_in_pixel| = 0 is passed

Comment 1 by mastiz@chromium.org, Apr 18 2017

Blocking: 698671
Project Member

Comment 2 by bugdroid1@chromium.org, May 3 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cdb043cb1911124da7f20aa869aa8423c7a6714

commit 2cdb043cb1911124da7f20aa869aa8423c7a6714
Author: pkotwicz <pkotwicz@chromium.org>
Date: Wed May 03 02:24:40 2017

[Refactor] Simplify HistoryBackend::UpdateFaviconMappingsAndFetchImpl() signature

This CL makes HistoryBackend::UpdateFaviconMappingsAndFetchImpl() take one icon
type and one icon URL. All of its callers only pass in a single type and a
single icon URL

This CL makes HistoryService::GetFavicon() take a one icon type and one icon
URL as well.

BUG= 712466 

Review-Url: https://codereview.chromium.org/2828173002
Cr-Commit-Position: refs/heads/master@{#468874}

[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/favicon/core/favicon_handler.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/favicon/core/favicon_handler_unittest.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/favicon/core/favicon_service.h
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/favicon/core/favicon_service_impl.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/favicon/core/favicon_service_impl.h
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/favicon/core/test/mock_favicon_service.h
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/history_backend.h
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/history_backend_unittest.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/history_service.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/history_service.h
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/thumbnail_database.cc
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/thumbnail_database.h
[modify] https://crrev.com/2cdb043cb1911124da7f20aa869aa8423c7a6714/components/history/core/browser/thumbnail_database_unittest.cc
Project Member

Comment 3 by bugdroid1@chromium.org, May 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1fb2030a6e961c668f3636d6fb65ca4effee0341

commit 1fb2030a6e961c668f3636d6fb65ca4effee0341
Author: pkotwicz <pkotwicz@chromium.org>
Date: Mon May 15 05:29:42 2017

Make FaviconService::GetRawFaviconForPageURL() select the best candidate among all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *)
to return the icon url which is a better match.

This CL changes the behavior FaviconHelper#getLocalFaviconImageForURL()

BUG= 712466 

Review-Url: https://codereview.chromium.org/2823093002
Cr-Commit-Position: refs/heads/master@{#471674}

[modify] https://crrev.com/1fb2030a6e961c668f3636d6fb65ca4effee0341/components/favicon/core/favicon_service.h
[modify] https://crrev.com/1fb2030a6e961c668f3636d6fb65ca4effee0341/components/history/core/browser/history_backend_unittest.cc
[modify] https://crrev.com/1fb2030a6e961c668f3636d6fb65ca4effee0341/components/history/core/browser/history_service.h
[modify] https://crrev.com/1fb2030a6e961c668f3636d6fb65ca4effee0341/components/history/core/browser/thumbnail_database.cc
[modify] https://crrev.com/1fb2030a6e961c668f3636d6fb65ca4effee0341/components/history/core/browser/thumbnail_database.h
[modify] https://crrev.com/1fb2030a6e961c668f3636d6fb65ca4effee0341/components/history/core/browser/thumbnail_database_unittest.cc

Comment 4 by pkotw...@chromium.org, May 15 2017

Status: Fixed (was: Started)
►

Sign in to add a comment