ntp_tiles: fetch popular sites icons for 2nd NTP

ntp_tiles: fetch popular sites icons for 2nd NTP

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

[sfiera, 2017-02-27 12:28:01]

sfiera@chromium.org changed reviewers:
+ fhorschig@chromium.org, treib@chromium.org

[sfiera, 2017-02-27 12:28:02]

This doesn't make sense before https://crrev.com/2695713004 lands.

[Marc Treib, 2017-02-27 12:33:36]

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:424:
icon_cacher_->StartFetch(popular_site,
Hrm. So if the NTP gets closed in the meantime, then all this will get aborted,
right?

That indeed sounds like we should make MostVisitedSites long-lived. On desktop,
it already is, through being owned by InstantService.

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:425: base::Bind([](bool
newly_available) {}));
Can we just pass a null Callback here?

[sfiera, 2017-02-27 12:51:33]

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:424:
icon_cacher_->StartFetch(popular_site,
On 2017/02/27 12:33:35, Marc Treib wrote:
> Hrm. So if the NTP gets closed in the meantime, then all this will get
aborted,
> right?

Yes. But that's already true of PopularSites as a whole.

> That indeed sounds like we should make MostVisitedSites long-lived. On
desktop,
> it already is, through being owned by InstantService.

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:425: base::Bind([](bool
newly_available) {}));
On 2017/02/27 12:33:35, Marc Treib wrote:
> Can we just pass a null Callback here?

Do you mean to change the signature of StartFetch(), or some other way to make
this callback?

[Marc Treib, 2017-02-27 13:09:35]

lgtm

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:425: base::Bind([](bool
newly_available) {}));
On 2017/02/27 12:51:33, sfiera wrote:
> On 2017/02/27 12:33:35, Marc Treib wrote:
> > Can we just pass a null Callback here?
> 
> Do you mean to change the signature of StartFetch(), or some other way to make
> this callback?

No, I mean just passing "PopularSites::FinishedCallback()" which IMO would be a
bit easier to read than the "empty" Bind() call.
(But then PopularSites would have to check the callback for validity before
calling it, so maybe that's more churn than it's worth.)

[fhorschig, 2017-02-27 15:40:28]

lgtm, thank you!

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
File components/ntp_tiles/most_visited_sites.cc (right):

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:424:
icon_cacher_->StartFetch(popular_site,
On 2017/02/27 12:51:33, sfiera wrote:
> On 2017/02/27 12:33:35, Marc Treib wrote:
> > Hrm. So if the NTP gets closed in the meantime, then all this will get
> aborted,
> > right?
> 
> Yes. But that's already true of PopularSites as a whole.
> 
> > That indeed sounds like we should make MostVisitedSites long-lived. On
> desktop,
> > it already is, through being owned by InstantService.

+1 to making MVS/PS long-lived.
(Maybe not in this CL, though)

https://codereview.chromium.org/2720513004/diff/1/components/ntp_tiles/most_v...
components/ntp_tiles/most_visited_sites.cc:425: base::Bind([](bool
newly_available) {}));
On 2017/02/27 13:09:35, Marc Treib wrote:
> On 2017/02/27 12:51:33, sfiera wrote:
> > On 2017/02/27 12:33:35, Marc Treib wrote:
> > > Can we just pass a null Callback here?
> > 
> > Do you mean to change the signature of StartFetch(), or some other way to
make
> > this callback?
> 
> No, I mean just passing "PopularSites::FinishedCallback()" which IMO would be
a
> bit easier to read than the "empty" Bind() call.
> (But then PopularSites would have to check the callback for validity before
> calling it, so maybe that's more churn than it's worth.)

Agree to the readability but I think it's more likely that 
we drop the callback in the future than checking it for validity.
We currently use it only for warning, testing and debugging.
(And all of those uses are debatable...)

[sfiera, 2017-02-27 17:43:21]

I'm inclined to keep the change local.

Waiting on fhorschig@'s main CL before submitting.

[sfiera, 2017-03-02 19:37:39]

Closing in favor of fhorschig's patched version in https://crrev.com/2724313003

[sfiera, 2017-03-02 19:37:46]

Description was changed from

==========
ntp_tiles: fetch popular sites icons for 2nd NTP

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

==========
ntp_tiles: fetch popular sites icons for 2nd NTP

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
==========