[NTP Snippets] Safe(r) defaults for background fetching intervals

[NTP Snippets] Safe(r) defaults for background fetching intervals

BUG=none

Committed: https://crrev.com/8e47dfc3ac0fb7394aa781e30b03794e558cc7b5
Cr-Commit-Position: refs/heads/master@{#414407}

[Marc Treib, 2016-08-25 11:24:33]

Description was changed from

==========
[NTP Snippets] Use safe defaults for background fetching intervals

BUG=none
==========

to

==========
[NTP Snippets] Use safe(r) defaults for background fetching intervals

BUG=none
==========

[Marc Treib, 2016-08-25 11:24:43]

Description was changed from

==========
[NTP Snippets] Use safe(r) defaults for background fetching intervals

BUG=none
==========

to

==========
[NTP Snippets] Safe(r) defaults for background fetching intervals

BUG=none
==========

[Marc Treib, 2016-08-25 11:24:54]

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

[Marc Treib, 2016-08-25 11:25:10]

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

[Marc Treib, 2016-08-25 11:25:10]

PTAL!

[commit-bot: I haz the power, 2016-08-25 11:25:13]

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

[sfiera, 2016-08-25 11:29:43]

LGTM

[commit-bot: I haz the power, 2016-08-25 11:30:36]

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

[commit-bot: I haz the power, 2016-08-25 11:30:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2016-08-25 11:36:28]

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

[commit-bot: I haz the power, 2016-08-25 11:36:45]

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-08-25 11:41:43]

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

[commit-bot: I haz the power, 2016-08-25 11:41:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Marc Treib, 2016-08-25 11:49:52]

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

[commit-bot: I haz the power, 2016-08-25 11:50:07]

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-08-25 12:24:21]

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

[commit-bot: I haz the power, 2016-08-25 12:24:22]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-08-25 12:25:42]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-08-25 12:26:04]

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-08-25 12:30:05]

Description was changed from

==========
[NTP Snippets] Safe(r) defaults for background fetching intervals

BUG=none
==========

to

==========
[NTP Snippets] Safe(r) defaults for background fetching intervals

BUG=none
==========

[commit-bot: I haz the power, 2016-08-25 12:30:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-25 12:32:57]

Description was changed from

==========
[NTP Snippets] Safe(r) defaults for background fetching intervals

BUG=none
==========

to

==========
[NTP Snippets] Safe(r) defaults for background fetching intervals

BUG=none

Committed: https://crrev.com/8e47dfc3ac0fb7394aa781e30b03794e558cc7b5
Cr-Commit-Position: refs/heads/master@{#414407}
==========

[commit-bot: I haz the power, 2016-08-25 12:32:58]

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

[tschumann, 2016-08-25 20:28:52]

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

[tschumann, 2016-08-25 20:28:52]

good to get these values into a range that scalable ;-)

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

https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:54: const int
kDefaultFetchingIntervalWifiChargingSeconds = 0;
it would be really good to document this behavior -- how does this result in
once per day?
Is there any preference implemented to get it do a request over wifi?
is a value of zero deactivating the scheduling? (and what are the implications
on other fetches).

That aside: this also works when the program got kicked out of memory, right?

[Marc Treib, 2016-08-26 08:24:49]

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

https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:54: const int
kDefaultFetchingIntervalWifiChargingSeconds = 0;
On 2016/08/25 20:28:52, tschumann wrote:
> it would be really good to document this behavior -- how does this result in
> once per day?
> Is there any preference implemented to get it do a request over wifi?
> is a value of zero deactivating the scheduling? (and what are the implications
> on other fetches).
> 
> That aside: this also works when the program got kicked out of memory, right?

A value of "0" disables the fetching, correct. That behavior is documented on
the NTPSnippetsScheduler interface, though it might make sense to repeat it here
:)

The "once per day" just comes from kDefaultFetchingIntervalFallbackSeconds,
which is not zero.

All these default values can be overridden via Finch.

Yes, this works even when Chrome isn't running. The OS will wake up the device,
start Chrome, and tell it to fetch.

[tschumann, 2016-08-29 12:38:42]

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

https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:54: const int
kDefaultFetchingIntervalWifiChargingSeconds = 0;
On 2016/08/26 08:24:49, Marc Treib wrote:
> On 2016/08/25 20:28:52, tschumann wrote:
> > it would be really good to document this behavior -- how does this result in
> > once per day?
> > Is there any preference implemented to get it do a request over wifi?
> > is a value of zero deactivating the scheduling? (and what are the
implications
> > on other fetches).
> > 
> > That aside: this also works when the program got kicked out of memory,
right?
> 
> A value of "0" disables the fetching, correct. That behavior is documented on
> the NTPSnippetsScheduler interface, though it might make sense to repeat it
here
> :)
yes, cause when choosing these values you rely on the implementation details, so
better document.
> 
> The "once per day" just comes from kDefaultFetchingIntervalFallbackSeconds,
> which is not zero.
> 
> All these default values can be overridden via Finch.
> 
> Yes, this works even when Chrome isn't running. The OS will wake up the
device,
> start Chrome, and tell it to fetch.

So this means, we don't have a preference to fetch data on wifi-only? 
Now that we have the throttler in place, we can set an upper limit of one
background fetch per day. And then try fetching via wifi every hour and have the
24h fallback in case of no wifi connectivity. WDYT?

[Marc Treib, 2016-08-29 12:44:24]

On 2016/08/29 12:38:42, tschumann wrote:
>
https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
> File components/ntp_snippets/ntp_snippets_service.cc (right):
> 
>
https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
> components/ntp_snippets/ntp_snippets_service.cc:54: const int
> kDefaultFetchingIntervalWifiChargingSeconds = 0;
> On 2016/08/26 08:24:49, Marc Treib wrote:
> > On 2016/08/25 20:28:52, tschumann wrote:
> > > it would be really good to document this behavior -- how does this result
in
> > > once per day?
> > > Is there any preference implemented to get it do a request over wifi?
> > > is a value of zero deactivating the scheduling? (and what are the
> implications
> > > on other fetches).
> > > 
> > > That aside: this also works when the program got kicked out of memory,
> right?
> > 
> > A value of "0" disables the fetching, correct. That behavior is documented
on
> > the NTPSnippetsScheduler interface, though it might make sense to repeat it
> here
> > :)
> yes, cause when choosing these values you rely on the implementation details,
so
> better document.
> > 
> > The "once per day" just comes from kDefaultFetchingIntervalFallbackSeconds,
> > which is not zero.
> > 
> > All these default values can be overridden via Finch.
> > 
> > Yes, this works even when Chrome isn't running. The OS will wake up the
> device,
> > start Chrome, and tell it to fetch.
> 
> So this means, we don't have a preference to fetch data on wifi-only? 
> Now that we have the throttler in place, we can set an upper limit of one
> background fetch per day. And then try fetching via wifi every hour and have
the
> 24h fallback in case of no wifi connectivity. WDYT?

Right now, a successful fetch doesn't reset the 24h fallback. It probably should
:)
Anyway, the point here was just to have hardcoded defaults that won't completely
fry our servers, if the Finch config were missing for some reason. This CL
doesn't take away any functionality, we can still configure everything.

[tschumann, 2016-08-29 12:56:02]

On 2016/08/29 12:44:24, Marc Treib wrote:
> On 2016/08/29 12:38:42, tschumann wrote:
> >
>
https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
> > File components/ntp_snippets/ntp_snippets_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2277223002/diff/20001/components/ntp_snippets...
> > components/ntp_snippets/ntp_snippets_service.cc:54: const int
> > kDefaultFetchingIntervalWifiChargingSeconds = 0;
> > On 2016/08/26 08:24:49, Marc Treib wrote:
> > > On 2016/08/25 20:28:52, tschumann wrote:
> > > > it would be really good to document this behavior -- how does this
result
> in
> > > > once per day?
> > > > Is there any preference implemented to get it do a request over wifi?
> > > > is a value of zero deactivating the scheduling? (and what are the
> > implications
> > > > on other fetches).
> > > > 
> > > > That aside: this also works when the program got kicked out of memory,
> > right?
> > > 
> > > A value of "0" disables the fetching, correct. That behavior is documented
> on
> > > the NTPSnippetsScheduler interface, though it might make sense to repeat
it
> > here
> > > :)
> > yes, cause when choosing these values you rely on the implementation
details,
> so
> > better document.
> > > 
> > > The "once per day" just comes from
kDefaultFetchingIntervalFallbackSeconds,
> > > which is not zero.
> > > 
> > > All these default values can be overridden via Finch.
> > > 
> > > Yes, this works even when Chrome isn't running. The OS will wake up the
> > device,
> > > start Chrome, and tell it to fetch.
> > 
> > So this means, we don't have a preference to fetch data on wifi-only? 
> > Now that we have the throttler in place, we can set an upper limit of one
> > background fetch per day. And then try fetching via wifi every hour and have
> the
> > 24h fallback in case of no wifi connectivity. WDYT?
> 
> Right now, a successful fetch doesn't reset the 24h fallback. It probably
should
> :)
> Anyway, the point here was just to have hardcoded defaults that won't
completely
> fry our servers, if the Finch config were missing for some reason. This CL
> doesn't take away any functionality, we can still configure everything.

that's true, but if we're assuming the finch config might get screwed up badly,
I wonder if we could guarantee a bit better user experience (e.g. try fetching
over wifi every 12h?). Maybe some logic is actually quite broken here -- let's
sync up offline (I have to run for an interview now I'm available then).