[Remote suggestions] Prioritize wifi for soft fetches.

[Remote suggestions] Prioritize wifi for soft fetches.

This CL allows to set different intervals for soft fetches for wifi and
fallback connections. This puts soft fetches on par to persistent
fetches with respect to parameters.

Previously we prioritized soft fetches triggered by NTP_OPENED. This
feature is removed by this CL for the sake of simplicity (NTP_OPENED
is the only trigger that we employ on M57 Stable).

BUG=708146
Review-Url: https://codereview.chromium.org/2794313002
Cr-Commit-Position: refs/heads/master@{#462937}
Committed: https://chromium.googlesource.com/chromium/src/+/2f06179869a269f69f4e10505afa82edd2a6fe81

[jkrcal, 2017-04-04 13:14:49]

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

[jkrcal, 2017-04-04 13:14:49]

Marc, could you PTAL?

[jkrcal, 2017-04-04 13:16:07]

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

[commit-bot: I haz the power, 2017-04-04 13:16:23]

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

[Marc Treib, 2017-04-04 13:28:16]

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:65:
"soft_on_ntp_opened_interval_hours-rare_ntp_user"};
Removing the existing params means we'll have to update the Finch configs at the
correct time. Recent experience shows that we're bad at this. Do we have any
plans in place to make sure we don't miss it this time?

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:55: const
double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 4.0,
Any particular reason for this change?

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:105:
"ntp_snippets.soft_fetching_interval_on_ntp_opened";
I recently learned of a good place for that kind of thing:
https://cs.chromium.org/chromium/src/chrome/browser/prefs/browser_prefs.cc?rc...

(optional; it's also fine to keep it here)

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:144: //
Return true for Wi-Fi (and any other likely unmetered connection type).
Hm yeah, "wifi" is really a misnomer, also for the persistent scheduler. Hrm.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:259: //
Cleanup procedure in M59. Remove for M60.
I'd leave it for a bit longer, maybe M62?

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:340: if
(!ShouldRefetchInTheBackgroundNow()) {
Doesn't this mean that all triggers will be treated the same now, i.e. a change
in behavior from what we have on stable now?

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:476: if
(IsOnWifi()) {
nit: IMO this would be simpler as an arithmetic if:
first_allowed_fetch_time = last_fetch_attempt_time + (IsOnWifi() ?
schedule_.interval_soft_wifi : schedule_.interval_soft_fallback);

Or, if you find that line too long:
first_allowed_fetch_time = last_fetch_attempt_time;
first_allowed_fetch_time += IsOnWifi() ? schedule_.interval_soft_wifi :
schedule_.interval_soft_fallback;

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:101:
class FakeNetworkChangeNotifierWIFI : public net::NetworkChangeNotifier {
s/WIFI/Wifi/ ?

optional: Merge the two classes and pass in the type as a ctor param?

[commit-bot: I haz the power, 2017-04-04 13:59:19]

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

[commit-bot: I haz the power, 2017-04-04 13:59:20]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-04-05 17:32:57]

Thanks, Marc!

PTAL, again.

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:65:
"soft_on_ntp_opened_interval_hours-rare_ntp_user"};
On 2017/04/04 13:28:15, Marc Treib wrote:
> Removing the existing params means we'll have to update the Finch configs at
the
> correct time. Recent experience shows that we're bad at this. Do we have any
> plans in place to make sure we don't miss it this time?

I'll update the current finch configs asap and create a new Beta config for M59
to reflect this change.

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:55: const
double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 4.0,
On 2017/04/04 13:28:16, Marc Treib wrote:
> Any particular reason for this change?

Yes, for two reasons:
 - I would like to be cautious about data we use for our users in EM (and in
general);
 - We probably need to turn down the traffic a bit. I will adapt the defaults
once again, close to BP to reflect the situation in M57 Stable.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:105:
"ntp_snippets.soft_fetching_interval_on_ntp_opened";
On 2017/04/04 13:28:15, Marc Treib wrote:
> I recently learned of a good place for that kind of thing:
>
https://cs.chromium.org/chromium/src/chrome/browser/prefs/browser_prefs.cc?rc...
> 
> (optional; it's also fine to keep it here)

Ah, interesting, thanks for the pointer!
I'd keep it here for this time (so that I do not need lgtm from a browser
OWNER)...

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:144: //
Return true for Wi-Fi (and any other likely unmetered connection type).
On 2017/04/04 13:28:16, Marc Treib wrote:
> Hm yeah, "wifi" is really a misnomer, also for the persistent scheduler. Hrm.

Yeah, I've updated the comments to explain what we mean by wifi.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:259: //
Cleanup procedure in M59. Remove for M60.
On 2017/04/04 13:28:16, Marc Treib wrote:
> I'd leave it for a bit longer, maybe M62?

Done.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:340: if
(!ShouldRefetchInTheBackgroundNow()) {
On 2017/04/04 13:28:16, Marc Treib wrote:
> Doesn't this mean that all triggers will be treated the same now, i.e. a
change
> in behavior from what we have on stable now?

I've rewritten the code. Hope the structure is now clearer.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:476: if
(IsOnWifi()) {
On 2017/04/04 13:28:15, Marc Treib wrote:
> nit: IMO this would be simpler as an arithmetic if:
> first_allowed_fetch_time = last_fetch_attempt_time + (IsOnWifi() ?
> schedule_.interval_soft_wifi : schedule_.interval_soft_fallback);
> 
> Or, if you find that line too long:
> first_allowed_fetch_time = last_fetch_attempt_time;
> first_allowed_fetch_time += IsOnWifi() ? schedule_.interval_soft_wifi :
> schedule_.interval_soft_fallback;

Done.

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:101:
class FakeNetworkChangeNotifierWIFI : public net::NetworkChangeNotifier {
On 2017/04/04 13:28:16, Marc Treib wrote:
> s/WIFI/Wifi/ ?
> 
> optional: Merge the two classes and pass in the type as a ctor param?

Not relevant any more.

[Marc Treib, 2017-04-06 08:29:23]

lgtm

Thanks!

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:55: const
double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 4.0,
On 2017/04/05 17:32:56, jkrcal wrote:
> On 2017/04/04 13:28:16, Marc Treib wrote:
> > Any particular reason for this change?
> 
> Yes, for two reasons:
>  - I would like to be cautious about data we use for our users in EM (and in
> general);
>  - We probably need to turn down the traffic a bit. I will adapt the defaults
> once again, close to BP to reflect the situation in M57 Stable.

Then, arguably, all the changes to the defaults should happen in that follow-up
CL. But I guess it doesn't really matter.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:105:
"ntp_snippets.soft_fetching_interval_on_ntp_opened";
On 2017/04/05 17:32:56, jkrcal wrote:
> On 2017/04/04 13:28:15, Marc Treib wrote:
> > I recently learned of a good place for that kind of thing:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/prefs/browser_prefs.cc?rc...
> > 
> > (optional; it's also fine to keep it here)
> 
> Ah, interesting, thanks for the pointer!
> I'd keep it here for this time (so that I do not need lgtm from a browser
> OWNER)...

FWIW, bauerb@ and battre@ are both owners, so no timezone roundtrips required.
(But it's still fine to keep this here.)

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:340: if
(!ShouldRefetchInTheBackgroundNow()) {
On 2017/04/05 17:32:56, jkrcal wrote:
> On 2017/04/04 13:28:16, Marc Treib wrote:
> > Doesn't this mean that all triggers will be treated the same now, i.e. a
> change
> > in behavior from what we have on stable now?
> 
> I've rewritten the code. Hope the structure is now clearer.

Yup, much better, thanks!

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

https://codereview.chromium.org/2794313002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:38: // 
enabled (a "persistent" fetch is initiated automatically by the OS after the
nitty nit: Should the part in parens be its own bullet?

https://codereview.chromium.org/2794313002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2794313002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:634:
.WillRepeatedly(Return(true));
Is this necessary, since it's already set in the ctor?
I guess you want to be explicit for this test?

[jkrcal, 2017-04-06 12:04:08]

Thanks!

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

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:55: const
double kDefaultFetchingIntervalHoursActiveNtpUser[] = {24.0, 6.0, 4.0,
On 2017/04/06 08:29:23, Marc Treib wrote:
> On 2017/04/05 17:32:56, jkrcal wrote:
> > On 2017/04/04 13:28:16, Marc Treib wrote:
> > > Any particular reason for this change?
> > 
> > Yes, for two reasons:
> >  - I would like to be cautious about data we use for our users in EM (and in
> > general);
> >  - We probably need to turn down the traffic a bit. I will adapt the
defaults
> > once again, close to BP to reflect the situation in M57 Stable.
> 
> Then, arguably, all the changes to the defaults should happen in that
follow-up
> CL. But I guess it doesn't really matter.

Acknowledged.

https://codereview.chromium.org/2794313002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:105:
"ntp_snippets.soft_fetching_interval_on_ntp_opened";
On 2017/04/06 08:29:23, Marc Treib wrote:
> On 2017/04/05 17:32:56, jkrcal wrote:
> > On 2017/04/04 13:28:15, Marc Treib wrote:
> > > I recently learned of a good place for that kind of thing:
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/prefs/browser_prefs.cc?rc...
> > > 
> > > (optional; it's also fine to keep it here)
> > 
> > Ah, interesting, thanks for the pointer!
> > I'd keep it here for this time (so that I do not need lgtm from a browser
> > OWNER)...
> 
> FWIW, bauerb@ and battre@ are both owners, so no timezone roundtrips required.
> (But it's still fine to keep this here.)

Acknowledged.

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

https://codereview.chromium.org/2794313002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:38: // 
enabled (a "persistent" fetch is initiated automatically by the OS after the
On 2017/04/06 08:29:23, Marc Treib wrote:
> nitty nit: Should the part in parens be its own bullet?

Done.

https://codereview.chromium.org/2794313002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2794313002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:634:
.WillRepeatedly(Return(true));
On 2017/04/06 08:29:23, Marc Treib wrote:
> Is this necessary, since it's already set in the ctor?
> I guess you want to be explicit for this test?

Yep, updated the comment to make it clear.

[jkrcal, 2017-04-06 12:04:13]

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

[commit-bot: I haz the power, 2017-04-06 12:04:27]

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-04-06 14:14:40]

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

[commit-bot: I haz the power, 2017-04-06 14:14:42]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-04-06 14:15:27]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-04-06 14:15:27]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2794313002/#ps60001
(title: "Marc's nits")

[commit-bot: I haz the power, 2017-04-06 14:15:55]

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-04-06 14:22:49]

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

[commit-bot: I haz the power, 2017-04-06 14:22:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jkrcal, 2017-04-06 15:07:41]

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

[jkrcal, 2017-04-06 15:07:41]

Peter, could you PTAL at .java?

[jkrcal, 2017-04-07 15:38:35]

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

[commit-bot: I haz the power, 2017-04-07 15:39:12]

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-04-07 16:03:12]

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

[commit-bot: I haz the power, 2017-04-07 16:03:13]

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...)

[Michael van Ouwerkerk, 2017-04-07 16:11:10]

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

[Michael van Ouwerkerk, 2017-04-07 16:11:11]

lgtm

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

https://codereview.chromium.org/2794313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:171:
public boolean isOnUnmeteredConnection() {
This functionality looks fine, but please ensure this does what you want to do.
There's a class called DeviceState with a method isUnmeteredNetworkAvailable
which checks a bunch of other things as well.

[jkrcal, 2017-04-07 16:26:19]

Thanks, Michael!

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

https://codereview.chromium.org/2794313002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:171:
public boolean isOnUnmeteredConnection() {
On 2017/04/07 16:11:11, Michael van Ouwerkerk wrote:
> This functionality looks fine, but please ensure this does what you want to
do.
> There's a class called DeviceState with a method isUnmeteredNetworkAvailable
> which checks a bunch of other things as well.

I believe that ConnectivityManager does what we need (and I do not want to
introduce a new dependency).

[jkrcal, 2017-04-07 16:36:13]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-04-07 16:36:13]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2794313002/#ps100001
(title: "Fix an error in rebase")

[commit-bot: I haz the power, 2017-04-07 16:36:40]

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-04-07 18:37:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491582973218600,
"parent_rev": "d821806d4441025512588c514e66756323eb1408", "commit_rev":
"2f06179869a269f69f4e10505afa82edd2a6fe81"}

[commit-bot: I haz the power, 2017-04-07 18:38:06]

Description was changed from

==========
[Remote suggestions] Prioritize wifi for soft fetches.

This CL allows to set different intervals for soft fetches for wifi and
fallback connections. This puts soft fetches on par to persistent
fetches with respect to parameters.

Previously we prioritized soft fetches triggered by NTP_OPENED. This
feature is removed by this CL for the sake of simplicity (NTP_OPENED
is the only trigger that we employ on M57 Stable).

BUG=708146
==========

to

==========
[Remote suggestions] Prioritize wifi for soft fetches.

This CL allows to set different intervals for soft fetches for wifi and
fallback connections. This puts soft fetches on par to persistent
fetches with respect to parameters.

Previously we prioritized soft fetches triggered by NTP_OPENED. This
feature is removed by this CL for the sake of simplicity (NTP_OPENED
is the only trigger that we employ on M57 Stable).

BUG=708146

Review-Url: https://codereview.chromium.org/2794313002
Cr-Commit-Position: refs/heads/master@{#462937}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f06179869a269f69f4e10505afa...
==========

[commit-bot: I haz the power, 2017-04-07 18:38:07]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2f06179869a269f69f4e10505afa...