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

chrome/browser/installable/installable_manager.cc

Index: chrome/browser/installable/installable_manager.cc
diff --git a/chrome/browser/installable/installable_manager.cc b/chrome/browser/installable/installable_manager.cc
index a529d291356af458ad5a6f23bdeed5eef7e4a234..5bfa16bbf3b75ed932584d9bb9530bde71f35efd 100644
--- a/chrome/browser/installable/installable_manager.cc
+++ b/chrome/browser/installable/installable_manager.cc
@@ -15,7 +15,6 @@
 #include "content/public/browser/manifest_icon_downloader.h"
 #include "content/public/browser/manifest_icon_selector.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/storage_partition.h"
 #include "net/base/url_util.h"
 #include "third_party/WebKit/public/platform/WebDisplayMode.h"
@@ -77,9 +76,16 @@ struct InstallableManager::ManifestProperty {
   bool fetched = false;
 };
 
-struct InstallableManager::InstallableProperty {
+struct InstallableManager::ValidManifestProperty {
   InstallableStatusCode error = NO_ERROR_DETECTED;
-  bool installable = false;
+  bool is_valid = false;
+  bool fetched = false;
+};
+
+struct InstallableManager::ServiceWorkerProperty {
+  InstallableStatusCode error = NO_ERROR_DETECTED;
+  bool has_worker = false;
+  bool is_waiting = false;
   bool fetched = false;
 };
 
@@ -101,16 +107,34 @@ struct InstallableManager::IconProperty {
 
 InstallableManager::InstallableManager(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
-      manifest_(new ManifestProperty()),
-      installable_(new InstallableProperty()),
+      manifest_(base::MakeUnique<ManifestProperty>()),
+      valid_manifest_(base::MakeUnique<ValidManifestProperty>()),
+      worker_(base::MakeUnique<ServiceWorkerProperty>()),
+      service_worker_context_(nullptr),
       page_status_(InstallabilityCheckStatus::NOT_STARTED),
       menu_open_count_(0),
       menu_item_add_to_homescreen_count_(0),
       is_active_(false),
       is_pwa_check_complete_(false),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  // This is null in unit tests.
+  if (web_contents) {
+    content::StoragePartition* storage_partition =
+        content::BrowserContext::GetStoragePartition(
+            Profile::FromBrowserContext(web_contents->GetBrowserContext()),
+            web_contents->GetSiteInstance());
+    DCHECK(storage_partition);
+
+    service_worker_context_ = storage_partition->GetServiceWorkerContext();
+    service_worker_context_->AddObserver(this);
+  }
+}
 
-InstallableManager::~InstallableManager() = default;
+InstallableManager::~InstallableManager() {
+  // Null in unit tests.
+  if (service_worker_context_)
+    service_worker_context_->RemoveObserver(this);
+}
 
 // static
 bool InstallableManager::IsContentSecure(content::WebContents* web_contents) {
@@ -229,8 +253,12 @@ InstallableStatusCode InstallableManager::GetErrorCode(
   if (manifest_->error != NO_ERROR_DETECTED)
     return manifest_->error;
 
-  if (params.check_installable && installable_->error != NO_ERROR_DETECTED)
-    return installable_->error;
+  if (params.check_installable) {
+    if (valid_manifest_->error != NO_ERROR_DETECTED)
+      return valid_manifest_->error;
+    if (worker_->error != NO_ERROR_DETECTED)
+      return worker_->error;
+  }
 
   if (params.fetch_valid_primary_icon) {
     IconProperty& icon = icons_[ParamsForPrimaryIcon(params)];
@@ -255,13 +283,21 @@ InstallableStatusCode InstallableManager::manifest_error() const {
   return manifest_->error;
 }
 
-InstallableStatusCode InstallableManager::installable_error() const {
-  return installable_->error;
+InstallableStatusCode InstallableManager::valid_manifest_error() const {
+  return valid_manifest_->error;
 }
 
-void InstallableManager::set_installable_error(
+void InstallableManager::set_valid_manifest_error(
     InstallableStatusCode error_code) {
-  installable_->error = error_code;
+  valid_manifest_->error = error_code;
+}
+
+InstallableStatusCode InstallableManager::worker_error() const {
+  return worker_->error;
+}
+
+bool InstallableManager::worker_waiting() const {
+  return worker_->is_waiting;
 }
 
 InstallableStatusCode InstallableManager::icon_error(
@@ -289,11 +325,12 @@ bool InstallableManager::IsComplete(const InstallableParams& params) const {
   //  a. the params did not request it, OR
   //  b. the resource has been fetched/checked.
   return manifest_->fetched &&
-         (!params.check_installable || installable_->fetched) &&
+         (!params.check_installable ||
+          (valid_manifest_->fetched && worker_->fetched)) &&
          (!params.fetch_valid_primary_icon ||
-             IsIconFetched(ParamsForPrimaryIcon(params))) &&
+          IsIconFetched(ParamsForPrimaryIcon(params))) &&
          (!params.fetch_valid_badge_icon ||
-             IsIconFetched(ParamsForBadgeIcon(params)));
+          IsIconFetched(ParamsForBadgeIcon(params)));
 }
 
 void InstallableManager::Reset() {
@@ -317,8 +354,9 @@ void InstallableManager::Reset() {
   }
 
   page_status_ = InstallabilityCheckStatus::NOT_STARTED;
-  manifest_.reset(new ManifestProperty());
-  installable_.reset(new InstallableProperty());
+  manifest_ = base::MakeUnique<ManifestProperty>();
+  valid_manifest_ = base::MakeUnique<ValidManifestProperty>();
+  worker_ = base::MakeUnique<ServiceWorkerProperty>();
 
   is_active_ = false;
 }
@@ -327,7 +365,8 @@ void InstallableManager::SetManifestDependentTasksComplete() {
   DCHECK(!tasks_.empty());
   const InstallableParams& params = tasks_[0].first;
 
-  installable_->fetched = true;
+  valid_manifest_->fetched = true;
+  worker_->fetched = true;
   SetIconFetched(ParamsForPrimaryIcon(params));
   SetIconFetched(ParamsForBadgeIcon(params));
 }
@@ -381,6 +420,12 @@ void InstallableManager::WorkOnTask() {
   if (!check_passed || IsComplete(params)) {
     RecordQueuedMetricsOnTaskCompletion(params, check_passed);
     RunCallback(task, code);
+
+    // Sites can always register a service worker after we finish checking, so
+    // don't cache a missing service worker error to ensure we always check
+    // again.
+    if (worker_error() == NO_MATCHING_SERVICE_WORKER)
+      worker_ = base::MakeUnique<ServiceWorkerProperty>();
     tasks_.erase(tasks_.begin());
     StartNextTask();
     return;
@@ -391,8 +436,10 @@ void InstallableManager::WorkOnTask() {
   } else if (params.fetch_valid_primary_icon &&
              !IsIconFetched(ParamsForPrimaryIcon(params))) {
     CheckAndFetchBestIcon(ParamsForPrimaryIcon(params));
-  } else if (params.check_installable && !installable_->fetched) {
+  } else if (params.check_installable && !valid_manifest_->fetched) {
     CheckInstallable();
+  } else if (params.check_installable && !worker_->fetched) {
+    CheckServiceWorker();
   } else if (params.fetch_valid_badge_icon &&
              !IsIconFetched(ParamsForBadgeIcon(params))) {
     CheckAndFetchBestIcon(ParamsForBadgeIcon(params));
@@ -431,33 +478,29 @@ void InstallableManager::OnDidGetManifest(const GURL& manifest_url,
 }
 
 void InstallableManager::CheckInstallable() {
-  DCHECK(!installable_->fetched);
+  DCHECK(!valid_manifest_->fetched);
   DCHECK(!manifest().IsEmpty());
 
-  if (IsManifestValidForWebApp(manifest())) {
-    CheckServiceWorker();
-  } else {
-    installable_->installable = false;
-    installable_->fetched = true;
-    WorkOnTask();
-  }
+  valid_manifest_->is_valid = IsManifestValidForWebApp(manifest());
+  valid_manifest_->fetched = true;
+  WorkOnTask();
 }
 
 bool InstallableManager::IsManifestValidForWebApp(
     const content::Manifest& manifest) {
   if (manifest.IsEmpty()) {
-    installable_->error = MANIFEST_EMPTY;
+    valid_manifest_->error = MANIFEST_EMPTY;
     return false;
   }
 
   if (!manifest.start_url.is_valid()) {
-    installable_->error = START_URL_NOT_VALID;
+    valid_manifest_->error = START_URL_NOT_VALID;
     return false;
   }
 
   if ((manifest.name.is_null() || manifest.name.string().empty()) &&
       (manifest.short_name.is_null() || manifest.short_name.string().empty())) {
-    installable_->error = MANIFEST_MISSING_NAME_OR_SHORT_NAME;
+    valid_manifest_->error = MANIFEST_MISSING_NAME_OR_SHORT_NAME;
     return false;
   }
 
@@ -466,12 +509,12 @@ bool InstallableManager::IsManifestValidForWebApp(
   // this check moot. See https://crbug.com/604390.
   if (manifest.display != blink::kWebDisplayModeStandalone &&
       manifest.display != blink::kWebDisplayModeFullscreen) {
-    installable_->error = MANIFEST_DISPLAY_NOT_SUPPORTED;
+    valid_manifest_->error = MANIFEST_DISPLAY_NOT_SUPPORTED;
     return false;
   }
 
   if (!DoesManifestContainRequiredIcon(manifest)) {
-    installable_->error = MANIFEST_MISSING_SUITABLE_ICON;
+    valid_manifest_->error = MANIFEST_MISSING_SUITABLE_ICON;
     return false;
   }
 
@@ -479,22 +522,14 @@ bool InstallableManager::IsManifestValidForWebApp(
 }
 
 void InstallableManager::CheckServiceWorker() {
-  DCHECK(!installable_->fetched);
+  DCHECK(!worker_->fetched);
   DCHECK(!manifest().IsEmpty());
   DCHECK(manifest().start_url.is_valid());
 
-  content::WebContents* web_contents = GetWebContents();
-
   // Check to see if there is a single service worker controlling this page
   // and the manifest's start url.
-  content::StoragePartition* storage_partition =
-      content::BrowserContext::GetStoragePartition(
-          Profile::FromBrowserContext(web_contents->GetBrowserContext()),
-          web_contents->GetSiteInstance());
-  DCHECK(storage_partition);
-
-  storage_partition->GetServiceWorkerContext()->CheckHasServiceWorker(
-      web_contents->GetLastCommittedURL(), manifest().start_url,
+  service_worker_context_->CheckHasServiceWorker(
+      GetWebContents()->GetLastCommittedURL(), manifest().start_url,
       base::Bind(&InstallableManager::OnDidCheckHasServiceWorker,
                  weak_factory_.GetWeakPtr()));
 }
@@ -506,19 +541,28 @@ void InstallableManager::OnDidCheckHasServiceWorker(
 
   switch (capability) {
     case content::ServiceWorkerCapability::SERVICE_WORKER_WITH_FETCH_HANDLER:
-      installable_->installable = true;
+      worker_->has_worker = true;
       break;
     case content::ServiceWorkerCapability::SERVICE_WORKER_NO_FETCH_HANDLER:
-      installable_->installable = false;
-      installable_->error = NOT_OFFLINE_CAPABLE;
+      worker_->has_worker = false;
+      worker_->error = NOT_OFFLINE_CAPABLE;
       break;
     case content::ServiceWorkerCapability::NO_SERVICE_WORKER:
-      installable_->installable = false;
-      installable_->error = NO_MATCHING_SERVICE_WORKER;
+      InstallableParams& params = tasks_[0].first;
+      if (params.wait_for_worker) {
+        // Wait for ServiceWorkerContextObserver::OnRegistrationStored. Set the
+        // param |wait_for_worker| to false so we only wait once per task.
+        params.wait_for_worker = false;
+        worker_->is_waiting = true;
+        OnWaitingForServiceWorker();
+        return;
+      }
+      worker_->has_worker = false;
+      worker_->error = NO_MATCHING_SERVICE_WORKER;
       break;
   }
 
-  installable_->fetched = true;
+  worker_->fetched = true;
   WorkOnTask();
 }
 
@@ -569,6 +613,22 @@ void InstallableManager::OnIconFetched(
   WorkOnTask();
 }
 
+void InstallableManager::OnRegistrationStored(const GURL& pattern) {
+  // If we aren't currently waiting, either:
+  //   a) we've already failed the check, or
+  //   b) we haven't yet called CheckHasServiceWorker.
+  // Otherwise if the scope doesn't match we keep waiting.
+  if (!worker_->is_waiting || !content::ServiceWorkerContext::ScopeMatches(
+                                  pattern, manifest().start_url)) {
+    return;
+  }
+
+  // This will call CheckHasServiceWorker to check whether the service worker
+  // controls the start_url and if it has a fetch handler.
+  worker_->is_waiting = false;
+  WorkOnTask();
+}
+
 void InstallableManager::DidFinishNavigation(
     content::NavigationHandle* handle) {
   if (handle->IsInMainFrame() && handle->HasCommitted() &&
@@ -591,5 +651,5 @@ const content::Manifest& InstallableManager::manifest() const {
 }
 
 bool InstallableManager::is_installable() const {
-  return installable_->installable;
+  return valid_manifest_->is_valid && worker_->has_worker;
 }