Use heuristic to work out when to prompt for app install banners.

Use heuristic to work out when to prompt for app install banners.

The heuristic ensures that the user is not prompted too much, and is only
prompted after showing sustained engagement with a site.

BUG=452825
Committed: https://crrev.com/4a1448294ae0d52682a3d5f1d10a4448367020eb
Cr-Commit-Position: refs/heads/master@{#315055}

[benwells, 2015-02-05 19:58:36]

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

[gone, 2015-02-05 22:21:10]

lgtm with nits

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/an...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/an...
chrome/browser/android/banners/app_banner_manager.cc:201:
AppBannerSettingsHelper::APP_BANNER_EVENT_DID_SHOW, base::Time::Now());
Seems strange to record that we did show it in a function called
CheckIfShouldShow().  ShowIfPossible()?

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
chrome/browser/banners/app_banner_settings_helper.cc:31: const unsigned int
kMinimumBannerShownToBannerShown = 60;
kMinimumDaysBetweenBannerShows?

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
File chrome/browser/banners/app_banner_settings_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
chrome/browser/banners/app_banner_settings_helper_unittest.cc:153:
TEST_F(AppBannerSettingsHelperTest, ShouldShow) {
Maybe split into multiple tests?

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
chrome/browser/banners/app_banner_settings_helper_unittest.cc:167: // Visit the
site one, it still should not be shown.
one year later?

[gone, 2015-02-05 23:13:50]

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://chromiumcodereview.appspot.com/886643003/diff/20001/chrome/browser/ba...
chrome/browser/banners/app_banner_settings_helper.cc:35: const unsigned int
kMinimumBannerBlockedToBannerShown = 180;
This is 6 months, right?  I think the doc said 3?

[benwells, 2015-02-06 14:48:16]

New patchsets have been uploaded after l-g-t-m from dfalcantara@chromium.org

[benwells, 2015-02-06 16:36:29]

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_manager.cc:201:
AppBannerSettingsHelper::APP_BANNER_EVENT_DID_SHOW, base::Time::Now());
On 2015/02/05 22:21:10, dfalcantara wrote:
> Seems strange to record that we did show it in a function called
> CheckIfShouldShow().  ShowIfPossible()?

Good point. I've split this into two functions: RecordCouldShowBanner and
CheckIfShouldShow.

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/banners/a...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.cc:31: const unsigned int
kMinimumBannerShownToBannerShown = 60;
On 2015/02/05 22:21:10, dfalcantara wrote:
> kMinimumDaysBetweenBannerShows?

Done.

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.cc:35: const unsigned int
kMinimumBannerBlockedToBannerShown = 180;
On 2015/02/05 23:13:50, dfalcantara wrote:
> This is 6 months, right?  I think the doc said 3?

Done.

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/banners/a...
File chrome/browser/banners/app_banner_settings_helper_unittest.cc (right):

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper_unittest.cc:153:
TEST_F(AppBannerSettingsHelperTest, ShouldShow) {
On 2015/02/05 22:21:10, dfalcantara wrote:
> Maybe split into multiple tests?

Done.

https://codereview.chromium.org/886643003/diff/20001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper_unittest.cc:167: // Visit the
site one, it still should not be shown.
On 2015/02/05 22:21:10, dfalcantara wrote:
> one year later?

Something like that. Fixed.

[benwells, 2015-02-06 16:36:36]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2015-02-06 16:37:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886643003/80001

[commit-bot: I haz the power, 2015-02-06 17:28:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-06 17:29:12]

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