Remove the app banner navigation heuristic.

Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073
Committed: https://crrev.com/598df313ae463915342386163972df774453c570
Cr-Commit-Position: refs/heads/master@{#438452}

[dominickn, 2016-12-07 10:02:22]

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

[commit-bot: I haz the power, 2016-12-07 10:02:42]

Dry run: 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-12-07 11:45:05]

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

[commit-bot: I haz the power, 2016-12-07 11:45:05]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-12-07 11:58:22]

Description was changed from

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replace with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073
==========

to

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replace with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073
==========

[dominickn, 2016-12-07 11:58:22]

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

[dominickn, 2016-12-07 12:00:16]

benwells: PTAL at chrome/browser/banners, chrome/common, and the test files

dfalcantara: PTAL at Android code

Thanks! This is the follow-up to crrev.com/2553013002. -1000 lines (mostly
outdated tests)!

[pkotwicz, 2016-12-07 14:48:14]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2016-12-07 14:48:15]

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

https://codereview.chromium.org/2553013004/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:375: last_transition_type_ =
handle->GetPageTransition();
Drive by: It looks like |last_transition_type_| can now be removed?

[dominickn, 2016-12-08 02:10:31]

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

[commit-bot: I haz the power, 2016-12-08 02:10:59]

Dry run: 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-12-08 02:14:44]

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

[commit-bot: I haz the power, 2016-12-08 02:14:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dominickn, 2016-12-08 02:33:22]

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

[commit-bot: I haz the power, 2016-12-08 02:33:56]

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

[dominickn, 2016-12-08 02:41:50]

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

https://codereview.chromium.org/2553013004/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:375: last_transition_type_ =
handle->GetPageTransition();
On 2016/12/07 14:48:15, pkotwicz wrote:
> Drive by: It looks like |last_transition_type_| can now be removed?

Nice catch, done.

[commit-bot: I haz the power, 2016-12-08 03:56:16]

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

[commit-bot: I haz the power, 2016-12-08 03:56:17]

Dry run: This issue passed the CQ dry run.

[gone, 2016-12-08 19:51:21]

lgtm

https://codereview.chromium.org/2553013004/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/2553013004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:424:
@RetryOnFailure
Don't know if new tests are supposed to automatically add RetryOnFailure.

[benwells, 2016-12-09 04:55:05]

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager_browsertest.cc (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:27: // to changes in SW
code.
lolz

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:165: web_contents,
web_contents->GetLastCommittedURL(),
Comment: It would probably have been wiser to put these changes in a seperate
change, as I think they are unrelated.

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:56:
APP_BANNER_EVENT_COULD_SHOW,
Should this be renamed to something like "FIRST_COULD_SHOW"?

[dominickn, 2016-12-09 05:27:28]

Thanks!

https://codereview.chromium.org/2553013004/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/2553013004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:424:
@RetryOnFailure
On 2016/12/08 19:51:21, dfalcantara (check my queue) wrote:
> Don't know if new tests are supposed to automatically add RetryOnFailure.

Removed.

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager_browsertest.cc (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:27: // to changes in SW
code.
On 2016/12/09 04:55:05, benwells wrote:
> lolz

Acknowledged. :)

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:165: web_contents,
web_contents->GetLastCommittedURL(),
On 2016/12/09 04:55:05, benwells wrote:
> Comment: It would probably have been wiser to put these changes in a seperate
> change, as I think they are unrelated.

Acknowledged - I was doing so much surgery here that these changes got rolled
in. I can remove them if you like.

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:56:
APP_BANNER_EVENT_COULD_SHOW,
On 2016/12/09 04:55:05, benwells wrote:
> Should this be renamed to something like "FIRST_COULD_SHOW"?

I thought about that. The upside of not changing the name is that the old
couldShowEvent vector in the content setting should be overridden by the single
date, potentially saving us a bunch of data. Otherwise, it'll stay there
permanently until the user clears browsing data. I'm happy to go with your
preference here.

[dominickn, 2016-12-09 05:43:29]

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

[commit-bot: I haz the power, 2016-12-09 05:43:42]

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

[benwells, 2016-12-09 05:44:56]

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:165: web_contents,
web_contents->GetLastCommittedURL(),
On 2016/12/09 05:27:28, dominickn wrote:
> On 2016/12/09 04:55:05, benwells wrote:
> > Comment: It would probably have been wiser to put these changes in a
seperate
> > change, as I think they are unrelated.
> 
> Acknowledged - I was doing so much surgery here that these changes got rolled
> in. I can remove them if you like.

Understood. I'd leave it as is, it was more a comment for future CLs.

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:56:
APP_BANNER_EVENT_COULD_SHOW,
On 2016/12/09 05:27:28, dominickn wrote:
> On 2016/12/09 04:55:05, benwells wrote:
> > Should this be renamed to something like "FIRST_COULD_SHOW"?
> 
> I thought about that. The upside of not changing the name is that the old
> couldShowEvent vector in the content setting should be overridden by the
single
> date, potentially saving us a bunch of data. Otherwise, it'll stay there
> permanently until the user clears browsing data. I'm happy to go with your
> preference here.

Right now it's really not clear what this data is. I think you should either:
- add comments so it is clear what this actually means
- change the name but leave the key as is, with a comment explaining why
- add a new event, and add code to remove the old events.

I'll leave it up to you :)

[commit-bot: I haz the power, 2016-12-09 06:42:02]

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

[commit-bot: I haz the power, 2016-12-09 06:42:03]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-12-12 02:22:56]

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

[commit-bot: I haz the power, 2016-12-12 02:23:06]

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

[dominickn, 2016-12-12 02:23:15]

Thanks! PTAL

+cc mariakhomenko FYI

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/2553013004/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:56:
APP_BANNER_EVENT_COULD_SHOW,
On 2016/12/09 05:44:55, benwells wrote:
> On 2016/12/09 05:27:28, dominickn wrote:
> > On 2016/12/09 04:55:05, benwells wrote:
> > > Should this be renamed to something like "FIRST_COULD_SHOW"?
> > 
> > I thought about that. The upside of not changing the name is that the old
> > couldShowEvent vector in the content setting should be overridden by the
> single
> > date, potentially saving us a bunch of data. Otherwise, it'll stay there
> > permanently until the user clears browsing data. I'm happy to go with your
> > preference here.
> 
> Right now it's really not clear what this data is. I think you should either:
> - add comments so it is clear what this actually means
> - change the name but leave the key as is, with a comment explaining why
> - add a new event, and add code to remove the old events.
> 
> I'll leave it up to you :)

Done.

[commit-bot: I haz the power, 2016-12-12 04:10:02]

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

[commit-bot: I haz the power, 2016-12-12 04:10:03]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[benwells, 2016-12-14 05:55:37]

lgtm

[dominickn, 2016-12-14 06:01:20]

Description was changed from

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replace with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073
==========

to

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073
==========

[dominickn, 2016-12-14 06:01:31]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-12-14 06:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2553013004/#ps80001
(title: "Add comments")

[commit-bot: I haz the power, 2016-12-14 06:02:06]

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-12-14 07:26:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481695291778420,
"parent_rev": "df472b2db09c04d64aaeabeb007597379e703420", "commit_rev":
"c7821dcbc19b795144bea3864d15bccc4764c809"}

[commit-bot: I haz the power, 2016-12-14 07:26:51]

Description was changed from

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073
==========

to

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073

Review-Url: https://codereview.chromium.org/2553013004
==========

[commit-bot: I haz the power, 2016-12-14 07:26:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-14 07:29:43]

Description was changed from

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073

Review-Url: https://codereview.chromium.org/2553013004
==========

to

==========
Remove the app banner navigation heuristic.

App banners now use site engagement to determine when to trigger. This
CL removes the old navigation-based heuristic and all tests based on it.
New tests using the site engagement metric are added in their place.

The app banner settings helper still records a single
APP_BANNER_COULD_SHOW event for the purposes of metrics. The old vector
of COULD_SHOW events is removed in this CL and replaced with a single
event recorded the first time that a banner could be shown. Any
additional COULD_SHOW events are ignored; all other events will
overwrite old values when they are rewritten.

Many tests that checked the direct/indirect navigation accumulation have
been removed. New tests for checking the backoff period (and changing
that backoff) have been added in their place.

BUG=667073

Committed: https://crrev.com/598df313ae463915342386163972df774453c570
Cr-Commit-Position: refs/heads/master@{#438452}
==========

[commit-bot: I haz the power, 2016-12-14 07:29:44]

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