Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(6875)

Unified Diff: chrome/browser/installable/installable_manager.cc

Issue 2942513002: Allow banners to trigger on sites which don't register a service worker onload. (Closed)
Patch Set: Fix crashing Android test. Comments Created 3 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/installable/installable_manager.cc
diff --git a/chrome/browser/installable/installable_manager.cc b/chrome/browser/installable/installable_manager.cc
index a6d16ed16d76b633f176f256d50c4660ef6107cb..181ae50a443e8378710235f22ab168fad3a955d0 100644
--- a/chrome/browser/installable/installable_manager.cc
+++ b/chrome/browser/installable/installable_manager.cc
@@ -15,7 +15,6 @@
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.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,17 @@ 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;
}
InstallableStatusCode InstallableManager::icon_error(
@@ -289,11 +321,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 +350,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 +361,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));
}
@@ -391,8 +426,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 +468,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 +499,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 +512,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 +531,27 @@ 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;
+ return;
+ }
+ worker_->has_worker = false;
+ worker_->error = NO_MATCHING_SERVICE_WORKER;
break;
}
- installable_->fetched = true;
+ worker_->fetched = true;
WorkOnTask();
}
@@ -569,6 +602,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 +640,5 @@ const content::Manifest& InstallableManager::manifest() const {
}
bool InstallableManager::is_installable() const {
- return installable_->installable;
+ return valid_manifest_->is_valid && worker_->has_worker;
}

Powered by Google App Engine
This is Rietveld 408576698