Allow the site engagement service thresholds to be varied via field trial.

Allow the site engagement service thresholds to be varied via field trial.

This CL allows the maximum points accumulated per day, points
accumulated per navigation and user input, decay period in days, and
decay points to be independently varied and controlled through a field
trial.

BUG=464234
Committed: https://crrev.com/d43830e1ab4a8f814ca841b24343cf3d1e326f99
Cr-Commit-Position: refs/heads/master@{#352750}

[dominickn, 2015-09-28 06:33:19]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org, calamity@chromium.org

[dominickn, 2015-09-28 06:33:30]

PTAL, thanks!

[benwells, 2015-09-29 00:40:31]

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:78: void
SiteEngagementScore::UpdateFromVariations() {
Should something be calling this?

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:37: static const char*
kDecayPointsParam;
Do all these keys (and the content settings keys) need to be public?

[dominickn, 2015-09-29 00:52:30]

Thanks!

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:78: void
SiteEngagementScore::UpdateFromVariations() {
On 2015/09/29 00:40:31, benwells wrote:
> Should something be calling this?

It will once calamity's clean up CL lands. Then I will rebase this CL on that,
calling UpdateFromVariations() in a common init method that will clean up dead
scores, update from variations, and log UMA.

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:37: static const char*
kDecayPointsParam;
On 2015/09/29 00:40:31, benwells wrote:
> Do all these keys (and the content settings keys) need to be public?

@calamity: any thoughts on making all of these private? I believe that they're
accessed a lot in tests.

[benwells, 2015-09-29 01:10:45]

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:78: void
SiteEngagementScore::UpdateFromVariations() {
On 2015/09/29 00:52:30, dominickn wrote:
> On 2015/09/29 00:40:31, benwells wrote:
> > Should something be calling this?
> 
> It will once calamity's clean up CL lands. Then I will rebase this CL on that,
> calling UpdateFromVariations() in a common init method that will clean up dead
> scores, update from variations, and log UMA.

OK. Functions overwriting global variables feel weird, so I'm interested in
seeing where this is called. Ideally it will just be called once.

I'll wait to see how it is called before reviewing...

[dominickn, 2015-10-01 08:27:34]

Ping: now with the call to UpdateFromVariations

[calamity, 2015-10-01 08:41:19]

I'll do a proper review tomorrow. At a glance, this lg.

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:37: static const char*
kDecayPointsParam;
On 2015/09/29 00:52:30, dominickn wrote:
> On 2015/09/29 00:40:31, benwells wrote:
> > Do all these keys (and the content settings keys) need to be public?
> 
> @calamity: any thoughts on making all of these private? I believe that they're
> accessed a lot in tests.

These are just the keys, not the values?

I don't think they're used in tests too much.

I support privatization of this sector.

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:56: static double
gDecayPoints;
nit: g_decay_points and above to match majority of Chrome.

[dominickn, 2015-10-01 09:04:22]

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:37: static const char*
kDecayPointsParam;
On 2015/10/01 08:41:19, calamity wrote:
> On 2015/09/29 00:52:30, dominickn wrote:
> > On 2015/09/29 00:40:31, benwells wrote:
> > > Do all these keys (and the content settings keys) need to be public?
> > 
> > @calamity: any thoughts on making all of these private? I believe that
they're
> > accessed a lot in tests.
> 
> These are just the keys, not the values?
> 
> I don't think they're used in tests too much.
> 
> I support privatization of this sector.

Done.

[benwells, 2015-10-02 01:37:43]

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:336:
SiteEngagementScore::UpdateFromVariations();
I think this is the wrong place for this, as there will be some delay between
the service being created and the startup task happening.

You should probably do it in the constructor instead.

I think it would also be good to ensure it only happens once, maybe with another
global variable. Doing it multiple times (i.e. for every profile) is probably ok
but it just feels like something waiting to go wrong. (yes I know the banner
stuff does it like this but that made me a bit uneasy too).

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.h:79: static const char*
kEngagementParams;
Nit: are these needed in tests? If not they should be in the anon namespace.

[dominickn, 2015-10-02 06:29:06]

Thanks!

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:336:
SiteEngagementScore::UpdateFromVariations();
On 2015/10/02 01:37:42, benwells wrote:
> I think this is the wrong place for this, as there will be some delay between
> the service being created and the startup task happening.
> 
> You should probably do it in the constructor instead.
> 
> I think it would also be good to ensure it only happens once, maybe with
another
> global variable. Doing it multiple times (i.e. for every profile) is probably
ok
> but it just feels like something waiting to go wrong. (yes I know the banner
> stuff does it like this but that made me a bit uneasy too).

Done.

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.h:79: static const char*
kEngagementParams;
On 2015/10/02 01:37:43, benwells wrote:
> Nit: are these needed in tests? If not they should be in the anon namespace.

Done.

[benwells, 2015-10-06 02:55:50]

lgtm

https://codereview.chromium.org/1373453002/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:32: const char
kEngagementParams[] = "SiteEngagementParams";
Nit: can we just call this SiteEngagement?

[calamity, 2015-10-06 03:52:41]

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:56: static double
gDecayPoints;
On 2015/10/01 08:41:19, calamity wrote:
> nit: g_decay_points and above to match majority of Chrome.

Ping?

You can do this quickly by running

sed -i 's/gDecayPoints/g_decay_points/g' `git diff --name-only @{u}`

etc

[dominickn, 2015-10-06 05:00:39]

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_service.h (right):

https://codereview.chromium.org/1373453002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.h:56: static double
gDecayPoints;
On 2015/10/06 03:52:40, calamity wrote:
> On 2015/10/01 08:41:19, calamity wrote:
> > nit: g_decay_points and above to match majority of Chrome.
> 
> Ping?
> 
> You can do this quickly by running
> 
> sed -i 's/gDecayPoints/g_decay_points/g' `git diff --name-only @{u}`
> 
> etc

Done.

https://codereview.chromium.org/1373453002/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:32: const char
kEngagementParams[] = "SiteEngagementParams";
On 2015/10/06 02:55:50, benwells wrote:
> Nit: can we just call this SiteEngagement?

Done.

[calamity, 2015-10-07 00:27:23]

lgtm

https://codereview.chromium.org/1373453002/diff/120001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/120001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_service.cc:269: if
(!gUpdatedFromVariations) {
nit: One more here.

[dominickn, 2015-10-07 01:40:12]

https://codereview.chromium.org/1373453002/diff/120001/chrome/browser/engagem...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/1373453002/diff/120001/chrome/browser/engagem...
chrome/browser/engagement/site_engagement_service.cc:269: if
(!gUpdatedFromVariations) {
On 2015/10/07 00:27:23, calamity wrote:
> nit: One more here.

Done.

[dominickn, 2015-10-07 02:14:39]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-10-07 02:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/1373453002/#ps140001
(title: "Reviewer nits")

[commit-bot: I haz the power, 2015-10-07 02:17:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373453002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373453002/140001

[commit-bot: I haz the power, 2015-10-07 03:02:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-07 03:03:08]

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