Expose a method in VariationsService to trigger a seed fetch.

Expose a method in VariationsService to trigger a seed fetch.

On mobile this will launch a (slightly delayed) request, iff the seed hasn't been refreshed in the past 5 hours. On desktop this will use the currently existing method on desktop, which does not do that check.

BUG=321379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251532

[rkaplow, 2014-02-12 00:33:52]

Couple notes -

I have a first usage of this partway done for Android. Basically what it does is
call VariationsService.ScheduleFetch() upon foregrounding. We'll need either me
or someone else to do that bit for iOS still. 

I left the default/desktop implementation similar to how desktop is currently
setup - without any checking. So this probably isn't great to use on something
like application foregrounding on desktop. We could also add the last_fetch_time
diff check in VariationsRequestScheduler::ForceFetch as well (and probably have
a bit more code overlap in the process). It doesn't give much benefit at the
moment though so might not be worth it.

Might also note in the test I checked if the diff was 24hours instead of
checking the constant - I figured I'd leave the constant internal and the test
is verying the class is doing something 'reasonable' - I don't mind exposing it
and checking against it directly if you'd prefer that.

[Alexei Svitkine (slow), 2014-02-12 16:22:47]

https://codereview.chromium.org/147723010/diff/1/chrome/browser/metrics/varia...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/147723010/diff/1/chrome/browser/metrics/varia...
chrome/browser/metrics/variations/variations_service.h:58: void ScheduleFetch();
I prefer if this is done similarly to MetricsService, i.e. so that this listens
to OnAppEnterBackground()/OnAppEnterForeground().

That way, the logic of what to do on those events would be in VariationsService,
rather than external to it.

In the ideal world, it would be best if there was a standard API at the Chromium
C++ level to register for these events, rather needing custom plumbing for each
sort of thing that needs it.

[rkaplow, 2014-02-12 17:44:40]

On 2014/02/12 16:22:47, Alexei Svitkine wrote:
>
https://codereview.chromium.org/147723010/diff/1/chrome/browser/metrics/varia...
> File chrome/browser/metrics/variations/variations_service.h (right):
> 
>
https://codereview.chromium.org/147723010/diff/1/chrome/browser/metrics/varia...
> chrome/browser/metrics/variations/variations_service.h:58: void
ScheduleFetch();
> I prefer if this is done similarly to MetricsService, i.e. so that this
listens
> to OnAppEnterBackground()/OnAppEnterForeground().
> 
> That way, the logic of what to do on those events would be in
VariationsService,
> rather than external to it.
> 
> In the ideal world, it would be best if there was a standard API at the
Chromium
> C++ level to register for these events, rather needing custom plumbing for
each
> sort of thing that needs it.

Ok, renamed the method for variations_service to make it more how it is used
(i.e. triggered on application foreground versus) the specifics of what it is
doing. Added a TODO regarding the refactor we talked about offline.

[Alexei Svitkine (slow), 2014-02-12 21:17:43]

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc
(right):

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc:47:
void VariationsRequestSchedulerMobile::ScheduleFetch() {
What's the reason for this being separate from
ScheduleFetchShortly()/ForceFetch()?

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
File
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc
(right):

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
Nit: 2014

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:18:
void SetToOne(int *n) {
I'd actually change this instead to be Increment(), that way, you can ensure it
only got called once, rather than multiple times.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:24:
TEST(VariationsRequestSchedulerMobileTest, StartNoRun) {
Sweet, thanks for adding tests for this!

Can you add a test for the new ScheduleFetch() API you're adding too?

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:28:
prefs::kVariationsLastFetchTime, base::Time::Now().ToInternalValue());
Nit: Wrap like this instead:
  prefs.registry()->RegisterInt64Pref(prefs::kVariationsLastFetchTime,
                                      base::Time::Now().ToInternalValue());

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:42:
prefs::kVariationsLastFetchTime, old.ToInternalValue());
Ditto.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:261:
request_scheduler_->ScheduleFetch();
In the ideal world, I'd actual have OnAppEnterForeground() be something that the
mobile scheduler implements and listens for.

I'd add a TODO here to change to such a design.

[rkaplow, 2014-02-12 22:51:51]

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc
(right):

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc:47:
void VariationsRequestSchedulerMobile::ScheduleFetch() {
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> What's the reason for this being separate from
> ScheduleFetchShortly()/ForceFetch()?

ScheduleFetchShortly()/ForceFetch() is doing something slightly different. The
intention is to recover from network errors or other problems, and it is
currently waiting 5 minutes. It's also not checking that we already have a fresh
seed. 

it was a bit tricky to merge them as I want the seed check to be after the 5
second delay - because on a fresh application start it also counts as an
Foreground event, which would trigger two separate calls if the check happened
immediately.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
File
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc
(right):

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> Nit: 2014

Done.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:18:
void SetToOne(int *n) {
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> I'd actually change this instead to be Increment(), that way, you can ensure
it
> only got called once, rather than multiple times.

Done.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:24:
TEST(VariationsRequestSchedulerMobileTest, StartNoRun) {
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> Sweet, thanks for adding tests for this!
> 
> Can you add a test for the new ScheduleFetch() API you're adding too?

Done.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:28:
prefs::kVariationsLastFetchTime, base::Time::Now().ToInternalValue());
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> Nit: Wrap like this instead:
>   prefs.registry()->RegisterInt64Pref(prefs::kVariationsLastFetchTime,
>                                       base::Time::Now().ToInternalValue());

Done.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:42:
prefs::kVariationsLastFetchTime, old.ToInternalValue());
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/147723010/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:261:
request_scheduler_->ScheduleFetch();
On 2014/02/12 21:17:44, Alexei Svitkine wrote:
> In the ideal world, I'd actual have OnAppEnterForeground() be something that
the
> mobile scheduler implements and listens for.
> 
> I'd add a TODO here to change to such a design.

I already have a TODO in the .h, I can have it here as well.

[Alexei Svitkine (slow), 2014-02-13 16:13:08]

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler.h (right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler.h:30: void
ForceFetch();
Why did you rename this? I found the previous name more intuitive (since Force
sounds like it may be immediately, whereas the other name suggests there will be
a delay).

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler.h:34: virtual
void ScheduleFetch();
Actually, I'd prefer if this was also named OnAppEnterForeground().

The idea is that in the eventual refactoring, we'd want this object to listen
for the events, and VariationsService to have no knowledge of it.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc
(right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:13:

Nit: Remove extra blank line.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:54:
base::MessageLoopForUI message_loop_;
Indent is wrong.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:72:
// Force execution of the task on this timer.
I'd expand the comment to say that this is verifying that the timer will run the
callback.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:78:

Nit: Remove extra line.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:262: // code to notify
each service individually.
My previous comment meant to emphasize that in the refactoring, this should
become a detail of VariationsRequestSchedulerMobile, and not known to
VariationsService directly. Can you expand the TODO to mention this?

[rkaplow, 2014-02-13 16:55:59]

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler.h (right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler.h:30: void
ForceFetch();
On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> Why did you rename this? I found the previous name more intuitive (since Force
> sounds like it may be immediately, whereas the other name suggests there will
be
> a delay).

Renaming was required when I had ScheduleFetch named based on what it was doing,
and it seemed the clearest way to differentiate the two. If we're switching the
other to OnApp then I can leave this as it was.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler.h:34: virtual
void ScheduleFetch();
On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> Actually, I'd prefer if this was also named OnAppEnterForeground().
> 
> The idea is that in the eventual refactoring, we'd want this object to listen
> for the events, and VariationsService to have no knowledge of it.

Done.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc
(right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:13:

On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:54:
base::MessageLoopForUI message_loop_;
On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> Indent is wrong.

Done.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:72:
// Force execution of the task on this timer.
On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> I'd expand the comment to say that this is verifying that the timer will run
the
> callback.

I didn't see a way to simulate time passing for a timer. What I test here is
that the timer is running, and then I force the task attached to the timer to be
triggered. This verifies that the timer does start, and that it has the correct
task associated.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:78:

On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> Nit: Remove extra line.

Done.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_service.cc:262: // code to notify
each service individually.
On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> My previous comment meant to emphasize that in the refactoring, this should
> become a detail of VariationsRequestSchedulerMobile, and not known to
> VariationsService directly. Can you expand the TODO to mention this?

Done.

[Alexei Svitkine (slow), 2014-02-13 17:36:22]

Looks great, just a few more comments and I think it should be good to go.

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc
(right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:72:
// Force execution of the task on this timer.
On 2014/02/13 16:55:59, rkaplow wrote:
> On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> > I'd expand the comment to say that this is verifying that the timer will run
> the
> > callback.
> 
> I didn't see a way to simulate time passing for a timer. What I test here is
> that the timer is running, and then I force the task attached to the timer to
be
> triggered. This verifies that the timer does start, and that it has the
correct
> task associated.

Right. I was just saying that you should expand the comment in the code to say
that.

https://codereview.chromium.org/147723010/diff/330009/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler.cc (right):

https://codereview.chromium.org/147723010/diff/330009/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler.cc:40: // By
default (non-mobile) we can reuse ScheduleFetchShortly.
I would actually make the implementation of this be NOTREACHED();

The idea being that we don't expect this to ever be triggered on non-mobile
platforms.

https://codereview.chromium.org/147723010/diff/330009/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/147723010/diff/330009/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1065:
'browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc',
You may need to add a rule to exclude this file from non-mobile platforms, like
we do for variations_request_scheduler_mobile.cc.

[rkaplow, 2014-02-13 18:54:47]

I'll try the desktop bots again to see if the exception will fix the failures

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
File
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc
(right):

https://codereview.chromium.org/147723010/diff/190001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc:72:
// Force execution of the task on this timer.
On 2014/02/13 17:36:22, Alexei Svitkine wrote:
> On 2014/02/13 16:55:59, rkaplow wrote:
> > On 2014/02/13 16:13:08, Alexei Svitkine wrote:
> > > I'd expand the comment to say that this is verifying that the timer will
run
> > the
> > > callback.
> > 
> > I didn't see a way to simulate time passing for a timer. What I test here is
> > that the timer is running, and then I force the task attached to the timer
to
> be
> > triggered. This verifies that the timer does start, and that it has the
> correct
> > task associated.
> 
> Right. I was just saying that you should expand the comment in the code to say
> that.

Done.

https://codereview.chromium.org/147723010/diff/330009/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler.cc (right):

https://codereview.chromium.org/147723010/diff/330009/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler.cc:40: // By
default (non-mobile) we can reuse ScheduleFetchShortly.
On 2014/02/13 17:36:22, Alexei Svitkine wrote:
> I would actually make the implementation of this be NOTREACHED();
> 
> The idea being that we don't expect this to ever be triggered on non-mobile
> platforms.

Yeah that seems safer actually. Is this all that is needed for using
NOTREACHED()?

https://codereview.chromium.org/147723010/diff/330009/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/147723010/diff/330009/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1065:
'browser/metrics/variations/variations_request_scheduler_mobile_unittest.cc',
On 2014/02/13 17:36:22, Alexei Svitkine wrote:
> You may need to add a rule to exclude this file from non-mobile platforms,
like
> we do for variations_request_scheduler_mobile.cc.

Done. I think this looks right, for !android and !ios, exclude the source.
Similar to variations_request_scheduler_mobile.cc

[Alexei Svitkine (slow), 2014-02-13 19:20:05]

LGTM assuming you get green bots.

Thanks!

[rkaplow, 2014-02-14 01:05:12]

Added the new test (and have sync merged in this patchset, sorry).

Still need to add the check to verify that onApp doesn't run at the same time

[Alexei Svitkine (slow), 2014-02-14 19:11:51]

LGTM, thanks!

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc
(right):

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc:53:
last_request_time_ + base::TimeDelta::FromHours(kSeedFetchPeriodHours)) {
Nit: Align.

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler_mobile.h
(right):

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile.h:38: 
Nit: Remove extra blank lines.

[rkaplow, 2014-02-14 19:18:04]

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc
(right):

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile.cc:53:
last_request_time_ + base::TimeDelta::FromHours(kSeedFetchPeriodHours)) {
On 2014/02/14 19:11:51, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
File chrome/browser/metrics/variations/variations_request_scheduler_mobile.h
(right):

https://codereview.chromium.org/147723010/diff/830001/chrome/browser/metrics/...
chrome/browser/metrics/variations/variations_request_scheduler_mobile.h:38: 
On 2014/02/14 19:11:51, Alexei Svitkine wrote:
> Nit: Remove extra blank lines.

Done.

[rkaplow, 2014-02-14 22:25:10]

The CQ bit was checked by rkaplow@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkaplow@chromium.org/147723010/890001

[commit-bot: I haz the power, 2014-02-15 01:12:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-15 01:12:02]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[rkaplow1, 2014-02-15 01:16:48]

The CQ bit was checked by rkaplow@google.com

[commit-bot: I haz the power, 2014-02-15 01:24:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkaplow@chromium.org/147723010/890001

[commit-bot: I haz the power, 2014-02-15 05:14:28]

Change committed as 251532