Prevent site engagement scores from decaying when Chrome isn't in use.

Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occurred. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to some number of decay periods prior to now
(currently set at 1 decay period), based on their offset to the last known
engagement time.

BUG=575504
Committed: https://crrev.com/709246a0f3afee77f19d056a6306d2f63b613fad
Cr-Commit-Position: refs/heads/master@{#402653}

[dominickn, 2016-06-21 11:00:11]

Description was changed from

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occured. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to the current time (based on their offset to
the last known engagement time).

BUG=575504
==========

to

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occured. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to the current time (based on their offset to
the last known engagement time).

BUG=575504
==========

[dominickn, 2016-06-21 11:00:11]

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

[dominickn, 2016-06-22 00:53:01]

PTAL, thanks!

[calamity, 2016-06-23 05:18:41]

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

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_score.h:71:
OBSOLETE_LAST_ENGAGEMENT_PERIOD_IN_HOURS,
This name is weird. It makes it look like this is an obsolete variation.

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

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:276: void
SiteEngagementService::CleanupEngagementScores(
It might be worth renaming this method now since it does 2 things.

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:300:
score.set_last_engagement_time(now - offset);
I think that all scores should be rebased to the obsoletion time limit. This
will mean that all sites take a hit equal to the obsoletion time limit which
seems more continuous than '9 days, lose your engagement' but '10 days, keep it
all'.

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:90:
registry->RegisterInt64Pref(prefs::kSiteEngagementLastUpdateTime, 0);
This should be a lossy pref.

[dominickn, 2016-06-24 08:45:45]

Description was changed from

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occured. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to the current time (based on their offset to
the last known engagement time).

BUG=575504
==========

to

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occurred. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to some number of decay periods prior to now
(currently set at 1 decay period), based on their offset to the last known
engagement time.

BUG=575504
==========

[dominickn, 2016-06-27 02:53:35]

PTAL, thanks! Now with a different rebase date.

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

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_score.h:71:
OBSOLETE_LAST_ENGAGEMENT_PERIOD_IN_HOURS,
On 2016/06/23 05:18:40, calamity wrote:
> This name is weird. It makes it look like this is an obsolete variation.

Done.

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

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:276: void
SiteEngagementService::CleanupEngagementScores(
On 2016/06/23 05:18:40, calamity wrote:
> It might be worth renaming this method now since it does 2 things.

I think Cleanup is still the right name for the method - rebasing times is part
of cleaning up.

https://codereview.chromium.org/2082953002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_service.cc:300:
score.set_last_engagement_time(now - offset);
On 2016/06/23 05:18:40, calamity wrote:
> I think that all scores should be rebased to the obsoletion time limit. This
> will mean that all sites take a hit equal to the obsoletion time limit which
> seems more continuous than '9 days, lose your engagement' but '10 days, keep
it
> all'.

Done.

[calamity, 2016-06-28 01:48:26]

lgtm.

I'm worried that the tests are getting too arcane. There's random stuff littered
about to work around different parts of the system. It's probably best to
decouple the system components from each other when testing by setting the
testing values of new features to no-ops and then having meaningful values in
the tests for each feature.

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

https://codereview.chromium.org/2082953002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:412:
service->AddPoints(url1, 1.0);
Why did this appear?

[dominickn, 2016-06-28 01:54:49]

Agreed on the tests becoming more arcane, I'll file a PE bug on test improvement
that we should aim to get to next quarter.

That being said, I also think it's important to test the features interacting
with one another as well as in isolation.

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

https://codereview.chromium.org/2082953002/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:412:
service->AddPoints(url1, 1.0);
On 2016/06/28 01:48:25, calamity wrote:
> Why did this appear?

We trigger a cleanup otherwise (> 10 days from last engagement).

[dominickn, 2016-06-28 02:46:14]

dominickn@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[dominickn, 2016-06-28 02:46:14]

+anthonyvd for profile.cc. PTAL, thanks!

[anthonyvd, 2016-06-28 14:51:39]

profile.cc lgtm

[dominickn, 2016-06-28 23:33:52]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-06-28 23:35:23]

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-06-28 23:43:56]

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

[commit-bot: I haz the power, 2016-06-28 23:43:57]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dominickn, 2016-06-29 00:23:41]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-06-29 00:23:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2082953002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-29 00:24:48]

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-06-29 02:09:52]

Description was changed from

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occurred. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to some number of decay periods prior to now
(currently set at 1 decay period), based on their offset to the last known
engagement time.

BUG=575504
==========

to

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occurred. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to some number of decay periods prior to now
(currently set at 1 decay period), based on their offset to the last known
engagement time.

BUG=575504
==========

[commit-bot: I haz the power, 2016-06-29 02:09:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 02:11:28]

Description was changed from

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occurred. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to some number of decay periods prior to now
(currently set at 1 decay period), based on their offset to the last known
engagement time.

BUG=575504
==========

to

==========
Prevent site engagement scores from decaying when Chrome isn't in use.

The site engagement service decays origins which have not been accessed
in a week. However, if a user isn't using Chrome at all, the decay still
occurs.

This CL addresses this issue by persisting the last recorded time that
any engagement occurred. If too long a period passes (currently 10 days)
without *any* engagement, all scores have their last accessed time
adjusted to be relative to some number of decay periods prior to now
(currently set at 1 decay period), based on their offset to the last known
engagement time.

BUG=575504

Committed: https://crrev.com/709246a0f3afee77f19d056a6306d2f63b613fad
Cr-Commit-Position: refs/heads/master@{#402653}
==========

[commit-bot: I haz the power, 2016-06-29 02:11:29]

Patchset 4 (id:??) landed as
https://crrev.com/709246a0f3afee77f19d056a6306d2f63b613fad
Cr-Commit-Position: refs/heads/master@{#402653}