[NTP Snippets] Scheduler: Fetch on Wifi (without charging) only 6AM-10PM

[NTP Snippets] Scheduler: Fetch on Wifi (without charging) only 6AM-10PM

BUG=587857
Committed: https://crrev.com/e59dc80836a5ca8038b18c27f4660aed2aba253c
Cr-Commit-Position: refs/heads/master@{#388231}

[Marc Treib, 2016-04-08 15:50:47]

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

[Marc Treib, 2016-04-08 15:50:47]

One last one to round off the week :D
(Not urgent, no need to look at this before branch point or anything!)

[Bernhard Bauer, 2016-04-11 13:00:58]

https://codereview.chromium.org/1869323003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/1869323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:96:
rescheduleSnippets();
I'm not very happy about the fact that we launch the browser twice a day just to
change the parameters for background fetching. Would it be possible to do this
purely in Java? The native side would just pass in the configuration parameters
would do all the scheduling.

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:74: return base::TimeDelta();
Would it make sense to use the fallback value here? Otherwise between 10PM and
6AM we would not fetch all if we're on Wifi, but we might fetch if we're on
cellular o_O

[Marc Treib, 2016-04-11 13:25:26]

https://codereview.chromium.org/1869323003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/1869323003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:96:
rescheduleSnippets();
On 2016/04/11 13:00:58, Bernhard Bauer wrote:
> I'm not very happy about the fact that we launch the browser twice a day just
to
> change the parameters for background fetching. Would it be possible to do this
> purely in Java? The native side would just pass in the configuration
parameters
> would do all the scheduling.

Hm. Possible, sure, but it'd mean moving most of the scheduling logic into
platform-dependent code. (Or possibly even duplicating it, since we'll probably
need it in native code as well.)

Do we have numbers on the relative costs for launching the browser, vs. just
launching the Java part? But I guess the difference would be fairly big...

One step back: Do we absolutely need this at all? It's a lot of extra
complexity, for IMO somewhat questionable gain.

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:74: return base::TimeDelta();
On 2016/04/11 13:00:58, Bernhard Bauer wrote:
> Would it make sense to use the fallback value here? Otherwise between 10PM and
> 6AM we would not fetch all if we're on Wifi, but we might fetch if we're on
> cellular o_O

No, the "Fallback" wakeup would still trigger even if we're on Wifi.

Semi-related: It can happen that multiple wakeups trigger at the same time, e.g.
"Wifi" and "WifiCharging". Right now we'd fetch snippets twice in a row in that
case. I guess a minimum interval between automatic fetches would handle that.

[Marc Treib, 2016-04-14 13:30:13]

So, after our discussion: Ping! :)

There's crbug.com/602965 for the follow-up work.

[Bernhard Bauer, 2016-04-18 08:03:00]

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_scheduler.h:26: base::TimeDelta
reschedule_delay) = 0;
Would it make more sense to pass this as an absolute timestamp? That is what
we're after here anyway.

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:98: // or other shenanigans.
Wouldn't this just mean we'd reschedule for a couple of seconds later?

[Marc Treib, 2016-04-19 10:45:52]

Patchset #3 (id:40001) has been deleted

[Marc Treib, 2016-04-19 11:32:13]

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_scheduler.h:26: base::TimeDelta
reschedule_delay) = 0;
On 2016/04/18 08:03:00, Bernhard Bauer wrote:
> Would it make more sense to pass this as an absolute timestamp? That is what
> we're after here anyway.

Done.

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:98: // or other shenanigans.
On 2016/04/18 08:03:00, Bernhard Bauer wrote:
> Wouldn't this just mean we'd reschedule for a couple of seconds later?

Yup, which would mean one extra pointless wakeup that I'd like to avoid, and IMO
there's no harm in the 1 min delay. But if you feel strongly, I can of course
remove it..?

[Bernhard Bauer, 2016-04-19 15:45:35]

LGTM with the last thing fixed:

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:98: // or other shenanigans.
On 2016/04/19 11:32:13, Marc Treib wrote:
> On 2016/04/18 08:03:00, Bernhard Bauer wrote:
> > Wouldn't this just mean we'd reschedule for a couple of seconds later?
> 
> Yup, which would mean one extra pointless wakeup that I'd like to avoid, and
IMO
> there's no harm in the 1 min delay. But if you feel strongly, I can of course
> remove it..?

Yeah, because in that minute (or actually less than that, because it's just
going to be the value of the clock skew), the browser isn't that likely to get
killed again.

[Marc Treib, 2016-04-19 16:18:44]

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1869323003/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:98: // or other shenanigans.
On 2016/04/19 15:45:35, Bernhard Bauer wrote:
> On 2016/04/19 11:32:13, Marc Treib wrote:
> > On 2016/04/18 08:03:00, Bernhard Bauer wrote:
> > > Wouldn't this just mean we'd reschedule for a couple of seconds later?
> > 
> > Yup, which would mean one extra pointless wakeup that I'd like to avoid, and
> IMO
> > there's no harm in the 1 min delay. But if you feel strongly, I can of
course
> > remove it..?
> 
> Yeah, because in that minute (or actually less than that, because it's just
> going to be the value of the clock skew), the browser isn't that likely to get
> killed again.

Alright. Done.

[Marc Treib, 2016-04-19 16:18:49]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-04-19 16:18:49]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1869323003/#ps100001
(title: "remove fudge")

[commit-bot: I haz the power, 2016-04-19 16:19:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869323003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869323003/100001

[commit-bot: I haz the power, 2016-04-19 17:26:22]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:13:14]

Description was changed from

==========
[NTP Snippets] Scheduler: Fetch on Wifi (without charging) only 6AM-10PM

BUG=587857
==========

to

==========
[NTP Snippets] Scheduler: Fetch on Wifi (without charging) only 6AM-10PM

BUG=587857

Committed: https://crrev.com/e59dc80836a5ca8038b18c27f4660aed2aba253c
Cr-Commit-Position: refs/heads/master@{#388231}
==========

[commit-bot: I haz the power, 2016-04-22 19:13:15]

Patchset 5 (id:??) landed as
https://crrev.com/e59dc80836a5ca8038b18c27f4660aed2aba253c
Cr-Commit-Position: refs/heads/master@{#388231}