Check if a PWA is installed before checking the service worker and icon.

Check if a PWA is installed before checking the service worker and icon.

This CL reorganises the app banner validity checking to avoid the
asynchronous service worker check and app icon download if the site
being checked should not be permitted to show a banner. This may happen
if the banner was shown and dismissed / ignored recently, or if the PWA
has already been installed.

This requires a slight reorganisation in the code as the validity check
can only be run after the manifest is downloaded, or after native app
data has been fetched if we are checking a native app banner.

BUG=667073
Review-Url: https://codereview.chromium.org/2620613006
Cr-Commit-Position: refs/heads/master@{#443826}
Committed: https://chromium.googlesource.com/chromium/src/+/a8f5781bcfc7c13294895aa12205d81c3e102a82

[dominickn, 2017-01-10 06:11:43]

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

[commit-bot: I haz the power, 2017-01-10 06:12:21]

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

[dominickn, 2017-01-10 06:57:52]

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

[commit-bot: I haz the power, 2017-01-10 06:58:08]

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

[dominickn, 2017-01-10 08:06:40]

Patchset #2 (id:20001) has been deleted

[dominickn, 2017-01-10 08:07:18]

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

[commit-bot: I haz the power, 2017-01-10 08:07:28]

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, 2017-01-10 08:54:55]

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

[commit-bot: I haz the power, 2017-01-10 08:54:56]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-10 22:51:58]

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

[dominickn, 2017-01-10 22:51:59]

PTAL, thanks!

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

https://codereview.chromium.org/2620613006/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:471: case INSUFFICIENT_ENGAGEMENT:
ShouldShowBanner() will never return INSUFFICIENT_ENGAGEMENT any more.

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

https://codereview.chromium.org/2620613006/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:250: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
This is covered by AppBannerSettingsHelper::HasSufficientEngagement() and
AppBannerManager::IsDebugMode().

[benwells, 2017-01-13 06:04:27]

lgtm with one little thing

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

https://codereview.chromium.org/2620613006/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:465: break;
Could this break be NOTREACHED()?

[dominickn, 2017-01-13 06:10:57]

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

[commit-bot: I haz the power, 2017-01-13 06:11:12]

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

[dominickn, 2017-01-13 06:11:21]

Thanks!

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

https://codereview.chromium.org/2620613006/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:465: break;
On 2017/01/13 06:04:27, benwells wrote:
> Could this break be NOTREACHED()?

Done.

[dominickn, 2017-01-13 06:12:32]

The CQ bit was unchecked by dominickn@chromium.org

[dominickn, 2017-01-13 06:12:37]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-01-13 06:12:37]

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/2620613006/#ps60001
(title: "NOTREACHED")

[commit-bot: I haz the power, 2017-01-13 06:12:49]

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, 2017-01-13 06:57:27]

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

[commit-bot: I haz the power, 2017-01-13 06:57:28]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominickn, 2017-01-13 07:00:37]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-01-13 07:00:46]

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, 2017-01-13 07:44:00]

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

[commit-bot: I haz the power, 2017-01-13 07:44:01]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominickn, 2017-01-13 07:46:55]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-01-13 07:47:16]

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, 2017-01-13 08:35:31]

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

[commit-bot: I haz the power, 2017-01-13 08:35:32]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominickn, 2017-01-15 22:56:12]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-01-15 22:56:22]

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, 2017-01-15 23:41:10]

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

[commit-bot: I haz the power, 2017-01-15 23:41:11]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dominickn, 2017-01-16 00:09:45]

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

[commit-bot: I haz the power, 2017-01-16 00:09:52]

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, 2017-01-16 01:06:07]

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

[commit-bot: I haz the power, 2017-01-16 01:06:08]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-16 01:47:40]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-01-16 01:47:40]

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/2620613006/#ps80001
(title: "Add missing enum value")

[commit-bot: I haz the power, 2017-01-16 01:47:45]

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, 2017-01-16 01:51:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484531260490210,
"parent_rev": "e4dc09ecc98ecda0724d906241c7c7cc6ef7a88a", "commit_rev":
"a8f5781bcfc7c13294895aa12205d81c3e102a82"}

[commit-bot: I haz the power, 2017-01-16 01:52:53]

Description was changed from

==========
Check if a PWA is installed before checking the service worker and icon.

This CL reorganises the app banner validity checking to avoid the
asynchronous service worker check and app icon download if the site
being checked should not be permitted to show a banner. This may happen
if the banner was shown and dismissed / ignored recently, or if the PWA
has already been installed.

This requires a slight reorganisation in the code as the validity check
can only be run after the manifest is downloaded, or after native app
data has been fetched if we are checking a native app banner.

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

to

==========
Check if a PWA is installed before checking the service worker and icon.

This CL reorganises the app banner validity checking to avoid the
asynchronous service worker check and app icon download if the site
being checked should not be permitted to show a banner. This may happen
if the banner was shown and dismissed / ignored recently, or if the PWA
has already been installed.

This requires a slight reorganisation in the code as the validity check
can only be run after the manifest is downloaded, or after native app
data has been fetched if we are checking a native app banner.

BUG=667073

Review-Url: https://codereview.chromium.org/2620613006
Cr-Commit-Position: refs/heads/master@{#443826}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8f5781bcfc7c13294895aa12205...
==========

[commit-bot: I haz the power, 2017-01-16 01:52:54]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a8f5781bcfc7c13294895aa12205...