[Background fetching] Configure and report fetching triggers

[Background fetching] Configure and report fetching triggers

This CL enables configuring which trigger types result in a background
fetch. The default configuration can be overridden by a variation
parameter. It also reports the trigger type of each fetch to UMA.

Additionally, this CL enables two additional trigger types that were
previously inactive (Chrome cold start / Chrome foregrounding).

BUG=672479
Review-Url: https://codereview.chromium.org/2626433005
Cr-Commit-Position: refs/heads/master@{#443272}
Committed: https://chromium.googlesource.com/chromium/src/+/ffa982507bece3662d3b48dd73f8b1c1b85999f2

[jkrcal, 2017-01-10 10:17:14]

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

[jkrcal, 2017-01-10 10:17:15]

Tim, could you PTAL?

(no unit-tests yet, I want quick feedback)

[tschumann, 2017-01-10 14:28:16]

overall looks good -- just some comments on implementation details.

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

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:214: if
(!ShouldRefetchInTheBackgroundNow()) {
this is asymmetric to the other triggers. Shouldn't they check this as well?

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:485:
DLOG(WARNING) << "Invalid default value for param " << kTriggerTypesParamName;
this should be a DCHECK(), no?
Actually, if you're going to hard-code a default, why not just hard-code the
enum values?

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:494:
"Fill in names for trigger types.");
hmm... i wonder if it wouldn't be cleaner to have a function:
TriggerTypeToConfigString() (or the like). That way there wouldn't be a chance
for getting out of sync as you could have a nice switch.

you could put the tokens in a set and simply go through the enum values and look
up their string-representations.

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:504:
out->clear();
this seems a bit brittle. Honestly, I wouldn't worry about the state of 'out' in
case the function returns 'false'. It's a common contract to leave output
parameters unspecified in case of errors.

If you insist, then I'd vote for clearing it at the beginning of the function.

[tschumann, 2017-01-10 14:29:04]

https://codereview.chromium.org/2626433005/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2626433005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:77625: +  <int value="2" label="Cold
start of the browser"/>
isn't there one missing for NTP-open?

[jkrcal, 2017-01-10 18:52:27]

Thanks! PTAL, again.

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

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:214: if
(!ShouldRefetchInTheBackgroundNow()) {
On 2017/01/10 14:28:16, tschumann wrote:
> this is asymmetric to the other triggers. Shouldn't they check this as well?

Oops, sure. Thanks!

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:485:
DLOG(WARNING) << "Invalid default value for param " << kTriggerTypesParamName;
On 2017/01/10 14:28:16, tschumann wrote:
> this should be a DCHECK(), no?
> Actually, if you're going to hard-code a default, why not just hard-code the
> enum values?

Done.

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:494:
"Fill in names for trigger types.");
On 2017/01/10 14:28:16, tschumann wrote:
> hmm... i wonder if it wouldn't be cleaner to have a function:
> TriggerTypeToConfigString() (or the like). That way there wouldn't be a chance
> for getting out of sync as you could have a nice switch.
> 
> you could put the tokens in a set and simply go through the enum values and
look
> up their string-representations.

To me, it seems that I would then need to iterate over enum values. Is there a
clean way to do so? Otherwise, did you mean it differently?

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:504:
out->clear();
On 2017/01/10 14:28:16, tschumann wrote:
> this seems a bit brittle. Honestly, I wouldn't worry about the state of 'out'
in
> case the function returns 'false'. It's a common contract to leave output
> parameters unspecified in case of errors.
> 
> If you insist, then I'd vote for clearing it at the beginning of the function.

Done.

https://codereview.chromium.org/2626433005/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2626433005/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:77625: +  <int value="2" label="Cold
start of the browser"/>
On 2017/01/10 14:29:04, tschumann wrote:
> isn't there one missing for NTP-open?

Done.

[tschumann, 2017-01-12 12:44:20]

lgtm

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

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:494:
"Fill in names for trigger types.");
On 2017/01/10 18:52:27, jkrcal wrote:
> On 2017/01/10 14:28:16, tschumann wrote:
> > hmm... i wonder if it wouldn't be cleaner to have a function:
> > TriggerTypeToConfigString() (or the like). That way there wouldn't be a
chance
> > for getting out of sync as you could have a nice switch.
> > 
> > you could put the tokens in a set and simply go through the enum values and
> look
> > up their string-representations.
> 
> To me, it seems that I would then need to iterate over enum values. Is there a
> clean way to do so? Otherwise, did you mean it differently?

no, there's no way I know of to iterate over them.
the nice thing about an explicit mapping function is that mix-ups in values get
very visible. On the other hand, unit test coverage should also get this
ensured.

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

https://codereview.chromium.org/2626433005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:75:
const char* kTriggerTypesParamValueForEmptyList = "-";
would be curious to understand why the empty string wouldn't work here. 
Is there a way to define a default value for the variation parameter so that we
can use the empty string to signal no triggers?

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

https://codereview.chromium.org/2626433005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:312:
// First enable the scheduler.
would it make sense to only enable that trigger making sure we don't mix up
names in the variation mapping? (same for the other triggers below).

[jkrcal, 2017-01-12 15:35:28]

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

https://codereview.chromium.org/2626433005/diff/1/components/ntp_snippets/rem...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:494:
"Fill in names for trigger types.");
On 2017/01/12 12:44:20, tschumann wrote:
> On 2017/01/10 18:52:27, jkrcal wrote:
> > On 2017/01/10 14:28:16, tschumann wrote:
> > > hmm... i wonder if it wouldn't be cleaner to have a function:
> > > TriggerTypeToConfigString() (or the like). That way there wouldn't be a
> chance
> > > for getting out of sync as you could have a nice switch.
> > > 
> > > you could put the tokens in a set and simply go through the enum values
and
> > look
> > > up their string-representations.
> > 
> > To me, it seems that I would then need to iterate over enum values. Is there
a
> > clean way to do so? Otherwise, did you mean it differently?
> 
> no, there's no way I know of to iterate over them.
> the nice thing about an explicit mapping function is that mix-ups in values
get
> very visible. On the other hand, unit test coverage should also get this
> ensured.

ok, I adapted the unit-tests as you proposed.

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

https://codereview.chromium.org/2626433005/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:75:
const char* kTriggerTypesParamValueForEmptyList = "-";
On 2017/01/12 12:44:20, tschumann wrote:
> would be curious to understand why the empty string wouldn't work here. 
> Is there a way to define a default value for the variation parameter so that
we
> can use the empty string to signal no triggers?

No, there is no way. This is something to fix, in my opinion, in the long-term.

I think variation parameters should be declared in the code (like Features
currently) and should have default values. I plan to talk about it with Alexei
when I get to MTV, next time.

Currently, when I get an empty value, it can mean both "empty value explicitly
provided" and "no value provided". Hence this ugly "-".

[jkrcal, 2017-01-12 15:35:33]

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

[commit-bot: I haz the power, 2017-01-12 15:36:05]

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

[jkrcal, 2017-01-12 15:36:10]

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

[jkrcal, 2017-01-12 15:36:11]

Robert, could you PTAL at histograms.xml?

[commit-bot: I haz the power, 2017-01-12 16:37:54]

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

[commit-bot: I haz the power, 2017-01-12 16:37:55]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-01-12 16:49:41]

lgtm

[jkrcal, 2017-01-12 17:05:25]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-01-12 17:05:25]

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

[commit-bot: I haz the power, 2017-01-12 17:06:16]

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-01-12 17:13:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484240725201270,
"parent_rev": "1b273edcb44ef69ce19b93a73622460de70fe14d", "commit_rev":
"ffa982507bece3662d3b48dd73f8b1c1b85999f2"}

[commit-bot: I haz the power, 2017-01-12 17:14:31]

Description was changed from

==========
[Background fetching] Configure and report fetching triggers

This CL enables configuring which trigger types result in a background
fetch. The default configuration can be overridden by a variation
parameter. It also reports the trigger type of each fetch to UMA.

Additionally, this CL enables two additional trigger types that were
previously inactive (Chrome cold start / Chrome foregrounding).

BUG=672479
==========

to

==========
[Background fetching] Configure and report fetching triggers

This CL enables configuring which trigger types result in a background
fetch. The default configuration can be overridden by a variation
parameter. It also reports the trigger type of each fetch to UMA.

Additionally, this CL enables two additional trigger types that were
previously inactive (Chrome cold start / Chrome foregrounding).

BUG=672479

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

[commit-bot: I haz the power, 2017-01-12 17:14:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ffa982507bece3662d3b48dd73f8...