Issue metadata
Sign in to add a comment
|
Webpage favicons are not being shown in most visited tiles |
||||||||||||||||||||
Issue descriptionApp Version: 63.0.3239.64 Beta iOS Version: 11.2 beta 4, 11.1.2, 10.3.3 Device: iPhone and iPad URL: bbc.com or any other web page having favicons. Steps to reproduce: 1. Fresh install and launch chrome 2. Load bbc.com web page 3. Once the page loads completely, wait for few seconds or a minute 4. Open a new tab page( if bbc.com tile is not displayed open another new tab page) Observed results: Notice that bbc.com tile is displayed without favicon Note 1: Go back to step 2 tab page and reload the page, now open another new tab page notice the favicon is now displayed. Note 2: Tested on multiple webpages like cnn.com, ebay.com observe the same behavior Expected results: Webpage favicons should be displayed in most visited tiles, when the webpage is loaded for the first time Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes/No Bug reproducible after clearing cache and cookies: Yes/No Bug reproducible on Chrome Mobile on Android: Not tested Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): No on M62 Bug reproducible on the current beta channel build (App Version, iOS Version): Yes on M63 Last known good build: 63.0.3231.0 Canary, bad build: 63.0.3232.0 Canary Link to video/image: M63 Beta behavior: https://drive.google.com/a/google.com/file/d/1MDlg9qyt7H7_fc2dvbkVdNcIAQnt3Wd8/view?usp=sharing M62 Stable behavior: https://drive.google.com/a/google.com/file/d/1b4MNFXbr9O9SoYBuXPIvQ5YX-NkujVWt/view?usp=sharing
,
Nov 28 2017
I think this is related to Web more than NTP. If you take a look at the M63 Beta video, at 0:25 in the stack view, there is no icon for the tab. I think the favicon for this tab is not loaded. Assigning to eugenebut@, please reassign if you think this is a mistake.
,
Nov 28 2017
WebStateObserver::FaviconUrlUpdated returns one favicon, which looks like a valid image to me: http://static.bbci.co.uk/wwhp/1.118.0/responsive/img/apple-touch/apple-touch-180.jpg Gutier, could you please take a look if the bug happens in FaviconDriver or NTP code. Ramesh, could you please bisect this bug and update with good and bad git versions. Marking as RBS, because this is a regression. Please remove if this should not be RBS.
,
Nov 28 2017
CC-ing myself since some of my recent CLs could have caused this issue, but unlikely for M63.
,
Nov 28 2017
,
Nov 28 2017
,
Nov 28 2017
Good Version: 63.0.3231.0 #064785b2 Bad Version 63.0.3232.0 #6de9aff
,
Nov 28 2017
The good/bad versions mentioned in comment 7 are canary versions and landed between October 2 and 3. Full list of changes here: https://paste.googleplex.com/5336155054669824
,
Nov 28 2017
This commit looks particularly suspicious: 8db59b970550 Tue Oct 3 01:21:58 2017 Sylvain Defresne Fix favicon in case of same document navigation.
,
Nov 28 2017
It's is not possible/easy to revert 8db59b970550 on the trunk as several other changes landed on top of it. Directly reverting 8db59b970550 on M63 branch has 1 conflict which can be resolved manually. With that, I can demonstrate that favicon for BBC.com is loaded on first try. However, reverting this CL will likely reintroduce issue 769049 .
,
Nov 29 2017
We know the two CLs that introduced this problem just before M63 branched: http://crrev/c/695761 http://crrev/c/707003 There's now a pending CL http://crrev/c/795033 on branch M63 that reverts both CLs and should fix this problem, but re-introduces the other problem ( issue 769049 ). It is preferable in our opinion that issue 769049 is less prominent than this issue which affects all users (it's on NTP). Further, that issue 769049 is already in M62 App Store release.
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/01cb1cc326f478a6d8f37d1ed60f5c88d1de85d9 commit 01cb1cc326f478a6d8f37d1ed60f5c88d1de85d9 Author: Peter K. Lee <pkl@chromium.org> Date: Wed Nov 29 00:08:25 2017 Reverts 2 CLs to fix missing favicons issue. The following 2 CLs are reverted on M63 branch in this CL. https://chromium-review.googlesource.com/c/chromium/src/+/707003 https://chromium-review.googlesource.com/c/chromium/src/+/695761 TBR=sdefresne@chromium.org Bug: 788708 Change-Id: Ia95d76cd3882f494ad3d34a74ab136189847d2a0 Reviewed-on: https://chromium-review.googlesource.com/795033 Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#595} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/01cb1cc326f478a6d8f37d1ed60f5c88d1de85d9/components/favicon/ios/web_favicon_driver.h [modify] https://crrev.com/01cb1cc326f478a6d8f37d1ed60f5c88d1de85d9/components/favicon/ios/web_favicon_driver.mm
,
Nov 29 2017
,
Nov 29 2017
,
Nov 29 2017
Is this bug still happening on M64? Is there a bug created?
,
Nov 29 2017
Verified on 64.0.3280.0 Canary tested on iPhone 7(iOS 10.3.3) The issue is reproducible on M64 Note: This issue is not tested in latest beta build Since the build is not available.
,
Nov 29 2017
This was fixed only on the branch for Beta build. Trunk fix will be landed separately. Lindsay, could you please ask QA to verify the fix with M63 beta.
,
Nov 29 2017
We don't have a new build (http://crbug/789477) on M63 to verify the fix. Will verify once we get the new build. I just created a new tracker http://crbug/789581 for fix on M64. Can you update that bug and close this one as fixed.
,
Nov 29 2017
Thanks!
,
Nov 29 2017
Verified in M63.0.3239.72 beta iPhoneX, iPad Pro iOS11.2, 10.3.3 Favicons are displayed correctly in NTP |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by pkl@chromium.org
, Nov 27 2017Labels: -Pri-2 M-64 Pri-1
Owner: gambard@chromium.org
Status: Assigned (was: Untriaged)