[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:
 - It makes sure the task doesn't run (significantly) before its initial period has elapsed. In practice, the scheduler seems to behave like that anyway, but it doesn't guarantee that, so we shouldn't rely on it.
 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
Committed: https://crrev.com/df864545989e5d99922d2ba88f94e5170aa0d833
Cr-Commit-Position: refs/heads/master@{#421175}

[Marc Treib, 2016-09-23 18:16:49]

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

[Marc Treib, 2016-09-23 18:16:50]

...and another (small) one. This should be the last in the series!

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:106:
long flexSeconds = (long) (periodSeconds * 0.2);
The factors are a bit arbitrary, but IMO the exact values don't really matter -
WDYT?

[jkrcal, 2016-09-26 16:08:18]

lgtm  (the comment below is for the future)

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:106:
long flexSeconds = (long) (periodSeconds * 0.2);
On 2016/09/23 18:16:50, Marc Treib wrote:
> The factors are a bit arbitrary, but IMO the exact values don't really matter
-
> WDYT?

This looks okay. I am not happy with the overall thing, though, we achieve
through the API now. What I would like to have:
 a) Wait for the period (say, 0.9 * period)
 b) Try to find an opportune time on wifi to fetch (say, in a window of
0.2*period, could be even larger if we have good data that Android is not always
pushing it to the far end)
 c) If it fails, try to fetch as soon as you get wifi connectivity (and Android
thinks it is a good idea at the given moment).
 d) Only if you do not have wi-fi in the whole time up to the scheduled
fallback, fetch over cellular.

Definitely beyond the scope of this CL, anyway:
 - In the current strategy, I miss c). Does it make sense to you?
 - I would like to get an UMA histogram reporting the difference of the period
and the actual fetching. (Maybe two histograms - one for fetching earlier than
period, the other for fetching later then period to make sure the values are
always positive.)

[Marc Treib, 2016-09-27 08:27:24]

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:106:
long flexSeconds = (long) (periodSeconds * 0.2);
On 2016/09/26 16:08:18, jkrcal wrote:
> On 2016/09/23 18:16:50, Marc Treib wrote:
> > The factors are a bit arbitrary, but IMO the exact values don't really
matter
> -
> > WDYT?
> 
> This looks okay. I am not happy with the overall thing, though, we achieve
> through the API now. What I would like to have:
>  a) Wait for the period (say, 0.9 * period)
>  b) Try to find an opportune time on wifi to fetch (say, in a window of
> 0.2*period, could be even larger if we have good data that Android is not
always
> pushing it to the far end)
>  c) If it fails, try to fetch as soon as you get wifi connectivity (and
Android
> thinks it is a good idea at the given moment).
>  d) Only if you do not have wi-fi in the whole time up to the scheduled
> fallback, fetch over cellular.
> 
> Definitely beyond the scope of this CL, anyway:
>  - In the current strategy, I miss c). Does it make sense to you?
>  - I would like to get an UMA histogram reporting the difference of the period
> and the actual fetching. (Maybe two histograms - one for fetching earlier than
> period, the other for fetching later then period to make sure the values are
> always positive.)

Re c): That's what the API seems to do in practice anyway (at least without flex
- haven't tested it with flex).

Re UMA: Yes, something like that is probably a good idea. For another CL... :)

[Marc Treib, 2016-09-27 08:28:15]

Description was changed from

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

BUG=646842
==========

to

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:

 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.

 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
==========

[Marc Treib, 2016-09-27 08:28:25]

Description was changed from

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:

 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.

 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
==========

to

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:
 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.
 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
==========

[Marc Treib, 2016-09-27 08:28:45]

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

[Marc Treib, 2016-09-27 08:28:46]

+bauerb for OWNERS approval. PTAL!

[Bernhard Bauer, 2016-09-27 09:41:58]

lgtm

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:105:
long effectivePeriodSeconds = (long) (periodSeconds * 1.1);
Maybe extract these parameters to constants?

[Marc Treib, 2016-09-27 10:25:52]

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/2366813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:105:
long effectivePeriodSeconds = (long) (periodSeconds * 1.1);
On 2016/09/27 09:41:58, Bernhard Bauer wrote:
> Maybe extract these parameters to constants?

Since they're essentially the same constant, I've made one constant:
FLEX_FACTOR = 0.1

[Marc Treib, 2016-09-27 11:12:52]

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

[commit-bot: I haz the power, 2016-09-27 11:13:00]

Dry run: 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-09-27 11:48:48]

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

[commit-bot: I haz the power, 2016-09-27 11:48:49]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-09-27 11:49:35]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-09-27 11:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2366813003/#ps20001
(title: "bauerb review")

[commit-bot: I haz the power, 2016-09-27 11:49:49]

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-09-27 11:54:52]

Description was changed from

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:
 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.
 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
==========

to

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:
 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.
 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
==========

[commit-bot: I haz the power, 2016-09-27 11:54:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-27 11:56:58]

Description was changed from

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:
 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.
 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842
==========

to

==========
[NTP Snippets] Add a bit of 'flex' to the background fetching intervals

That achieves the following:
 - It makes sure the task doesn't run (significantly) before its initial period
has elapsed. In practice, the scheduler seems to behave like that anyway, but it
doesn't guarantee that, so we shouldn't rely on it.
 - It gives the scheduler a bit of room to optimize for battery life.

BUG=646842

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

[commit-bot: I haz the power, 2016-09-27 11:56:59]

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