Replace is_installable in InstallableManager with valid_manifest and has_worker.

This CL splits the two components of the 'installable' check into its two
components. This makes it clearer to clients what they are checking for
and removes the confusion that a check that returns 'installable' may
not actually be installable given the context of the check (e.g icons
may be required for PWA installability).

Further, the CL extracts the service worker wait behavior into a
required argument in order to make explicit the checking and callback
behavior. This prevents clients naively blocking forever on the callback
on sites which have no service worker, which was previously the default
behavior of InstallableParams.

Bug: None
Change-Id: I13b76d3d651e5ba9a9c1b7055bcc5f72689a2ea1
Reviewed-on: https://chromium-review.googlesource.com/704475
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508602}

chrome/browser/android/banners/app_banner_manager_android.cc

@@ -161,7 +161,7 @@ bool AppBannerManagerAndroid::IsWebAppInstalled(
 InstallableParams AppBannerManagerAndroid::ParamsToPerformInstallableCheck() {
   InstallableParams params =
       AppBannerManager::ParamsToPerformInstallableCheck();
-  params.fetch_valid_badge_icon = can_install_webapk_;
+  params.valid_badge_icon = can_install_webapk_;
 
   return params;
 }

chrome/browser/android/webapk/webapk_update_data_fetcher.cc

@@ -102,9 +102,11 @@ void WebApkUpdateDataFetcher::FetchInstallableData() {
     return;
 
   InstallableParams params;
-  params.check_installable = true;
-  params.fetch_valid_primary_icon = true;
-  params.fetch_valid_badge_icon = true;
+  params.valid_manifest = true;
+  params.has_worker = true;
+  params.valid_primary_icon = true;
+  params.valid_badge_icon = true;
+  params.wait_for_worker = true;
   InstallableManager* installable_manager =
       InstallableManager::FromWebContents(web_contents());
   installable_manager->GetData(

chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc

@@ -44,16 +44,19 @@ GURL GetShortcutUrl(const content::WebContents* web_contents) {
 InstallableParams ParamsToPerformManifestAndIconFetch(
     bool check_webapk_compatibility) {
   InstallableParams params;
-  params.fetch_valid_primary_icon = true;
-  params.fetch_valid_badge_icon = check_webapk_compatibility;
+  params.valid_primary_icon = true;
+  params.valid_badge_icon = check_webapk_compatibility;
+  params.wait_for_worker = true;
   return params;
 }
 
 InstallableParams ParamsToPerformInstallableCheck(
     bool check_webapk_compatibility) {
   InstallableParams params;
-  params.check_installable = check_webapk_compatibility;
-  params.fetch_valid_primary_icon = check_webapk_compatibility;
+  params.valid_manifest = check_webapk_compatibility;
+  params.has_worker = check_webapk_compatibility;
+  params.valid_primary_icon = check_webapk_compatibility;
+  params.wait_for_worker = true;
   return params;
 }
 
@@ -268,8 +271,8 @@ void AddToHomescreenDataFetcher::OnDidPerformInstallableCheck(
   bool webapk_compatible = false;
   if (check_webapk_compatibility_) {
     webapk_compatible =
-        (data.error_code == NO_ERROR_DETECTED && data.is_installable &&
-         AreWebManifestUrlsWebApkCompatible(data.manifest));
+        (data.error_code == NO_ERROR_DETECTED && data.valid_manifest &&
+         data.has_worker && AreWebManifestUrlsWebApkCompatible(data.manifest));
     observer_->OnDidDetermineWebApkCompatibility(webapk_compatible);
   }
 

chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc

@@ -146,10 +146,10 @@ class TestInstallableManager : public InstallableManager {
 
     InstallableStatusCode code = NO_ERROR_DETECTED;
     bool is_installable = is_installable_;
-    if (params.fetch_valid_primary_icon && !primary_icon_) {
+    if (params.valid_primary_icon && !primary_icon_) {
       code = NO_ACCEPTABLE_ICON;
       is_installable = false;
-    } else if (params.check_installable) {
+    } else if (params.valid_manifest && params.has_worker) {
       if (!IsManifestValidForWebApp(manifest_)) {
         code = valid_manifest_->error;
         is_installable = false;
@@ -160,7 +160,8 @@ class TestInstallableManager : public InstallableManager {
     }
 
     if (should_manifest_time_out_ ||
-        (params.check_installable && should_installable_time_out_)) {
+        (params.valid_manifest && params.has_worker &&
+         should_installable_time_out_)) {
       // Bind the metrics resolution callback. We want to test when this is
       // and isn't called (corresponding to InstallableManager finishing work
       // after the timeout, and when it never finishes at all).
@@ -171,16 +172,16 @@ class TestInstallableManager : public InstallableManager {
     }
 
     // Otherwise, directly call the metrics finalisation.
-    if (params.check_installable && is_installable)
+    if (params.valid_manifest && params.has_worker && is_installable)
       ResolveMetrics(params, is_installable);
 
-    callback.Run(
-        {code, GURL(kDefaultManifestUrl), manifest_,
-         params.fetch_valid_primary_icon ? primary_icon_url_ : GURL(),
-         params.fetch_valid_primary_icon ? primary_icon_.get() : nullptr,
-         params.fetch_valid_badge_icon ? badge_icon_url_ : GURL(),
-         params.fetch_valid_badge_icon ? badge_icon_.get() : nullptr,
-         params.check_installable ? is_installable : false});
+    callback.Run({code, GURL(kDefaultManifestUrl), manifest_,
+                  params.valid_primary_icon ? primary_icon_url_ : GURL(),
+                  params.valid_primary_icon ? primary_icon_.get() : nullptr,
+                  params.valid_badge_icon ? badge_icon_url_ : GURL(),
+                  params.valid_badge_icon ? badge_icon_.get() : nullptr,
+                  params.valid_manifest ? is_installable : false,
+                  params.has_worker ? is_installable : false});
   }
 
   void SetInstallable(bool is_installable) { is_installable_ = is_installable; }

chrome/browser/banners/app_banner_manager.cc

@@ -185,9 +185,9 @@ void AppBannerManager::OnDidGetManifest(const InstallableData& data) {
 
 InstallableParams AppBannerManager::ParamsToPerformInstallableCheck() {
   InstallableParams params;
-  params.check_installable = true;
-  params.fetch_valid_primary_icon = true;
-
+  params.valid_primary_icon = true;
+  params.valid_manifest = true;
+  params.has_worker = true;
   // Don't wait for the service worker if this was triggered from devtools.
   params.wait_for_worker = !triggered_by_devtools_;
 
@@ -208,7 +208,7 @@ void AppBannerManager::PerformInstallableCheck() {
 void AppBannerManager::OnDidPerformInstallableCheck(
     const InstallableData& data) {
   UpdateState(State::ACTIVE);
-  if (data.is_installable)
+  if (data.has_worker && data.valid_manifest)
     TrackDisplayEvent(DISPLAY_EVENT_WEB_APP_BANNER_REQUESTED);
 
   if (data.error_code != NO_ERROR_DETECTED) {
@@ -219,7 +219,7 @@ void AppBannerManager::OnDidPerformInstallableCheck(
     return;
   }
 
-  DCHECK(data.is_installable);
+  DCHECK(data.has_worker && data.valid_manifest);
   DCHECK(!data.primary_icon_url.is_empty());
   DCHECK(data.primary_icon);
 

chrome/browser/installable/installable_data.h

@@ -30,8 +30,8 @@ struct InstallableData {
   // nullptr if the most appropriate primary icon couldn't be determined or
   // downloaded. The underlying primary icon is owned by the InstallableManager;
   // clients must copy the bitmap if they want to to use it. If
-  // fetch_valid_primary_icon was true and a primary icon could not be
-  // retrieved, the reason will be in error_code.
+  // valid_primary_icon was true and a primary icon could not be retrieved, the
+  // reason will be in error_code.
   const SkBitmap* primary_icon;
 
   // Empty if no badge_icon was requested.
@@ -41,14 +41,18 @@ struct InstallableData {
   // downloaded. The underlying badge icon is owned by the InstallableManager;
   // clients must copy the bitmap if they want to to use it. Since the badge
   // icon is optional, no error code is set if it cannot be fetched, and clients
-  // specifying fetch_valid_badge_icon must check that the bitmap exists before
-  // using it.
+  // specifying valid_badge_icon must check that the bitmap exists before using
+  // it.
   const SkBitmap* badge_icon;
 
-  // true if the site has a service worker with a fetch handler and a viable web
-  // app manifest. If check_installable was true and the site isn't installable,
-  // the reason will be in error_code.
-  const bool is_installable;
+  // true if the site has a viable web app manifest. If valid_manifest or
+  // has_worker was true and the site isn't installable, the reason will be in
+  // error_code.
+  const bool valid_manifest;
+
+  // true if the site has a service worker with a fetch handler. If has_worker
+  // was true and the site isn't installable, the reason will be in error_code.
+  const bool has_worker;
 };
 
 using InstallableCallback = base::Callback<void(const InstallableData&)>;

chrome/browser/installable/installable_manager.cc

@@ -96,7 +96,8 @@ bool DoesManifestContainRequiredIcon(const content::Manifest& manifest) {
 
 // Returns true if |params| specifies a full PWA check.
 bool IsParamsForPwaCheck(const InstallableParams& params) {
-  return params.check_installable && params.fetch_valid_primary_icon;
+  return params.valid_manifest && params.has_worker &&
+         params.valid_primary_icon;
 }
 
 // Used for a no-op call to GetData.
@@ -205,8 +206,10 @@ void InstallableManager::RecordAddToHomescreenManifestAndIconTimeout() {
   // not a site is a PWA, assuming that the check finishes prior to resetting.
   if (!has_pwa_check_) {
     InstallableParams params;
-    params.check_installable = true;
-    params.fetch_valid_primary_icon = true;
+    params.valid_manifest = true;
+    params.has_worker = true;
+    params.valid_primary_icon = true;
+    params.wait_for_worker = true;
     GetData(params, base::Bind(&DoNothingCallback));
   }
 }
@@ -232,20 +235,19 @@ InstallableStatusCode InstallableManager::GetErrorCode(
   if (manifest_->error != NO_ERROR_DETECTED)
     return manifest_->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.valid_manifest && valid_manifest_->error != NO_ERROR_DETECTED)
+    return valid_manifest_->error;
+
+  if (params.has_worker && worker_->error != NO_ERROR_DETECTED)
+    return worker_->error;
 
-  if (params.fetch_valid_primary_icon) {
+  if (params.valid_primary_icon) {
     IconProperty& icon = icons_[IconPurpose::ANY];
     if (icon.error != NO_ERROR_DETECTED)
       return icon.error;
   }
 
-  if (params.fetch_valid_badge_icon) {
+  if (params.valid_badge_icon) {
     IconProperty& icon = icons_[IconPurpose::BADGE];
 
     // If the error is NO_ACCEPTABLE_ICON, there is no icon suitable as a badge
@@ -305,11 +307,10 @@ bool InstallableManager::IsComplete(const InstallableParams& params) const {
   //  b. the resource has been fetched/checked.
   return (!params.check_eligibility || eligibility_->fetched) &&
          manifest_->fetched &&
-         (!params.check_installable ||
-          (valid_manifest_->fetched && worker_->fetched)) &&
-         (!params.fetch_valid_primary_icon ||
-          IsIconFetched(IconPurpose::ANY)) &&
-         (!params.fetch_valid_badge_icon || IsIconFetched(IconPurpose::BADGE));
+         (!params.valid_manifest || valid_manifest_->fetched) &&
+         (!params.has_worker || worker_->fetched) &&
+         (!params.valid_primary_icon || IsIconFetched(IconPurpose::ANY)) &&
+         (!params.valid_badge_icon || IsIconFetched(IconPurpose::BADGE));
 }
 
 void InstallableManager::ResolveMetrics(const InstallableParams& params,
@@ -349,16 +350,14 @@ void InstallableManager::SetManifestDependentTasksComplete() {
 
 void InstallableManager::RunCallback(const InstallableTask& task,
                                      InstallableStatusCode code) {
-  const InstallableParams& params = task.first;
+  const InstallableParams& params = task.params;
   IconProperty null_icon;
   IconProperty* primary_icon = &null_icon;
   IconProperty* badge_icon = &null_icon;
 
-  if (params.fetch_valid_primary_icon &&
-      base::ContainsKey(icons_, IconPurpose::ANY))
+  if (params.valid_primary_icon && base::ContainsKey(icons_, IconPurpose::ANY))
     primary_icon = &icons_[IconPurpose::ANY];
-  if (params.fetch_valid_badge_icon &&
-      base::ContainsKey(icons_, IconPurpose::BADGE))
+  if (params.valid_badge_icon && base::ContainsKey(icons_, IconPurpose::BADGE))
     badge_icon = &icons_[IconPurpose::BADGE];
 
   InstallableData data = {
@@ -369,14 +368,16 @@ void InstallableManager::RunCallback(const InstallableTask& task,
       primary_icon->icon.get(),
       badge_icon->url,
       badge_icon->icon.get(),
-      params.check_installable ? is_installable() : false};
+      valid_manifest_->is_valid,
+      worker_->has_worker,
+  };
 
-  task.second.Run(data);
+  task.callback.Run(data);
 }
 
 void InstallableManager::WorkOnTask() {
   const InstallableTask& task = task_queue_.Current();
-  const InstallableParams& params = task.first;
+  const InstallableParams& params = task.params;
 
   InstallableStatusCode code = GetErrorCode(params);
   bool check_passed = (code == NO_ERROR_DETECTED);
@@ -402,16 +403,14 @@ void InstallableManager::WorkOnTask() {
     CheckEligiblity();
   } else if (!manifest_->fetched) {
     FetchManifest();
-  } else if (params.fetch_valid_primary_icon &&
-             !IsIconFetched(IconPurpose::ANY)) {
+  } else if (params.valid_primary_icon && !IsIconFetched(IconPurpose::ANY)) {
     CheckAndFetchBestIcon(GetIdealPrimaryIconSizeInPx(),
                           GetMinimumPrimaryIconSizeInPx(), IconPurpose::ANY);
-  } else if (params.check_installable && !valid_manifest_->fetched) {
-    CheckInstallable();
-  } else if (params.check_installable && !worker_->fetched) {
+  } else if (params.valid_manifest && !valid_manifest_->fetched) {
+    CheckManifestValid();
+  } else if (params.has_worker && !worker_->fetched) {
     CheckServiceWorker();
-  } else if (params.fetch_valid_badge_icon &&
-             !IsIconFetched(IconPurpose::BADGE)) {
+  } else if (params.valid_badge_icon && !IsIconFetched(IconPurpose::BADGE)) {
     CheckAndFetchBestIcon(GetIdealBadgeIconSizeInPx(),
                           GetIdealBadgeIconSizeInPx(), IconPurpose::BADGE);
   } else {
@@ -464,7 +463,7 @@ void InstallableManager::OnDidGetManifest(const GURL& manifest_url,
   WorkOnTask();
 }
 
-void InstallableManager::CheckInstallable() {
+void InstallableManager::CheckManifestValid() {
   DCHECK(!valid_manifest_->fetched);
   DCHECK(!manifest().IsEmpty());
 
@@ -535,11 +534,10 @@ void InstallableManager::OnDidCheckHasServiceWorker(
       break;
     case content::ServiceWorkerCapability::NO_SERVICE_WORKER:
       InstallableTask& task = task_queue_.Current();
-      InstallableParams& params = task.first;
-      if (params.wait_for_worker) {
+      if (task.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;
+        task.params.wait_for_worker = false;
         OnWaitingForServiceWorker();
         task_queue_.PauseCurrent();
         if (task_queue_.HasCurrent())
@@ -647,6 +645,10 @@ const content::Manifest& InstallableManager::manifest() const {
   return manifest_->manifest;
 }
 
-bool InstallableManager::is_installable() const {
-  return valid_manifest_->is_valid && worker_->has_worker;
+bool InstallableManager::valid_manifest() {
+  return valid_manifest_->is_valid;
+}
+
+bool InstallableManager::has_worker() {
+  return worker_->has_worker;
 }

chrome/browser/installable/installable_manager.h

@@ -180,7 +180,7 @@ class InstallableManager
   void OnDidGetManifest(const GURL& manifest_url,
                         const content::Manifest& manifest);
 
-  void CheckInstallable();
+  void CheckManifestValid();
   bool IsManifestValidForWebApp(const content::Manifest& manifest);
   void CheckServiceWorker();
   void OnDidCheckHasServiceWorker(content::ServiceWorkerCapability capability);
@@ -201,7 +201,8 @@ class InstallableManager
 
   const GURL& manifest_url() const;
   const content::Manifest& manifest() const;
-  bool is_installable() const;
+  bool valid_manifest();
+  bool has_worker();
 
   InstallableTaskQueue task_queue_;
   std::unique_ptr<InstallableMetrics> metrics_;

chrome/browser/installable/installable_params.h

@@ -18,21 +18,25 @@ struct InstallableParams {
 
   // 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;
+  bool 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;
+  bool valid_badge_icon = false;
 
-  // 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;
+  // Check whether the site has a manifest valid for a web app.
+  bool valid_manifest = false;
+
+  // Check whether the site has a service worker controlling the manifest start
+  // URL and the current URL.
+  bool has_worker = false;
 
   // Whether or not to wait indefinitely for a service worker. If this is set to
   // false, the worker status will not be cached and will be re-checked if
-  // GetData() is called again for the current page.
-  bool wait_for_worker = true;
+  // GetData() is called again for the current page. Setting this to true means
+  // that the callback will not be called for any site that does not install a
+  // service worker.
+  bool wait_for_worker = false;
 };
 
 #endif  // CHROME_BROWSER_INSTALLABLE_INSTALLABLE_PARAMS_H_

chrome/browser/installable/installable_task_queue.cc

@@ -4,6 +4,15 @@
 
 #include "chrome/browser/installable/installable_task_queue.h"
 
+InstallableTask::InstallableTask() {}
+InstallableTask::InstallableTask(const InstallableParams& params,
+                                 const InstallableCallback& callback)
+    : params(params), callback(callback) {}
+InstallableTask::~InstallableTask() {}
+InstallableTask::InstallableTask(const InstallableTask& other) = default;
+InstallableTask& InstallableTask::operator=(const InstallableTask& other) =
+    default;
+
 InstallableTaskQueue::InstallableTaskQueue() {}
 InstallableTaskQueue::~InstallableTaskQueue() {}
 

chrome/browser/installable/installable_task_queue.h

@@ -11,7 +11,18 @@
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 
-using InstallableTask = std::pair<InstallableParams, InstallableCallback>;
+struct InstallableTask {
+  InstallableTask();
+  InstallableTask(const InstallableParams& params,
+                  const InstallableCallback& callback);
+  InstallableTask(const InstallableTask& other);
+  ~InstallableTask();
+
+  InstallableTask& operator=(const InstallableTask& other);
+
+  InstallableParams params;
+  InstallableCallback callback;
+};
 
 // InstallableTaskQueue keeps track of pending tasks.
 class InstallableTaskQueue {

chrome/browser/installable/installable_task_queue_unittest.cc

@@ -10,28 +10,29 @@ using IconPurpose = content::Manifest::Icon::IconPurpose;
 class InstallableTaskQueueUnitTest : public testing::Test {};
 
 // Constructs an InstallableTask, with the supplied bools stored in it.
-InstallableTask CreateTask(bool check_installable,
-                           bool fetch_valid_primary_icon,
-                           bool fetch_valid_badge_icon) {
+InstallableTask CreateTask(bool valid_manifest,
+                           bool has_worker,
+                           bool valid_primary_icon,
+                           bool valid_badge_icon) {
   InstallableTask task;
-  task.first.check_installable = check_installable;
-  task.first.fetch_valid_primary_icon = fetch_valid_primary_icon;
-  task.first.fetch_valid_badge_icon = fetch_valid_badge_icon;
+  task.params.valid_manifest = valid_manifest;
+  task.params.has_worker = has_worker;
+  task.params.valid_primary_icon = valid_primary_icon;
+  task.params.valid_badge_icon = valid_badge_icon;
   return task;
 }
 
 bool IsEqual(const InstallableTask& task1, const InstallableTask& task2) {
-  return task1.first.check_installable == task2.first.check_installable &&
-         task1.first.fetch_valid_primary_icon ==
-             task2.first.fetch_valid_primary_icon &&
-         task1.first.fetch_valid_badge_icon ==
-             task2.first.fetch_valid_badge_icon;
+  return task1.params.valid_manifest == task2.params.valid_manifest &&
+         task1.params.has_worker == task2.params.has_worker &&
+         task1.params.valid_primary_icon == task2.params.valid_primary_icon &&
+         task1.params.valid_badge_icon == task2.params.valid_badge_icon;
 }
 
 TEST_F(InstallableTaskQueueUnitTest, PausingMakesNextTaskAvailable) {
   InstallableTaskQueue task_queue;
-  InstallableTask task1 = CreateTask(false, false, false);
-  InstallableTask task2 = CreateTask(true, true, true);
+  InstallableTask task1 = CreateTask(false, false, false, false);
+  InstallableTask task2 = CreateTask(true, true, true, true);
 
   task_queue.Add(task1);
   task_queue.Add(task2);
@@ -44,8 +45,8 @@ TEST_F(InstallableTaskQueueUnitTest, PausingMakesNextTaskAvailable) {
 
 TEST_F(InstallableTaskQueueUnitTest, PausedTaskCanBeRetrieved) {
   InstallableTaskQueue task_queue;
-  InstallableTask task1 = CreateTask(false, false, false);
-  InstallableTask task2 = CreateTask(true, true, true);
+  InstallableTask task1 = CreateTask(false, false, false, false);
+  InstallableTask task2 = CreateTask(true, true, true, true);
 
   task_queue.Add(task1);
   task_queue.Add(task2);
@@ -62,8 +63,8 @@ TEST_F(InstallableTaskQueueUnitTest, PausedTaskCanBeRetrieved) {
 
 TEST_F(InstallableTaskQueueUnitTest, NextDiscardsTask) {
   InstallableTaskQueue task_queue;
-  InstallableTask task1 = CreateTask(false, false, false);
-  InstallableTask task2 = CreateTask(true, true, true);
+  InstallableTask task1 = CreateTask(false, false, false, false);
+  InstallableTask task2 = CreateTask(true, true, true, true);
 
   task_queue.Add(task1);
   task_queue.Add(task2);