[App banners] Be less strict about navigations

[App banners] Be less strict about navigations

* In-page navigations no longer cancel a fetcher.  URL checks
  are removed for recording banner events.

* Remove dead code in AppBannerSettingsHelper for blocking
  banners.

* Add a new Java test for popping up a web app banner.  This
  will be extended to allow clicking on the "add to home screen"
  button soon.

BUG=468148
TEST=AppBannerManagerTest

Committed: https://crrev.com/6d609a9bcc1f72c780d7e075508b991c9c9a29ea
Cr-Commit-Position: refs/heads/master@{#321493}

[gone, 2015-03-18 18:20:26]

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

[gone, 2015-03-18 18:20:27]

Ben for the C++ stuff, Ted for the Java stuff

[gone, 2015-03-18 20:06:08]

Hrm, I'm missing something here.  Even if the AppBannerDataFetcher doesn't
cancel itself, the AppBannerManager will kick off a new one because a navigation
finished.  Hold off on reviewing.

[gone, 2015-03-18 21:46:21]

Alright, I think I've got a handle on it.  PTAL.

[Ted C, 2015-03-19 00:26:00]

lgtm

https://codereview.chromium.org/1017193002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://codereview.chromium.org/1017193002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:338:
@Override
the indent is inconsistent.

[benwells, 2015-03-19 07:18:55]

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

https://codereview.chromium.org/1017193002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:301: bool
AppBannerSettingsHelper::IsAllowed(
Thanks for removing this dead stuff, I'd forgotten about it!

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

https://codereview.chromium.org/1017193002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:85: && first.path() ==
second.path() && first.query() == second.query();
Formatting nit: the && should be on the previous line.

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

https://codereview.chromium.org/1017193002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:50: static bool
URLsAreForTheSamePage(const GURL& first, const GURL& second);
Why is this is AppBannerSettingsHelper? Seems like it should go in
AppBannerManager or the Fetcher.

[gone, 2015-03-19 19:01:35]

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/android/j...
File
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java
(right):

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/android/j...
chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java:338:
@Override
On 2015/03/19 00:26:00, Ted C wrote:
> the indent is inconsistent.

Done.

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:85: && first.path() ==
second.path() && first.query() == second.query();
On 2015/03/19 07:18:55, benwells wrote:
> Formatting nit: the && should be on the previous line.

Ah, the fun differences between C++ and Java style.

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:85: && first.path() ==
second.path() && first.query() == second.query();
On 2015/03/19 07:18:55, benwells wrote:
> Formatting nit: the && should be on the previous line.

Done.

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.h:50: static bool
URLsAreForTheSamePage(const GURL& first, const GURL& second);
On 2015/03/19 07:18:55, benwells wrote:
> Why is this is AppBannerSettingsHelper? Seems like it should go in
> AppBannerManager or the Fetcher.

Done.

[benwells, 2015-03-20 00:44:33]

lgtm

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://chromiumcodereview.appspot.com/1017193002/diff/40001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.cc:85: && first.path() ==
second.path() && first.query() == second.query();
On 2015/03/19 19:01:35, dfalcantara wrote:
> On 2015/03/19 07:18:55, benwells wrote:
> > Formatting nit: the && should be on the previous line.
> 
> Done.

git cl format is your friend.

[gone, 2015-03-20 00:47:20]

Could've sworn git cl format did some weird things in Java code, but it seems to
work fine for this patch.  Thanks!

[gone, 2015-03-20 00:47:26]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2015-03-20 00:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1017193002/#ps80001
(title: "git cl format")

[commit-bot: I haz the power, 2015-03-20 00:47:53]

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

[commit-bot: I haz the power, 2015-03-20 01:58:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-20 01:59:33]

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