New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 803773 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , iOS
Pri: 2
Type: Bug



Sign in to add a comment

Popular sites are not displayed in new tab page

Project Member Reported by rakurati@chromium.org, Jan 19 2018

Issue description

App Version: 64.0.3282.87 Beta
iOS Version: 10.3.2, 11.2.2, 11.2.5 beta 5
Device: iPhone and iPad

Steps to reproduce:
1. Fresh install chrome 
2. Turn on Airplane mode and launch chrome
3. Wait until baked in sites are displayed in tab page
4. Turn off the device Airplane mode and open a new tab page
5. Force quit the app and relaunch chrome 

Observed results:
Baked in popular sites are not replaced with location based popular sites

Expected results:
Once device is connected to internet the location based popular sites should be displayed in new tab page

Number of times you were able to reproduce: 5/5
Bug reproducible after clean install: Yes
Bug reproducible after clearing cache and cookies: Yes
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): Yes on M63
Bug reproducible on the current beta channel build (App Version, iOS Version): Yes on M64

Link to video/image:
https://drive.google.com/file/d/1s9FEaomfSAJDT_EvB8CoSqVE7TqpkqVH/view?usp=sharing

 
Components: UI>Browser>ContentSuggestions
Owner: gambard@chromium.org
Status: Assigned (was: Untriaged)
Cc: gambard@chromium.org
Owner: fhorschig@chromium.org
Assigning to fhorschig@: I am not sure who is the current owner for that, please reassign if needed.
Cc: mastiz@chromium.org
Labels: M-65 zine-triaged OS-Android
Reproducible on Android Chrome Beta 64.
Mikel, do you know who will maintain popular sites in the future?

What an interesting edge case. The ntp-internals (thanks for opening them in the video) show that IN is already set as country and - as it's not automatically refetching - the update went far enough to cache the last fetch of the sites.
Still: the displayed sites (and apparently cached sites) are the baked-in ones which should be completely replaced by the newer ones.

I don't really have an idea how this can happen. Observations so far:
- Logs don't show any failures (like they should when the download fails)
- gstatic links look good (i.e. for India/Germany where it's reproducible)
- version and country are always cached once the all new sites were written to the cache: https://cs.chromium.org/chromium/src/components/ntp_tiles/popular_sites_impl.cc?sq=package:chromium&dr=CSs&l=483 ... so how can the cache still contain the old sites but the new version indicator?
Found the underlying issue:
We refresh popular sites only, if we have less than 8 tiles which are not "POPULAR". We introduced a second type of popular tiles called "POPULAR_BAKED_IN" which were not considered "POPULAR". The fix is just checking for that second line.
The one liner is out for review here: https://crrev.com/c/878322
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 22 2018

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

commit 764f29082e59ba67ca66961611475f7b647e1bd2
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Mon Jan 22 14:28:48 2018

Fix popular sites missing refreshes

We count baked-in popular sites to personal sites when deciding
whether to refetch the current set of popular sites.
This causes popular sites to not refresh if the first fetch failed.

With this CL, we treat baked-in popular sites as popular sites which
brings back timely refreshes.

Bug:  803773 
Change-Id: I96c34f02f440d453eb1ae3b422c422c4827b0127
Reviewed-on: https://chromium-review.googlesource.com/878322
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530857}
[modify] https://crrev.com/764f29082e59ba67ca66961611475f7b647e1bd2/components/ntp_tiles/most_visited_sites.cc

Status: Fixed (was: Assigned)
This issue should now be fixed on M65. I assume we want to merge back to M64. Would someone please verify with an iOS device?
Issue is still reproducible in 65.0.3325.11 beta, might require to merge the changes in to M65.  

Issue is fixed in 66.0.3329.0 Canary, popular sites are now displayed on following steps mentioned in comment #0.

Tested on:
iPhone 7(iOS 10.3.3)
iPhone 8 plus(iOS 11.2.5 beta 7)
iPad Air(iOS 11.2.5 beta7)

Link to video of M66:
https://drive.google.com/file/d/1MyoWdMWtG5e38kldJUZNzpJEfSdq9nM9/view?usp=sharing

Labels: Merge-Request-65
Thanks for testing and the correction: yes, I meant 66.
I am requesting a merge for 65.
Project Member

Comment 9 by sheriffbot@chromium.org, Jan 24 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 24 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e19030082989ad8bded403ff7534d68c7030359

commit 9e19030082989ad8bded403ff7534d68c7030359
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Wed Jan 24 11:47:45 2018

Fix popular sites missing refreshes

We count baked-in popular sites to personal sites when deciding
whether to refetch the current set of popular sites.
This causes popular sites to not refresh if the first fetch failed.

With this CL, we treat baked-in popular sites as popular sites which
brings back timely refreshes.

Bug:  803773 
Change-Id: I96c34f02f440d453eb1ae3b422c422c4827b0127
Reviewed-on: https://chromium-review.googlesource.com/878322
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#530857}(cherry picked from commit 764f29082e59ba67ca66961611475f7b647e1bd2)
Reviewed-on: https://chromium-review.googlesource.com/883441
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#58}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/9e19030082989ad8bded403ff7534d68c7030359/components/ntp_tiles/most_visited_sites.cc

The merge happened, with the next 65 build, it should work as intended again.
Verified in 65.0.3325.20 Beta, popular sites are now displayed on following step mentioned in comment #0. Works as intended.

Tested on following devices:
iPhone 7plus(iOS 10.3.3)
iPhone X (iOS 11.2.2)
iPad Air(iOS 11.2.5 beta7)
Status: Verified (was: Fixed)

Sign in to add a comment