Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883293002:
LargeIconService returns icon of the desired size (Closed)
DescriptionLargeIconService returns icon of the desired size
For now LargeIconService returns the an icon bigger than the minimal size.
If the page has multiple type of favicon, it might not be the biggest favicon,
and it might be smaller than the desired size.
This CL changes the service to return an icon bigger than the desired size and
the minimum size, or the biggest possible.
BUG=722770
Review-Url: https://codereview.chromium.org/2883293002
Cr-Commit-Position: refs/heads/master@{#478235}
Committed: https://chromium.googlesource.com/chromium/src/+/d8572898d2d9bab32c5888e87d1e779cd8d4c291
Patch Set 1 #Patch Set 2 : Add comment and change the logic of the min size #
Total comments: 7
Messages
Total messages: 23 (6 generated)
gambard@chromium.org changed reviewers: + pkotwicz@chromium.org
PTAL.
ping
Why don't you change the minimum size parameter passed to GetLargeIconOrFallbackStyle() instead of removing the minimum size argument?
On 2017/05/17 15:32:40, pkotwicz wrote: > Why don't you change the minimum size parameter passed to > GetLargeIconOrFallbackStyle() instead of removing the minimum size argument? Because I want to accept small favicon, but I still want the one as close as possible from the desired size. I am not removing the minimum size argument. I am giving the desired size as minimum size for the backend, for the backend to give me a favicon bigger than the desired size, or the biggest possible. Then if this is lower than the minimal size, the fallback style should take over.
pkotwicz@chromium.org changed reviewers: + jkrcal@chromium.org, michaelbai@chromium.org
LGTM on my part Adding extra reviews: jkrcal@ who has been working on the NTP side of things michaelbai@ who I think is familiar with the desired behavior of LargeIconBridge#getLargeIconForUrl() gambard@ can you clarify the CL description? My understanding is that this CL only affects web pages which list multiple types of icons e.g. <link rel="icon"> and <link rel="apple-touch-icon">
Should you update document of LargeIconService::GetLargeIconOrFallbackStyle ?
Description was changed from ========== LargeIconService returns icon of the desired size For now LargeIconService returns the an icon bigger than the minimal size. This CL changes it to return an icon bigger than the desired size. BUG=722770 ========== to ========== LargeIconService returns icon of the desired size For now LargeIconService returns the an icon bigger than the minimal size. If the page has multiple type of favicon, it might not be the biggest favicon, and it might be smaller than the desired size. This CL changes the service to return an icon bigger than the desired size and the minimum size, or the biggest possible. BUG=722770 ==========
Thanks, I have updated comment in code and for the CL. I have also changed the behavior to check for an icon bigger than the minimum size and the desired size in case 0 is passed as desired size. pkotwicz@: PTAL.
https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:311: large_icon_types_.push_back(favicon_base::IconType::FAVICON); Actually another fix for the issue could be using only: large_icon_types_.push_back( favicon_base::IconType::FAVICON | favicon_base::IconType::TOUCH_ICON | favicon_base::IconType::TOUCH_PRECOMPOSED_ICON); Then, we would specify no preference for icon type and thus could use minimum size below as 0. In general, this seems simpler, to mee. WDYT? https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:395: page_url, large_icon_types_, max_size_in_pixel, nit: I do not like the naming. Passing in a max_size_in_pixel variable seems misleading to me as the function does not give any guarantee about maximum size of what is returned. What about desired_size_in_pixel = std::max(desired_size_in_pixel, min_source_size_in_pixel); and using desired_size_in_pixel here? https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.h (right): https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.h:48: // Case 4. An icon exists whose size is < |min_source_size_in_pixel|: Where is Case 3?
https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:311: large_icon_types_.push_back(favicon_base::IconType::FAVICON); On 2017/05/18 12:33:55, jkrcal wrote: > Actually another fix for the issue could be using only: > large_icon_types_.push_back( > favicon_base::IconType::FAVICON > | favicon_base::IconType::TOUCH_ICON > | favicon_base::IconType::TOUCH_PRECOMPOSED_ICON); > > Then, we would specify no preference for icon type and thus could use minimum > size below as 0. > > In general, this seems simpler, to mee. > WDYT? Yes but I think we want to have a preference. I remember some people saying we should prefer FAVICON over TOUCH, if the size is big enough. https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:395: page_url, large_icon_types_, max_size_in_pixel, On 2017/05/18 12:33:55, jkrcal wrote: > nit: I do not like the naming. Passing in a max_size_in_pixel variable seems > misleading to me as the function does not give any guarantee about maximum size > of what is returned. > > What about > desired_size_in_pixel = > std::max(desired_size_in_pixel, min_source_size_in_pixel); > and using desired_size_in_pixel here? I don't like changing the value of arguments. but maybe ideal_size_in_pixel = std::max(desired_size_in_pixel, min_source_size_in_pixel); is better?
lgtm https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:311: large_icon_types_.push_back(favicon_base::IconType::FAVICON); On 2017/05/18 13:43:59, gambard wrote: > On 2017/05/18 12:33:55, jkrcal wrote: > > Actually another fix for the issue could be using only: > > large_icon_types_.push_back( > > favicon_base::IconType::FAVICON > > | favicon_base::IconType::TOUCH_ICON > > | favicon_base::IconType::TOUCH_PRECOMPOSED_ICON); > > > > Then, we would specify no preference for icon type and thus could use minimum > > size below as 0. > > > > In general, this seems simpler, to mee. > > WDYT? > > Yes but I think we want to have a preference. I remember some people saying we > should prefer FAVICON over TOUCH, if the size is big enough. Okay, let's keep it as it is now. https://codereview.chromium.org/2883293002/diff/20001/components/favicon/core... components/favicon/core/large_icon_service.cc:395: page_url, large_icon_types_, max_size_in_pixel, On 2017/05/18 13:43:59, gambard wrote: > On 2017/05/18 12:33:55, jkrcal wrote: > > nit: I do not like the naming. Passing in a max_size_in_pixel variable seems > > misleading to me as the function does not give any guarantee about maximum > size > > of what is returned. > > > > What about > > desired_size_in_pixel = > > std::max(desired_size_in_pixel, min_source_size_in_pixel); > > and using desired_size_in_pixel here? > > I don't like changing the value of arguments. but maybe > ideal_size_in_pixel = std::max(desired_size_in_pixel, min_source_size_in_pixel); > is better? I see. ideal is better to me than max. Maybe size_in_pixel_to_request fits even better? (as this is the size we request from the service).
ping. pkotwicz@: PTAL as the behavior changed michaelbai@: PTAL.
ping again. pkotwicz@: PTAL as the behavior changed michaelbai@: PTAL.
Still LGTM
michaelbai@: ping
LGTM
The CQ bit was checked by gambard@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496992913576020,
"parent_rev": "bf2bd9149f20b8ffe73e74c0c6a939cb2220c5e5", "commit_rev":
"d8572898d2d9bab32c5888e87d1e779cd8d4c291"}
Message was sent while issue was closed.
Description was changed from ========== LargeIconService returns icon of the desired size For now LargeIconService returns the an icon bigger than the minimal size. If the page has multiple type of favicon, it might not be the biggest favicon, and it might be smaller than the desired size. This CL changes the service to return an icon bigger than the desired size and the minimum size, or the biggest possible. BUG=722770 ========== to ========== LargeIconService returns icon of the desired size For now LargeIconService returns the an icon bigger than the minimal size. If the page has multiple type of favicon, it might not be the biggest favicon, and it might be smaller than the desired size. This CL changes the service to return an icon bigger than the desired size and the minimum size, or the biggest possible. BUG=722770 Review-Url: https://codereview.chromium.org/2883293002 Cr-Commit-Position: refs/heads/master@{#478235} Committed: https://chromium.googlesource.com/chromium/src/+/d8572898d2d9bab32c5888e87d1e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d8572898d2d9bab32c5888e87d1e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
