History/Bookmark/ReadingList/RecentTabs are passing favicon-size parameters to FaviconLoader in the wrong order. |
||||||||
Issue descriptionChrome Version: 73.0.3645.0 OS: iOS What steps will reproduce the problem? (1)Visit github.com or cs.chromium.org; (2)Open History panel. The same thing happens in Bookmark/ReadingList/RecentTabs. What is the expected result? The favicon should be shown. What happens instead? Only fallback favicon is shown.
,
Dec 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a4b960dbe46c2ee541af6e9811fdea78c63563d0 commit a4b960dbe46c2ee541af6e9811fdea78c63563d0 Author: Yi Su <mrsuyi@chromium.org> Date: Tue Dec 18 17:25:50 2018 Fix reversed parameters for FaviconForUrl. History/Bookmark/ReadingList/RecentTabs are getting favicons by calling FaviconLoader::FaviconForUrl with reversed parameter "size" and "min_size". Bug: 916135 Change-Id: I79b4a3e2677d49b15ddcd9e44aca7e2bc2d93e42 Reviewed-on: https://chromium-review.googlesource.com/c/1382498 Reviewed-by: Mark Cogan <marq@chromium.org> Commit-Queue: Yi Su <mrsuyi@chromium.org> Cr-Commit-Position: refs/heads/master@{#617545} [modify] https://crrev.com/a4b960dbe46c2ee541af6e9811fdea78c63563d0/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm [modify] https://crrev.com/a4b960dbe46c2ee541af6e9811fdea78c63563d0/ios/chrome/browser/ui/history/history_mediator.mm [modify] https://crrev.com/a4b960dbe46c2ee541af6e9811fdea78c63563d0/ios/chrome/browser/ui/reading_list/reading_list_mediator.mm [modify] https://crrev.com/a4b960dbe46c2ee541af6e9811fdea78c63563d0/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm
,
Dec 18
,
Dec 18
,
Dec 18
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 3
Canary verification please.
,
Jan 8
Tested in Build: 73.0.3665.0 Canary Device details: iPhone X(iOS 11.4.1), iPhone XS Max(iOS 12.1.2) Issue is still reproducible on following the steps mentioned in comment#0. Link to video: https://drive.google.com/file/d/1cN_5wcgfpy0hVC5JnDVkPSSkFi7VQsiK/view?usp=sharing
,
Jan 8
Please re-request merge when ready.
,
Jan 8
I think currently our favicon services are very buggy. This CL only works for cases that are caused by passing improper icon-size parameters, and I don't have any idea how much it can improve. At this time maybe we should not merge it in M72 since it has been buggy for quite a long time :D I would prefer to narrow down this bug to a smaller scope and close it. Fixing favicons for all websites would be a big project and requires much longer time to finish.
,
Jan 8
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by mrsuyi@chromium.org
, Dec 18