Replace AppBannerDataFetcher with InstallableManager.

Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org

Committed: https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45
Cr-Commit-Position: refs/heads/master@{#410959}

[dominickn, 2016-07-18 12:27:48]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

The app banner logging has been migrated to the installable/ component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

All app banner data fetcher code and tests is deleted in this CL. Additionally,
The app banner emulation class is no longer necessary to emulate mobile
banners on desktop. Instead, the AppBannerManagerDesktop class can
be used directly.

The app banner logging has been migrated to the installable/ component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

[dominickn, 2016-07-18 12:29:05]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

All app banner data fetcher code and tests is deleted in this CL. Additionally,
The app banner emulation class is no longer necessary to emulate mobile
banners on desktop. Instead, the AppBannerManagerDesktop class can
be used directly.

The app banner logging has been migrated to the installable/ component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

All app banner data fetcher code and tests is deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly.

The app banner logging has been migrated to the installable/ component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

[dominickn, 2016-07-18 12:29:06]

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

[dominickn, 2016-07-18 12:29:43]

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

[commit-bot: I haz the power, 2016-07-18 12:30:00]

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

[dominickn, 2016-07-18 12:32:30]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

All app banner data fetcher code and tests is deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly.

The app banner logging has been migrated to the installable/ component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

[dominickn, 2016-07-18 12:45:57]

PTAL, thanks! (and sorry again).

This is the second of two CLs which implement the InstallableCHecker and replace
AppBannerDataFetcher. This CL is a top to bottom refactor of app banners to use
InstallableChecker and consolidate generic and platform-specific code as much as
possible.

CC list: FYI

[commit-bot: I haz the power, 2016-07-18 13:06:34]

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

[commit-bot: I haz the power, 2016-07-18 13:06:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2016-07-18 14:22:00]

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

[commit-bot: I haz the power, 2016-07-18 14:22:21]

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

[dominickn, 2016-07-18 14:25:37]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which is
a generic and reusable version of AppBannerDataFetcher. This CL
replaces AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) have been moved to the AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies the app
banner code, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

[dominickn, 2016-07-18 14:26:07]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the Installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

[commit-bot: I haz the power, 2016-07-18 15:02:26]

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

[commit-bot: I haz the power, 2016-07-18 15:02:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominickn, 2016-07-19 09:07:19]

On 2016/07/18 12:45:57, dominickn wrote:
> PTAL, thanks! (and sorry again).
> 
> This is the second of two CLs which implement the InstallableCHecker and
replace
> AppBannerDataFetcher. This CL is a top to bottom refactor of app banners to
use
> InstallableChecker and consolidate generic and platform-specific code as much
as
> possible.
> 
> CC list: FYI

Failing Service Worker tests will be fixed in crrev.com/2158253002 (the tests
are inappropriately using app banners to fetch a site's manifest).

[dominickn, 2016-07-21 00:43:43]

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

[dominickn, 2016-07-21 00:44:05]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. A future CL will introduce further tests and new
metrics with the installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

[commit-bot: I haz the power, 2016-07-21 00:44:12]

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-07-21 00:46:44]

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

[commit-bot: I haz the power, 2016-07-21 00:46:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dominickn, 2016-07-21 01:44:33]

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

[commit-bot: I haz the power, 2016-07-21 01:44:58]

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-07-21 01:58:45]

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

[commit-bot: I haz the power, 2016-07-21 01:58:46]

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

[dominickn, 2016-07-21 04:19:44]

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

[commit-bot: I haz the power, 2016-07-21 04:19:57]

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-07-21 04:22:58]

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

[commit-bot: I haz the power, 2016-07-21 04:22:59]

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...)

[gone, 2016-07-23 23:39:23]

Didn't go over the desktop stuff.  Is this going to have to be rebased on the
other patch you have out?

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:155:
/** Returns whether an AppBannerDataFetcher is actively retrieving data. */
Comment should be updated.

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:304: 
Extra newline?

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/android/banners/app_banner_manager_android.cc:134: // This
function does not check whether a non-WebAPK web app is installed.
Seems like this comment should be flipped so that the part about not checking
for a web app is more prominent, given the name of the function.

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/banners/app_banner_manager.cc:166: return "";
Is the standard thing to do to use std::string()?

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/banners/app_banner_manager.cc:350:
active_media_players_.push_back(id);
Eh, why do app banners care if media started playing?  Kind of a weird place to
check.  Is this to prevent banners from appearing while a video is playing or
something?

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
File chrome/browser/banners/app_banner_manager.h (right):

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/banners/app_banner_manager.h:36: // Creates an app banner.
Should probably add // at the beginning of the blank lines to show that the
comments go together.

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/banners/app_banner_manager.h:128: // so anything in the browser
process which needs this data can access it
This entire thing reads like it should be in the class comment to serve as
documentation.  Does it make sense to move it upward?

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
File chrome/browser/banners/app_banner_manager_browsertest.cc (left):

https://chromiumcodereview.appspot.com/2156113002/diff/100001/chrome/browser/...
chrome/browser/banners/app_banner_manager_browsertest.cc:297:
IN_PROC_BROWSER_TEST_F(AppBannerDataFetcherBrowserTest, PlayAppManifest) {
Where'd this test go?  Are we relying on the Java test?

[dominickn, 2016-07-25 00:23:51]

Thanks! This CL will be rebased onto the final version of the InstallableChecker
CL.

https://codereview.chromium.org/2156113002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java
(right):

https://codereview.chromium.org/2156113002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/banners/AppBannerManager.java:155:
/** Returns whether an AppBannerDataFetcher is actively retrieving data. */
On 2016/07/23 23:39:22, dfalcantara wrote:
> Comment should be updated.

Done.

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:304: 
On 2016/07/23 23:39:22, dfalcantara wrote:
> Extra newline?

Done.

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:134: // This
function does not check whether a non-WebAPK web app is installed.
On 2016/07/23 23:39:23, dfalcantara wrote:
> Seems like this comment should be flipped so that the part about not checking
> for a web app is more prominent, given the name of the function.

Done.

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

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:166: return "";
On 2016/07/23 23:39:23, dfalcantara wrote:
> Is the standard thing to do to use std::string()?

Done.

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:350:
active_media_players_.push_back(id);
On 2016/07/23 23:39:23, dfalcantara wrote:
> Eh, why do app banners care if media started playing?  Kind of a weird place
to
> check.  Is this to prevent banners from appearing while a video is playing or
> something?

Yes, this is to prevent banners from appearing while a video is playing. With
the new site engagement heuristic, banners can appear any time while a user is
on a page - including when a video is running. This is a pretty blunt way of
preventing the banner flow from triggering if a video is playing.

I'm going to add a heuristic to detect how many times the flow isn't started
because of this.

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:36: // Creates an app banner.
On 2016/07/23 23:39:23, dfalcantara wrote:
> Should probably add // at the beginning of the blank lines to show that the
> comments go together.

Done.

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.h:128: // so anything in the browser
process which needs this data can access it
On 2016/07/23 23:39:23, dfalcantara wrote:
> This entire thing reads like it should be in the class comment to serve as
> documentation.  Does it make sense to move it upward?

Done.

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
File chrome/browser/banners/app_banner_manager_browsertest.cc (left):

https://codereview.chromium.org/2156113002/diff/100001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager_browsertest.cc:297:
IN_PROC_BROWSER_TEST_F(AppBannerDataFetcherBrowserTest, PlayAppManifest) {
On 2016/07/23 23:39:23, dfalcantara wrote:
> Where'd this test go?  Are we relying on the Java test?

The fetching is now tested in InstallableChecker (there is the Play manifest
test there). The specific Play platform part is now solely housed in the
AppBannerManagerAndroid to avoid polluting the parent class with that
functionality. So yes, the Java test is now the sole location for testing the
native functionality (at least until browser_tests are functional on Android!)

[dominickn, 2016-07-29 09:29:14]

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

[commit-bot: I haz the power, 2016-07-29 09:29:23]

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-07-29 10:20:46]

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

[commit-bot: I haz the power, 2016-07-29 10:20:47]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-03 06:39:37]

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

[commit-bot: I haz the power, 2016-08-03 06:39:49]

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

[dominickn, 2016-08-03 06:40:26]

benwells: PTAL, thanks!

[dominickn, 2016-08-03 07:03:49]

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

[commit-bot: I haz the power, 2016-08-03 07:03:57]

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-08-03 08:05:38]

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

[commit-bot: I haz the power, 2016-08-03 08:05:39]

Dry run: This issue passed the CQ dry run.

[gone, 2016-08-03 19:33:08]

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:266:
weak_manager_->FetchWebappSplashScreenImageCallback(uid)));
Don't you need to check if the weak pointer is still valid before using it?

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
chrome/browser/android/banners/app_banner_manager_android.cc:199: }
Shouldn't this check if the infobar is null before trying to add it?

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
chrome/browser/banners/app_banner_manager.cc:231:
banners::TrackDisplayEvent(DISPLAY_EVENT_WEB_APP_BANNER_REQUESTED);
Do you need the banners:: namespace here?  Seems fine immediately below.

https://chromiumcodereview.appspot.com/2156113002/diff/220001/chrome/browser/...
chrome/browser/banners/app_banner_manager.cc:289: // for stopping now is that
the banner has been added to the homescreen.
The comment is a little weird.  The banner was added to the home screen?  The
app?

[dominickn, 2016-08-04 02:51:06]

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

[dominickn, 2016-08-04 02:51:07]

https://codereview.chromium.org/2156113002/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2156113002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:266:
weak_manager_->FetchWebappSplashScreenImageCallback(uid)));
On 2016/08/03 19:33:08, dfalcantara wrote:
> Don't you need to check if the weak pointer is still valid before using it?

Done.

https://codereview.chromium.org/2156113002/diff/220001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2156113002/diff/220001/chrome/browser/android...
chrome/browser/android/banners/app_banner_manager_android.cc:199: }
On 2016/08/03 19:33:08, dfalcantara wrote:
> Shouldn't this check if the infobar is null before trying to add it?

Done.

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

https://codereview.chromium.org/2156113002/diff/220001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:231:
banners::TrackDisplayEvent(DISPLAY_EVENT_WEB_APP_BANNER_REQUESTED);
On 2016/08/03 19:33:08, dfalcantara wrote:
> Do you need the banners:: namespace here?  Seems fine immediately below.

Done.

https://codereview.chromium.org/2156113002/diff/220001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:289: // for stopping now is that
the banner has been added to the homescreen.
On 2016/08/03 19:33:08, dfalcantara wrote:
> The comment is a little weird.  The banner was added to the home screen?  The
> app?

Done.

[gone, 2016-08-04 17:09:18]

Android stuff seems fine.  lgtm.

[pkotwicz, 2016-08-08 01:11:46]

benwells@: Ping!

[benwells, 2016-08-09 03:41:26]

lgtm with nits / comments

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

https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:67: base::Time
AppBannerManager::GetCurrentTime() {
nit: If GetCurrentTime is only used for testing, call it
GetCurrentTimeForTesting

https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:118: manager_->GetData(
Now I've seen this API in use it seems a bit awkward. In particular the way
GetData is called twice, once just to get the manifest (with a callback
OnDidGetManifest) and a second time to do the check.

It would be nice if you could tell the difference between the two calls more
obviously, with callbacks which correspond to the calls more. Another niggle is
the name ContinueInstallation which doesn't communicate much about what
specifically it will do.

In a future patch (or this one if you feel like it) I'd consider thinking about
that. One simple thing to do would be to have some functions to get Params which
communicate what the GetData will do, e.g. GetData(ParamsToGetManifest(),
...OnDidGetManifest...) and GetData(ParamsToPerformCheck(),
...OnDidPerformCheck...).

[dominickn, 2016-08-09 08:28:57]

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

https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:67: base::Time
AppBannerManager::GetCurrentTime() {
On 2016/08/09 03:41:26, benwells wrote:
> nit: If GetCurrentTime is only used for testing, call it
> GetCurrentTimeForTesting

It's not only used for testing - it's also used for recording things to content
settings.

https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:118: manager_->GetData(
On 2016/08/09 03:41:26, benwells wrote:
> Now I've seen this API in use it seems a bit awkward. In particular the way
> GetData is called twice, once just to get the manifest (with a callback
> OnDidGetManifest) and a second time to do the check.
> 
> It would be nice if you could tell the difference between the two calls more
> obviously, with callbacks which correspond to the calls more. Another niggle
is
> the name ContinueInstallation which doesn't communicate much about what
> specifically it will do.
> 
> In a future patch (or this one if you feel like it) I'd consider thinking
about
> that. One simple thing to do would be to have some functions to get Params
which
> communicate what the GetData will do, e.g. GetData(ParamsToGetManifest(),
> ...OnDidGetManifest...) and GetData(ParamsToPerformCheck(),
> ...OnDidPerformCheck...). 

Acknowledged. I'd like to defer bigger changes to the future, but one of the
reasons I designed the API like this was because banners alone had this
multiple-call thing going, with the split paths for native versus web banners on
mobile.

It was hard coming up with a name for ContinueInstallableCheck, because that's
the divergement point where the Android code goes and looks for a native app.
I'll have a think tonight and land this patch tomorrow after we've had a chance
to confer.

[dominickn, 2016-08-10 03:33:37]

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

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

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

[benwells, 2016-08-10 03:36:05]

slgtm

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

https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
chrome/browser/banners/app_banner_manager.cc:67: base::Time
AppBannerManager::GetCurrentTime() {
On 2016/08/09 08:28:57, dominickn wrote:
> On 2016/08/09 03:41:26, benwells wrote:
> > nit: If GetCurrentTime is only used for testing, call it
> > GetCurrentTimeForTesting
> 
> It's not only used for testing - it's also used for recording things to
content
> settings.

Acknowledged.

[commit-bot: I haz the power, 2016-08-10 04:43:29]

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

[commit-bot: I haz the power, 2016-08-10 04:43:30]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-10 04:47:16]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
==========

to

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org
==========

[dominickn, 2016-08-10 04:48:09]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableChecker.

https://crrev.com/2160513002 introduces the InstallableChecker, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableChecker. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the installable::ErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org
==========

to

==========
Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org
==========

[dominickn, 2016-08-10 04:48:42]

On 2016/08/10 03:36:05, benwells wrote:
> slgtm
> 
>
https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
> File chrome/browser/banners/app_banner_manager.cc (right):
> 
>
https://codereview.chromium.org/2156113002/diff/260001/chrome/browser/banners...
> chrome/browser/banners/app_banner_manager.cc:67: base::Time
> AppBannerManager::GetCurrentTime() {
> On 2016/08/09 08:28:57, dominickn wrote:
> > On 2016/08/09 03:41:26, benwells wrote:
> > > nit: If GetCurrentTime is only used for testing, call it
> > > GetCurrentTimeForTesting
> > 
> > It's not only used for testing - it's also used for recording things to
> content
> > settings.
> 
> Acknowledged.

TBR'ing thestig@ for browser.cc as it's a trivial API change.

[dominickn, 2016-08-10 04:49:08]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-08-10 04:49:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2156113002/#ps280001
(title: "Naming, includes")

[commit-bot: I haz the power, 2016-08-10 04:49:55]

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-08-10 04:54:30]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org
==========

to

==========
Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org
==========

[commit-bot: I haz the power, 2016-08-10 04:54:31]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-08-10 04:56:52]

Description was changed from

==========
Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org
==========

to

==========
Replace AppBannerDataFetcher with InstallableManager.

https://crrev.com/2160513002 introduces the InstallableManager, which
is a generic, caching, WebContents-scoped version of
AppBannerDataFetcher. This CL replaces AppBannerDataFetcher,
AppBannerDataFetcherDesktop and AppBannerDataFetcherAndroid
with calls to InstallableManager. Banner-specific functionality (e.g.
sending IPCs to trigger JS events in the renderer, receiving renderer
responses, etc.) has been moved to AppBannerManager, and platform-
specific code now lives in AppBannerManagerDesktop and
AppBannerManagerAndroid. This consolidates and simplifies app
banners, making it more testable and cleaner.

All app banner data fetcher code and tests are deleted in this CL.
Additionally, the app banner emulation class is no longer needed to
emulate mobile banners on desktop. Instead, the
AppBannerManagerDesktop class can be used directly. The app
banner logging code has been migrated to the installable component,
allowing other systems to make use of it.

The data fetcher tests have been migrated to run with the
AppBannerManager. Future CLs will introduce further tests and new
metrics with the InstallableErrorCode enum that is now used to
signal error states.

BUG=628921
TBR=thestig@chromium.org

Committed: https://crrev.com/d223e62e9459d219141c09ac6b10f983c841fa45
Cr-Commit-Position: refs/heads/master@{#410959}
==========

[commit-bot: I haz the power, 2016-08-10 04:56:54]

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

[Lei Zhang, 2016-08-16 02:44:54]

On 2016/08/10 04:48:42, dominickn wrote:
> TBR'ing thestig@ for browser.cc as it's a trivial API change.

lgtm, but please actually add the people being TBR'd so they get notified.

[dominickn, 2016-08-16 03:15:56]

On 2016/08/16 02:44:54, Lei Zhang wrote:
> On 2016/08/10 04:48:42, dominickn wrote:
> > TBR'ing thestig@ for browser.cc as it's a trivial API change.
> 
> lgtm, but please actually add the people being TBR'd so they get notified.

Whoops. Apologies, I'll make sure it's on the list next time. :)