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

chrome/browser/installable/installable_manager.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_INSTALLABLE_INSTALLABLE_MANAGER_H_
 #define CHROME_BROWSER_INSTALLABLE_INSTALLABLE_MANAGER_H_
 
 #include <map>
 #include <memory>
 #include <tuple>
 #include <utility>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #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"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "url/gurl.h"
 
 // This struct specifies the work to be done by the InstallableManager.
 // Data is cached and fetched in the order specified in this struct. A web app
 // manifest will always be fetched first.
 struct InstallableParams {
@@ skipping 11 matching lines @@
 
   // The minimum badge icon size to fetch. Used only if
   // |fetch_valid_badge_icon| is true.
   int minimum_badge_icon_size_in_px = -1;
 
   // Check whether the site is installable. That is, it has a manifest valid for
   // a web app and a service worker controlling the manifest start URL and the
   // current URL.
   bool check_installable = false;
 
+  // Whether or not to wait indefinitely for a service worker.
+  bool wait_for_worker = true;

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

There aren't any callers which set this to false. Should we have the parameter?
If we have the parameter, we should handle the case of

1) InstallableManager::GetData() being called with wait_for_worker=false before
the service worker being registered
then
2) The service worker being registered
then
3) InstallableManager::GetData() being called with wait_for_worker=true

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

This is set to false in app_banner_manager.cc when |triggered_by_devtools| is
true. This is so developers who are testing banners via devtools never wait for
a service worker (that scenario doesn't make sense).

All three of your scenarios are explicitly tested in
installable_manager_browsertest.cc.

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

Sorry for the confusion. I had this test in particular in mind

IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
                       CheckDontWaitThenWaitForServiceWorker) {
  content::WebContents* web_contents =
      browser()->tab_strip_model()->GetActiveWebContents();
  base::RunLoop sw_run_loop;
  auto manager = base::MakeUnique<LazyWorkerInstallableManager>(
      web_contents, sw_run_loop.QuitClosure());

  // Load a URL with no service worker.
  GURL test_url = embedded_test_server()->GetURL(
      "/banners/manifest_no_service_worker.html");
  ui_test_utils::NavigateToURL(browser(), test_url);


  {
    base::RunLoop tester_run_loop;
    std::unique_ptr<CallbackTester> tester(
        new CallbackTester(tester_run_loop.QuitClosure()));

    InstallableParams params = GetWebAppParams();
    params.wait_for_worker = false;
    manager->GetData(params,
                     base::Bind(&CallbackTester::OnDidFinishInstallableCheck,
                                base::Unretained(tester.get())));
    tester_run_loop.Run();

    // We should have returned with an error.
    EXPECT_FALSE(tester->manifest().IsEmpty());
    EXPECT_FALSE(tester->is_installable());
    EXPECT_EQ(NO_MATCHING_SERVICE_WORKER, tester->error_code());
  }

  {
    base::RunLoop tester_run_loop;
    std::unique_ptr<CallbackTester> tester(
        new CallbackTester(tester_run_loop.QuitClosure()));

    InstallableParams params = GetWebAppParams();
    params.wait_for_worker = true;
    manager->GetData(params,
                     base::Bind(&CallbackTester::OnDidFinishInstallableCheck,
                                base::Unretained(tester.get())));

    sw_run_loop.Run();

    EXPECT_TRUE(content::ExecuteScript(
        web_contents,
        "navigator.serviceWorker.register('service_worker.js');"));
    tester_run_loop.Run();

    // The callback should tell us that the page is installable
    EXPECT_FALSE(tester->manifest().IsEmpty());
    EXPECT_TRUE(tester->is_installable());
    EXPECT_EQ(NO_ERROR_DETECTED, tester->error_code());
  }
}

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

That makes sense now. Added the proposed test and stopped caching a
NO_MATCHING_SERVICE_WORKER error.

+
   // 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;
 
   // Check whether there is a fetchable, non-empty icon in the manifest
   // conforming to the badge icon size parameters.
   bool fetch_valid_badge_icon = false;
 };
 
 // This struct is passed to an InstallableCallback when the InstallableManager
@@ skipping 35 matching lines @@
   // app manifest. If check_installable was true and the site isn't installable,
   // the reason will be in error_code.
   const bool is_installable;
 };
 
 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);
   ~InstallableManager() override;
 
   // Returns true if the overall security state of |web_contents| is sufficient
   // to be considered installable.
   static bool IsContentSecure(content::WebContents* web_contents);
 
   // Returns the minimum icon size in pixels for a site to be installable.
   // TODO(dominickn): consolidate this concept with minimum_icon_size_in_px
   // across all platforms.
   static int GetMinimumIconSizeInPx();
 
   // Get the installable data, fetching the resources specified in |params|.
   // |callback| is invoked synchronously (i.e. no via PostTask on the UI thread
   // 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
   // is opened on Android.
   void RecordMenuOpenHistogram();
   void RecordMenuItemAddToHomescreenHistogram();
   void RecordQueuedMetricsOnTaskCompletion(const InstallableParams& params,
                                            bool check_passed);
 
+ protected:
+  // For mocking in tests.
+  virtual void OnWaitingForServiceWorker() {}
+
  private:
   friend class InstallableManagerBrowserTest;
   friend class InstallableManagerUnitTest;
   FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest,
                            ManagerBeginsInEmptyState);
   FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest, CheckWebapp);
+  FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest,
+                           CheckLazyServiceWorkerPassesWhenWaiting);
+  FRIEND_TEST_ALL_PREFIXES(InstallableManagerBrowserTest,
+                           CheckLazyServiceWorkerNoFetchHandlerFails);
 
   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
   // the primary icon size parameters in |params|.
   IconParams ParamsForPrimaryIcon(const InstallableParams& params) const;
   // Returns an IconParams object that queries for a badge icon conforming to
   // the badge icon size parameters in |params|.
   IconParams ParamsForBadgeIcon(const InstallableParams& params) const;
 
   // Returns true if |params| matches any fetched icon, or false if no icon has
   // been requested yet or there is no match.
   bool IsIconFetched(const IconParams& params) const;
 
   // Sets the icon matching |params| as fetched.
   void SetIconFetched(const IconParams& params);
 
   // Returns the error code associated with the resources requested in |params|,
   // or NO_ERROR_DETECTED if there is no error.
   InstallableStatusCode GetErrorCode(const InstallableParams& params);
 
   // 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;
+  bool worker_waiting() const;
   InstallableStatusCode icon_error(const IconParams& icon_params);
   GURL& icon_url(const IconParams& icon_params);
   const SkBitmap* icon(const IconParams& icon);
 
   // Returns the WebContents to which this object is attached, or nullptr if the
   // WebContents doesn't exist or is currently being destroyed.
   content::WebContents* GetWebContents();
 
   // Returns true if |params| requires no more work to be done.
   bool IsComplete(const InstallableParams& params) const;
@@ skipping 16 matching lines @@
   void FetchManifest();
   void OnDidGetManifest(const GURL& manifest_url,
                         const content::Manifest& manifest);
 
   void CheckInstallable();
   bool IsManifestValidForWebApp(const content::Manifest& manifest);
   void CheckServiceWorker();
   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;
   void WebContentsDestroyed() override;
 
   const GURL& manifest_url() const;
   const content::Manifest& manifest() const;
   bool is_installable() const;
 
   // The list of <params, callback> pairs that have come from a call to GetData.
   std::vector<Task> tasks_;
 
   // 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.
   InstallabilityCheckStatus page_status_;
 
   // Counts for the number of queued requests of the menu and add to homescreen
   // menu item there have been whilst the installable check is awaiting
   // completion. Used for metrics recording.
   int menu_open_count_;
   int menu_item_add_to_homescreen_count_;
 
   bool is_active_;
   bool is_pwa_check_complete_;
 
   base::WeakPtrFactory<InstallableManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(InstallableManager);
 };
 
 #endif  // CHROME_BROWSER_INSTALLABLE_INSTALLABLE_MANAGER_H_