Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724313003:
Tiles On NTP (copy): fetch popular sites icons for 2nd NTP (Closed)
DescriptionTiles On NTP (copy): fetch popular sites icons for 2nd NTP
This CL is a copy of https://codereview.chromium.org/2720513004/ to
change ownership.
Copied description:
The first-run NTP uses our bundled popular sites file along with its
icons. It also starts a fetch for regionally-popular sites, but didn't
also fetch their icons, meaning that the icons would pop into the second
NTP after it had loaded. With this, we fetch them from the first NTP as
soon as we have the regionally-popular sites.
I think we've gotten ourselves into a really weird situation with
ntp_tiles at this point. By and large, we don't care about the result of
running popular sites; the sites are used only in the next NTP
iteration. Probably time to do https://crbug.com/650589. With that, we
might even have a chance to fetch regionally-popular sites before the
FRE makes it to the NTP.
BUG=694270
Review-Url: https://codereview.chromium.org/2724313003
Cr-Commit-Position: refs/heads/master@{#454428}
Committed: https://chromium.googlesource.com/chromium/src/+/b21ac75e9523f23a71e27ed0455c93815e65f49b
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 2
Depends on Patchset: Messages
Total messages: 21 (13 generated)
Hi Marc, could you please take a look? (I copied this CL from Chris, in case any merge conflicts occur with my large CL as he won't be able to merge it himself)
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
sfiera@chromium.org changed reviewers: + sfiera@chromium.org
LGTM (this isn't the exact same thing that I wrote, so my LGTM still counts, right?)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/02 19:31:59, sfiera wrote: > LGTM > > (this isn't the exact same thing that I wrote, so my LGTM still counts, right?) If you like, I can TBR marc and myself :D
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fhorschig@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": 1488496646364670,
"parent_rev": "ef768ffc5dbe5364af1db5c04da524109b4b5f08", "commit_rev":
"b21ac75e9523f23a71e27ed0455c93815e65f49b"}
Message was sent while issue was closed.
Description was changed from ========== Tiles On NTP (copy): fetch popular sites icons for 2nd NTP This CL is a copy of https://codereview.chromium.org/2720513004/ to change ownership. Copied description: The first-run NTP uses our bundled popular sites file along with its icons. It also starts a fetch for regionally-popular sites, but didn't also fetch their icons, meaning that the icons would pop into the second NTP after it had loaded. With this, we fetch them from the first NTP as soon as we have the regionally-popular sites. I think we've gotten ourselves into a really weird situation with ntp_tiles at this point. By and large, we don't care about the result of running popular sites; the sites are used only in the next NTP iteration. Probably time to do https://crbug.com/650589. With that, we might even have a chance to fetch regionally-popular sites before the FRE makes it to the NTP. BUG=694270 ========== to ========== Tiles On NTP (copy): fetch popular sites icons for 2nd NTP This CL is a copy of https://codereview.chromium.org/2720513004/ to change ownership. Copied description: The first-run NTP uses our bundled popular sites file along with its icons. It also starts a fetch for regionally-popular sites, but didn't also fetch their icons, meaning that the icons would pop into the second NTP after it had loaded. With this, we fetch them from the first NTP as soon as we have the regionally-popular sites. I think we've gotten ourselves into a really weird situation with ntp_tiles at this point. By and large, we don't care about the result of running popular sites; the sites are used only in the next NTP iteration. Probably time to do https://crbug.com/650589. With that, we might even have a chance to fetch regionally-popular sites before the FRE makes it to the NTP. BUG=694270 Review-Url: https://codereview.chromium.org/2724313003 Cr-Commit-Position: refs/heads/master@{#454428} Committed: https://chromium.googlesource.com/chromium/src/+/b21ac75e9523f23a71e27ed0455c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b21ac75e9523f23a71e27ed0455c...
Message was sent while issue was closed.
treib@chromium.org changed reviewers: + treib@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2724313003/diff/20001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2724313003/diff/20001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.cc:425: icon_cacher_->StartFetch(popular_site, base::Closure(), base::Closure()); IconCacherImpl DCHECKs that the first closure isn't null - looks like this would trigger that? Did I miss something?
Message was sent while issue was closed.
https://codereview.chromium.org/2724313003/diff/20001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2724313003/diff/20001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.cc:425: icon_cacher_->StartFetch(popular_site, base::Closure(), base::Closure()); On 2017/03/03 09:40:44, Marc Treib wrote: > IconCacherImpl DCHECKs that the first closure isn't null - looks like this would > trigger that? > Did I miss something? Yes, there is a fix for that: https://codereview.chromium.org/2725293002/ If that isn't fast enough, I can also rollback.
Message was sent while issue was closed.
https://codereview.chromium.org/2724313003/diff/20001/components/ntp_tiles/mo... File components/ntp_tiles/most_visited_sites.cc (right): https://codereview.chromium.org/2724313003/diff/20001/components/ntp_tiles/mo... components/ntp_tiles/most_visited_sites.cc:425: icon_cacher_->StartFetch(popular_site, base::Closure(), base::Closure()); On 2017/03/03 09:40:44, Marc Treib wrote: > IconCacherImpl DCHECKs that the first closure isn't null - looks like this would > trigger that? > Did I miss something? Yes, there is a fix for that: https://codereview.chromium.org/2725293002/ If that isn't fast enough, I can also rollback. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
