Allow banners to trigger on sites which don't register a service worker onload.

chrome/browser/installable/installable_manager.h

Index: chrome/browser/installable/installable_manager.h
diff --git a/chrome/browser/installable/installable_manager.h b/chrome/browser/installable/installable_manager.h
index 6f9204374150287a5b6ec8964a87d575f8b8d5e1..6bcc435a23345c553ff9991d08df4ab2f35b1968 100644
--- a/chrome/browser/installable/installable_manager.h
+++ b/chrome/browser/installable/installable_manager.h
@@ -18,6 +18,7 @@
 #include "chrome/browser/installable/installable_logging.h"
 #include "chrome/browser/installable/installable_metrics.h"
 #include "content/public/browser/service_worker_context.h"
+#include "content/public/browser/service_worker_context_observer.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "content/public/common/manifest.h"
@@ -49,6 +50,9 @@ struct InstallableParams {
   // current URL.
   bool check_installable = false;
 
+  // Whether or not to wait indefinitely for a service worker.
+  bool wait_for_worker = true;

[pkotwicz, 2017/06/14 15:34:36]

Drive by: This should be false for add_to_homescreen_data_fetcher.cc ?

[dominickn, 2017/06/15 04:16:07]

Nope, it should not be false for add_to_homescreen_data_fetcher.

1. Setting this to false means that https://crbug.com/721881 won't be fixed,
since we need to wait for the service worker to fix it.

2. add_to_homescreen_data_fetcher.cc already has a 4 second wait timeout so it
handles the case where this waits indefinitely.

[pkotwicz, 2017/06/15 15:10:04]

2. Doesn't your change cause the Web Manifest to be ignored if the page does not
have a service worker? e.g.
https://googlechrome.github.io/samples/web-application-manifest/index.html

1. I don't fully understand https://crbug.com/721881 It seems that
InstallableManager::OnRegistrationStored() should update the cached value
regardless of InstallableManager::worker_::is_waiting

[dominickn, 2017/06/15 23:45:38]

The fix for this is to make add to homescreen do what app banners do - get the
manifest first, pull necessary things out of it, and then do the rest of the
check. That's a follow up.
If the check has already finished, we've already dispatched the callback and
then there's no way for the InstallableManager to tell the add to homescreen
fetcher that something has changed. You end up with this:

1. User presses add to homescreen. No service worker registered yet
2. We do the check, there's no service worker. Run the callback
3. Fetcher determines it's not WebAPK compatible and shows the usual A2HS dialog
4. Service worker is registered. But the fetcher has already finished so there's
no point doing anything now.

The simple thing to do is as above: ask for the manifest first, and then do the
rest of the check.

[pkotwicz, 2017/06/16 23:07:43]

We will need to fetch the icons in the first call which gets the manifest.
Otherwise, we would ignore icons in the Web Manifest if the web page does not
have a service worker

We will need to add a timeout to webapk_update_data_fetcher.cc We want to update
WebAPKs for critical updates even if they no longer have a service worker

[dominickn, 2017/06/17 00:00:26]

No, we'll ignore icons if the web page has a manifest *that's WebAPKable*,
because that is checked before a service worker.

We can easily fetch manifest + icon, then check installability with
InstallableManager.
Off topic: I feel like what should happen is if a WebAPK loses its service
worker is that it should just start opening in a tab in Chrome. It's a pretty
bad experience for users if their WebAPK suddenly can't work offline any more.

[pkotwicz, 2017/06/19 18:46:50]

We need to decide whether to launch a page in full screen mode prior to loading
the page. Thus, we cannot check whether the page has a service worker registered
prior to deciding whether to launch the page in full screen mode.

[dominickn, 2017/06/20 02:26:24]

As discussed offline, this shouldn't be an issue because updating happens after
launch, and affects the next launch, not the current one.

+
   // Check whether there is a fetchable, non-empty icon in the manifest
   // conforming to the primary icon size parameters.
   bool fetch_valid_primary_icon = false;
@@ -104,7 +108,8 @@ using InstallableCallback = base::Callback<void(const InstallableData&)>;
 // This class is responsible for fetching the resources required to check and
 // install a site.
 class InstallableManager
-    : public content::WebContentsObserver,
+    : public content::ServiceWorkerContextObserver,
+      public content::WebContentsObserver,
       public content::WebContentsUserData<InstallableManager> {
  public:
   explicit InstallableManager(content::WebContents* web_contents);
@@ -124,10 +129,14 @@ class InstallableManager
   // when the data is ready; the synchronous execution ensures that the
   // references |callback| receives in its InstallableData argument are valid.
   //
+  // Clients must be prepared for |callback| to not ever be invoked. For
+  // instance, if installability checking is requested, this method will wait
+  // until the site registers a service worker (and hence not invoke |callback|
+  // at all if a service worker is never registered).
+  //
   // Calls requesting data that is already fetched will return the cached data.
-  // This method is marked virtual so clients may mock this object in tests.
-  virtual void GetData(const InstallableParams& params,
-                       const InstallableCallback& callback);
+  void GetData(const InstallableParams& params,
+               const InstallableCallback& callback);
 
   // Called via AppBannerManagerAndroid to record metrics on how often the
   // installable check is completed when the menu or add to homescreen menu item
@@ -143,12 +152,15 @@ class InstallableManager
   FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest,
                            ManagerBeginsInEmptyState);
   FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest, CheckWebapp);
+  FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest,
+                           CheckLazyServiceWorker);
 
   using Task = std::pair<InstallableParams, InstallableCallback>;
   using IconParams = std::tuple<int, int, content::Manifest::Icon::IconPurpose>;
 
   struct ManifestProperty;
-  struct InstallableProperty;
+  struct ValidManifestProperty;
+  struct ServiceWorkerProperty;
   struct IconProperty;
 
   // Returns an IconParams object that queries for a primary icon conforming to
@@ -171,8 +183,9 @@ class InstallableManager
 
   // Gets/sets parts of particular properties. Exposed for testing.
   InstallableStatusCode manifest_error() const;
-  InstallableStatusCode installable_error() const;
-  void set_installable_error(InstallableStatusCode error_code);
+  InstallableStatusCode valid_manifest_error() const;
+  void set_valid_manifest_error(InstallableStatusCode error_code);
+  InstallableStatusCode worker_error() const;
   InstallableStatusCode icon_error(const IconParams& icon_params);
   GURL& icon_url(const IconParams& icon_params);
   const SkBitmap* icon(const IconParams& icon);
@@ -209,8 +222,12 @@ class InstallableManager
   void OnDidCheckHasServiceWorker(content::ServiceWorkerCapability capability);
 
   void CheckAndFetchBestIcon(const IconParams& params);
-  void OnIconFetched(
-      const GURL icon_url, const IconParams& params, const SkBitmap& bitmap);
+  void OnIconFetched(const GURL icon_url,
+                     const IconParams& params,
+                     const SkBitmap& bitmap);
+
+  // content::ServiceWorkerContextObserver overrides
+  void OnRegistrationStored(const GURL& pattern) override;
 
   // content::WebContentsObserver overrides
   void DidFinishNavigation(content::NavigationHandle* handle) override;
@@ -225,9 +242,14 @@ class InstallableManager
 
   // Installable properties cached on this object.
   std::unique_ptr<ManifestProperty> manifest_;
-  std::unique_ptr<InstallableProperty> installable_;
+  std::unique_ptr<ValidManifestProperty> valid_manifest_;
+  std::unique_ptr<ServiceWorkerProperty> worker_;
   std::map<IconParams, IconProperty> icons_;
 
+  // Owned by the storage partition attached to the content::WebContents which
+  // this object is scoped to.
+  content::ServiceWorkerContext* service_worker_context_;
+
   // Whether or not the current page is a PWA. This is reset per navigation and
   // is independent of the caching mechanism, i.e. if a PWA check is run
   // multiple times for one page, this will be set on the first check.