Allow the minimum time between banner trigger visits to be varied via field trials.

Allow the minimum time between banner trigger visits to be varied via field trials.

The current heuristics for banner triggering are hard-coded. This CL
allows the minimum time between visits that are counted for the purposes
of banner triggering to be varied via field trials. The time will
default to the current value, which is one visit per day (implemented by
bucketing all visits to local midnight time). The new implementation
will bucket to a given resolution in minutes, where the specified
resolution is less than one day and evenly divisible into the number of
minutes in a day.

BUG=487519
Committed: https://crrev.com/b6d06bfc9c3dceb0a8f4ef88838705b8d5334d5a
Cr-Commit-Position: refs/heads/master@{#345228}

[dominickn, 2015-08-24 02:46:26]

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

[dominickn, 2015-08-24 04:28:52]

PTAL, thanks!

[benwells, 2015-08-24 06:55:52]

just some minor things; you shouldn't do the test refactoring mentioned now as
it will make the merge more painful.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:107: void
AppBannerManager::UpdateMinutesBetweenVisits() {
Could you just move all this logic into
AppBannerSettingsHelper::UpdateMinutesBetweenVisits or something? Seems like the
settings helper is the thing which should know about the finch trials.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_settings_helper.cc:113: base::Time
BucketTime(base::Time time, base::TimeDelta resolution) {
Nit: I think it would be simpler if this just took 'unsigned int minutes'
instead of a base::TimeDelta, as you would save a couple of conversions.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_settings_helper.cc:478: return
BucketTime(time, resolution);
Do you need a wrapper function for this? You could just move the anon version
into here and use it...

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_settings_helper_unittest.cc (right):

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_settings_helper_unittest.cc:693:
AppBannerSettingsHelper::SetMinimumMinutesBetweenVisits(1440);
At some point you might want to put all this boilerplate into a common function
or setup method.

[dominickn, 2015-08-24 07:43:16]

Thanks for the feedback.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:107: void
AppBannerManager::UpdateMinutesBetweenVisits() {
On 2015/08/24 06:55:52, benwells wrote:
> Could you just move all this logic into
> AppBannerSettingsHelper::UpdateMinutesBetweenVisits or something? Seems like
the
> settings helper is the thing which should know about the finch trials.

Done.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_settings_helper.cc:113: base::Time
BucketTime(base::Time time, base::TimeDelta resolution) {
On 2015/08/24 06:55:52, benwells wrote:
> Nit: I think it would be simpler if this just took 'unsigned int minutes'
> instead of a base::TimeDelta, as you would save a couple of conversions.

Done.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_settings_helper.cc:478: return
BucketTime(time, resolution);
On 2015/08/24 06:55:52, benwells wrote:
> Do you need a wrapper function for this? You could just move the anon version
> into here and use it...

Done.

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_settings_helper_unittest.cc (right):

https://codereview.chromium.org/1306693005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_settings_helper_unittest.cc:693:
AppBannerSettingsHelper::SetMinimumMinutesBetweenVisits(1440);
On 2015/08/24 06:55:52, benwells wrote:
> At some point you might want to put all this boilerplate into a common
function
> or setup method. 

Will investigate for a separate CL.

[benwells, 2015-08-24 08:01:58]

lgtm if you update the unit tests for your last patch set :)

[dominickn, 2015-08-25 00:23:31]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-08-25 00:23:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1306693005/#ps40001
(title: "Fix unit tests")

[commit-bot: I haz the power, 2015-08-25 00:24:06]

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

[commit-bot: I haz the power, 2015-08-25 00:56:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-25 00:57:08]

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