Start PopularSites fetch from separate function.

Start PopularSites fetch from separate function.

Then, inject a PopularSites into MostVisited, instead of injecting all
of its arguments and creating one locally.

BUG=617966
Committed: https://crrev.com/00a58ab2c18a43c66d4b2d13ee9f33f9a390b6e4
Cr-Commit-Position: refs/heads/master@{#408369}

[sfiera, 2016-07-26 13:39:42]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org

[sfiera, 2016-07-26 13:39:42]

After a long pause, a CL appears!

Note that we now always create a PopularSites object, even if we won't use it
(because PopularSites is off, or the user has 8 personal tiles). It's still
reasonably lightweight if we never start a fetch, but we could expose some of
MostVisited's logic about whether PopularSites is needed in order to avoid
creating one.

[Marc Treib, 2016-07-26 13:46:21]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-07-26 13:46:22]

Arguably, the logic whether PopularSites needed shouldn't be in MostVisitedSites
in the first place...

https://codereview.chromium.org/2179233003/diff/1/components/ntp_tiles/popula...
File components/ntp_tiles/popular_sites.h (right):

https://codereview.chromium.org/2179233003/diff/1/components/ntp_tiles/popula...
components/ntp_tiles/popular_sites.h:78: void StartFetch(bool force_download,
const FinishedCallback& callback);
It is legal to call this multiple times? I suppose not. I'd add that to the
comment, and if possible DCHECK in the implementation.

[sfiera, 2016-07-26 13:56:54]

On 2016/07/26 13:46:22, Marc Treib wrote:
> Arguably, the logic whether PopularSites needed shouldn't be in
MostVisitedSites
> in the first place...

But now that I look at it, that's difficult, because we might not know if
they're needed until we know the number of tiles we get through TopSites.

On the other hand, if the feature is disabled, we could short-circuit that
easily enough.

https://codereview.chromium.org/2179233003/diff/1/components/ntp_tiles/popula...
File components/ntp_tiles/popular_sites.h (right):

https://codereview.chromium.org/2179233003/diff/1/components/ntp_tiles/popula...
components/ntp_tiles/popular_sites.h:78: void StartFetch(bool force_download,
const FinishedCallback& callback);
On 2016/07/26 13:46:22, Marc Treib wrote:
> It is legal to call this multiple times? I suppose not. I'd add that to the
> comment, and if possible DCHECK in the implementation.

No, done, and done.

[Marc Treib, 2016-07-26 14:24:07]

LGTM

On Tue, Jul 26, 2016 at 3:56 PM <sfiera@chromium.org> wrote:

> On 2016/07/26 13:46:22, Marc Treib wrote:
> > Arguably, the logic whether PopularSites needed shouldn't be in
> MostVisitedSites
> > in the first place...
>
> But now that I look at it, that's difficult, because we might not know if
> they're needed until we know the number of tiles we get through TopSites.
>
> On the other hand, if the feature is disabled, we could short-circuit that
> easily enough.
>
>
> Right now, we get that info from a pref that stores the previous number of
personal sites. We might as well check that inside PopularSites.
Anyway, that's a discussion for another day.


>
>
>
https://codereview.chromium.org/2179233003/diff/1/components/ntp_tiles/popula...
> File components/ntp_tiles/popular_sites.h (right):
>
>
>
https://codereview.chromium.org/2179233003/diff/1/components/ntp_tiles/popula...
> components/ntp_tiles/popular_sites.h:78: void StartFetch(bool
> force_download, const FinishedCallback& callback);
> On 2016/07/26 13:46:22, Marc Treib wrote:
> > It is legal to call this multiple times? I suppose not. I'd add that
> to the
> > comment, and if possible DCHECK in the implementation.
>
> No, done, and done.
>
> https://codereview.chromium.org/2179233003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sfiera, 2016-07-28 09:07:35]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 09:07:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-07-28 09:15:38]

The CQ bit was checked by sfiera@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 09:15:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bernhard Bauer, 2016-07-28 09:19:16]

LGTM (did Rietveld not pick up yours, Marc?)

[Marc Treib, 2016-07-28 09:20:18]

On 2016/07/28 09:19:16, Bernhard Bauer wrote:
> LGTM (did Rietveld not pick up yours, Marc?)

Huh, apparently it didn't. Weird.
Is LGTMing via mail not supported or something?

[sfiera, 2016-07-28 09:23:09]

The CQ bit was unchecked by sfiera@chromium.org

[sfiera, 2016-07-28 09:23:10]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2016-07-28 09:23:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sfiera, 2016-07-28 10:02:43]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2016-07-28 10:03:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 10:21:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 10:23:53]

Description was changed from

==========
Start PopularSites fetch from separate function.

Then, inject a PopularSites into MostVisited, instead of injecting all
of its arguments and creating one locally.

BUG=617966
==========

to

==========
Start PopularSites fetch from separate function.

Then, inject a PopularSites into MostVisited, instead of injecting all
of its arguments and creating one locally.

BUG=617966

Committed: https://crrev.com/00a58ab2c18a43c66d4b2d13ee9f33f9a390b6e4
Cr-Commit-Position: refs/heads/master@{#408369}
==========

[commit-bot: I haz the power, 2016-07-28 10:23:54]

Patchset 3 (id:??) landed as
https://crrev.com/00a58ab2c18a43c66d4b2d13ee9f33f9a390b6e4
Cr-Commit-Position: refs/heads/master@{#408369}

[chromium-reviews, 2016-07-28 11:11:19]

Oh right, that's a security feature, as emails are ridiculously easy to
forge. I think it should reply with a message though telling you that you
need to LGTM from the web interface?

On Thu, Jul 28, 2016, 11:23 commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Patchset 3 (id:??) landed as
> https://crrev.com/00a58ab2c18a43c66d4b2d13ee9f33f9a390b6e4
> Cr-Commit-Position: refs/heads/master@{#408369}
>
> https://codereview.chromium.org/2179233003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Marc Treib, 2016-07-28 11:16:37]

Ah, I did get an email, but it wasn't all that clear, and it got lost in
the regular review/bug spam :)
TIL!

On Thu, Jul 28, 2016 at 1:11 PM Bernhard Bauer <bauerb@google.com> wrote:

> Oh right, that's a security feature, as emails are ridiculously easy to
> forge. I think it should reply with a message though telling you that you
> need to LGTM from the web interface?
>
> On Thu, Jul 28, 2016, 11:23 commit-bot@chromium.org via
> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com>
> wrote:
>
>> Patchset 3 (id:??) landed as
>> https://crrev.com/00a58ab2c18a43c66d4b2d13ee9f33f9a390b6e4
>> Cr-Commit-Position: refs/heads/master@{#408369}
>>
>> https://codereview.chromium.org/2179233003/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.