[Soft fetches] Set-up different intervals for <Chrome started> events.

[Soft fetches] Set-up different intervals for <Chrome started> events.

This CL creates different intervals for startup events. This allows:
 - iOS to compensate the missing persistent scheduler using startup soft
   fetches (setting the same intervals as for persistent fetches).
 - Android to replace most of the persistent fetches by startup soft
   fetches and hopefully save considerable traffic by that (setting
   the same intervals as they are now for persistent fetches and
   considerably enlarging the intervals for persistent fetches).

BUG=721346
Review-Url: https://codereview.chromium.org/2875953003
Cr-Commit-Position: refs/heads/master@{#472088}
Committed: https://chromium.googlesource.com/chromium/src/+/edce193983bdd176ced3394a88fb70935cf133b1

[jkrcal, 2017-05-11 13:23:06]

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

[jkrcal, 2017-05-11 13:23:06]

Markus, could you PTAL?
(would like to get it in before FF)

[jkrcal, 2017-05-11 13:23:12]

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

[commit-bot: I haz the power, 2017-05-11 13:23:30]

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, 2017-05-11 14:44:17]

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

[commit-bot: I haz the power, 2017-05-11 14:44:18]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-05-15 08:29:38]

On 2017/05/11 13:23:06, jkrcal wrote:
> Markus, could you PTAL?
> (would like to get it in before FF)

Friendly ping!

[markusheintz_, 2017-05-15 09:35:47]

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
File components/ntp_snippets/pref_names.cc (right):

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.cc:16: // For backwards compatibility, we do
not rename
I guess that comment can be moved to the corresponding prefs below.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
File components/ntp_snippets/pref_names.h (right):

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.h:25: //  - "Wifi" for situations with
unmetered network connectivity, and
We don't know whether a WIFI is unmetered.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.h:26: //  - "Fallback" for situations with
only metered network.
Same here just the other way around.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.h:28: // Trigger: wake-up of the persistent
scheduler.
The trigger comments are confusing. What information do you want to convey here?

That logic for checking whether this interval has already elapsed are triggered
by the mention situation?

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
(right):

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:36: //   - A
"startup" fetch is performed only at a moment when the user starts /
Can we mention the FetchInterval names from below in the comment? I guess here
we reference the STARTUP_* intervals.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:38: //  
BROWSER_FOREGROUNDED / BROWSER_COLD_START must be enabled);
I don't understand the part about the triggers. Could you elaborate on this?

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:67: 24.0,
8.0,  4.0};
nit: remove one space before "4.0"

[jkrcal, 2017-05-15 11:40:25]

Thanks! PTAL, again.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
File components/ntp_snippets/pref_names.cc (right):

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.cc:16: // For backwards compatibility, we do
not rename
On 2017/05/15 09:35:47, markusheintz_ wrote:
> I guess that comment can be moved to the corresponding prefs below.

Done.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
File components/ntp_snippets/pref_names.h (right):

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.h:25: //  - "Wifi" for situations with
unmetered network connectivity, and
On 2017/05/15 09:35:47, markusheintz_ wrote:
> We don't know whether a WIFI is unmetered.

Expanded to explain better.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.h:26: //  - "Fallback" for situations with
only metered network.
On 2017/05/15 09:35:47, markusheintz_ wrote:
> Same here just the other way around.

ditto

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/pre...
components/ntp_snippets/pref_names.h:28: // Trigger: wake-up of the persistent
scheduler.
On 2017/05/15 09:35:47, markusheintz_ wrote:
> The trigger comments are confusing. What information do you want to convey
here?
> 
> That logic for checking whether this interval has already elapsed are
triggered
> by the mention situation?

ditto

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
(right):

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:36: //   - A
"startup" fetch is performed only at a moment when the user starts /
On 2017/05/15 09:35:47, markusheintz_ wrote:
> Can we mention the FetchInterval names from below in the comment? I guess here
> we reference the STARTUP_* intervals.

Rephrased (I do not want to mention all cases I want to guide through both
dimensions separately.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:38: //  
BROWSER_FOREGROUNDED / BROWSER_COLD_START must be enabled);
On 2017/05/15 09:35:47, markusheintz_ wrote:
> I don't understand the part about the triggers. Could you elaborate on this?

Not needed here, removed.

https://codereview.chromium.org/2875953003/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:67: 24.0,
8.0,  4.0};
On 2017/05/15 09:35:47, markusheintz_ wrote:
> nit: remove one space before "4.0"

Done.

[markusheintz_, 2017-05-16 12:04:57]

LGTM

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

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
components/ntp_snippets/pref_names.h:22: ////////////////// Minimal interval
between two successive background fetches.
Comments should be like this:

// lalalala ...

not 

/////////////////// lalala

Pls fix

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
(right):

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:65: 24.0,
8.0,  4.0};
Please remove the space before te 4.0

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:652:
first_allowed_fetch_time += (wifi ? schedule_.interval_shown_wifi
optional: I'm not the biggest fan of the ternary operator. But that's just me.
Nested ifs are probably not better in terms of readability. And a if ... else if
chain :-/ your call.

[jkrcal, 2017-05-16 12:31:02]

Thanks!

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

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
components/ntp_snippets/pref_names.h:22: ////////////////// Minimal interval
between two successive background fetches.
On 2017/05/16 12:04:57, markusheintz_ wrote:
> Comments should be like this:
> 
> // lalalala ...
> 
> not 
> 
> /////////////////// lalala
> 
> Pls fix

Done.

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
(right):

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:65: 24.0,
8.0,  4.0};
On 2017/05/16 12:04:57, markusheintz_ wrote:
> Please remove the space before te 4.0

Huh, I am surprised that I haven't fixed it last time.

Actually, I have. It is "git cl format" who fixed it back. It aligns the numbers
(and puts extra spaces). Good to learn that.

https://codereview.chromium.org/2875953003/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:652:
first_allowed_fetch_time += (wifi ? schedule_.interval_shown_wifi
On 2017/05/16 12:04:57, markusheintz_ wrote:
> optional: I'm not the biggest fan of the ternary operator. But that's just me.
> Nested ifs are probably not better in terms of readability. And a if ... else
if
> chain :-/ your call.

I'll keep the ternary one. I really cannot see the other options compelling :|

[jkrcal, 2017-05-16 12:31:12]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-05-16 12:31:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
markusheintz@chromium.org
Link to the patchset: https://codereview.chromium.org/2875953003/#ps40001
(title: "Markus's comments #2")

[commit-bot: I haz the power, 2017-05-16 12:31:44]

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, 2017-05-16 13:43:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494937872423870,
"parent_rev": "34de850573c2fdf253b95441bf043fa8c5d5da62", "commit_rev":
"edce193983bdd176ced3394a88fb70935cf133b1"}

[commit-bot: I haz the power, 2017-05-16 13:43:51]

Description was changed from

==========
[Soft fetches] Set-up different intervals for <Chrome started> events.

This CL creates different intervals for startup events. This allows:
 - iOS to compensate the missing persistent scheduler using startup soft
   fetches (setting the same intervals as for persistent fetches).
 - Android to replace most of the persistent fetches by startup soft
   fetches and hopefully save considerable traffic by that (setting
   the same intervals as they are now for persistent fetches and
   considerably enlarging the intervals for persistent fetches).

BUG=721346
==========

to

==========
[Soft fetches] Set-up different intervals for <Chrome started> events.

This CL creates different intervals for startup events. This allows:
 - iOS to compensate the missing persistent scheduler using startup soft
   fetches (setting the same intervals as for persistent fetches).
 - Android to replace most of the persistent fetches by startup soft
   fetches and hopefully save considerable traffic by that (setting
   the same intervals as they are now for persistent fetches and
   considerably enlarging the intervals for persistent fetches).

BUG=721346

Review-Url: https://codereview.chromium.org/2875953003
Cr-Commit-Position: refs/heads/master@{#472088}
Committed:
https://chromium.googlesource.com/chromium/src/+/edce193983bdd176ced3394a88fb...
==========

[commit-bot: I haz the power, 2017-05-16 13:43:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/edce193983bdd176ced3394a88fb...