ServiceWorker: DB functions should return status code instead of boolean (3)

Part1: https://codereview.chromium.org/275103004/
Part2: https://codereview.chromium.org/287843002/

This change makes all remaining functions return status code instead of boolean.


BUG=372704
TEST=should pass all existing tests

Review URL: https://codereview.chromium.org/284123003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271342 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/service_worker/service_worker_database.h

@@ -97,20 +97,44 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
   // (resource ids will be added/removed from the uncommitted/purgeable
   // lists as needed)
 
-  bool ReadRegistration(int64 registration_id,
-                        const GURL& origin,
-                        RegistrationData* registration,
-                        std::vector<ResourceRecord>* resources);
-  bool WriteRegistration(const RegistrationData& registration,
-                         const std::vector<ResourceRecord>& resources);
-
-  bool UpdateVersionToActive(int64 registration_id,
-                             const GURL& origin);
-  bool UpdateLastCheckTime(int64 registration_id,
-                           const GURL& origin,
-                           const base::Time& time);
-  bool DeleteRegistration(int64 registration_id,
-                          const GURL& origin);
+  // Reads a registration for |registration_id| and resource records associated
+  // with it from the database. Returns OK if they are successfully read.
+  // Otherwise, returns an error.
+  Status ReadRegistration(
+      int64 registration_id,
+      const GURL& origin,
+      RegistrationData* registration,
+      std::vector<ResourceRecord>* resources);
+
+  // Writes |registration| and |resources| into the database and does following
+  // things:
+  //   - Deletes an old version of the registration if exists.
+  //   - Bumps the next registration id and the next version id if needed.
+  //   - Removes |resources| from the uncommitted list if exist.
+  // Returns OK they are successfully written. Otherwise, returns an error.
+  Status WriteRegistration(
+      const RegistrationData& registration,
+      const std::vector<ResourceRecord>& resources);
+
+  // Updates a registration for |registration_id| to an active state. Returns OK
+  // if it's successfully updated. Otherwise, returns an error.
+  Status UpdateVersionToActive(
+      int64 registration_id,
+      const GURL& origin);
+
+  // Updates last check time of a registration for |registration_id| by |time|.
+  // Returns OK if it's successfully updated. Otherwise, returns an error.
+  Status UpdateLastCheckTime(
+      int64 registration_id,
+      const GURL& origin,
+      const base::Time& time);
+
+  // Deletes a registration for |registration_id| and moves resource records
+  // associated with it into the purgeable list. Returns OK if it's successfully
+  // deleted or not found in the database. Otherwise, returns an error.
+  Status DeleteRegistration(
+      int64 registration_id,
+      const GURL& origin);
 
   // As new resources are put into the diskcache, they go into an uncommitted
   // list. When a registration is saved that refers to those ids, they're
@@ -119,33 +143,35 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
   // the purgeable list can be purged from the diskcache. At system startup, all
   // uncommitted ids are moved to the purgeable list.
 
-  // Reads uncommitted resource ids from the database. Returns true on success.
-  // Otherwise clears |ids| and returns false.
-  bool GetUncommittedResourceIds(std::set<int64>* ids);
+  // Reads uncommitted resource ids from the database. Returns OK on success.
+  // Otherwise clears |ids| and returns an error.
+  Status GetUncommittedResourceIds(std::set<int64>* ids);
 
-  // Writes |ids| into the database as uncommitted resources. Returns true on
-  // success. Otherwise writes nothing and returns false.
-  bool WriteUncommittedResourceIds(const std::set<int64>& ids);
+  // Writes |ids| into the database as uncommitted resources. Returns OK on
+  // success. Otherwise writes nothing and returns an error.
+  Status WriteUncommittedResourceIds(const std::set<int64>& ids);
 
   // Deletes uncommitted resource ids specified by |ids| from the database.
-  // Returns true on success. Otherwise deletes nothing and returns false.
-  bool ClearUncommittedResourceIds(const std::set<int64>& ids);
+  // Returns OK on success. Otherwise deletes nothing and returns an error.
+  Status ClearUncommittedResourceIds(const std::set<int64>& ids);
 
-  // Reads purgeable resource ids from the database. Returns true on success.
-  // Otherwise clears |ids| and returns false.
-  bool GetPurgeableResourceIds(std::set<int64>* ids);
+  // Reads purgeable resource ids from the database. Returns OK on success.
+  // Otherwise clears |ids| and returns an error.
+  Status GetPurgeableResourceIds(std::set<int64>* ids);
 
-  // Writes |ids| into the database as purgeable resources. Returns true on
-  // success. Otherwise writes nothing and returns false.
-  bool WritePurgeableResourceIds(const std::set<int64>& ids);
+  // Writes |ids| into the database as purgeable resources. Returns OK on
+  // success. Otherwise writes nothing and returns an error.
+  Status WritePurgeableResourceIds(const std::set<int64>& ids);
 
   // Deletes purgeable resource ids specified by |ids| from the database.
-  // Returns true on success. Otherwise deletes nothing and returns false.
-  bool ClearPurgeableResourceIds(const std::set<int64>& ids);
+  // Returns OK on success. Otherwise deletes nothing and returns an error.
+  Status ClearPurgeableResourceIds(const std::set<int64>& ids);
 
-  // Delete all data for |origin|, namely, unique origin, registrations and
-  // resource records. Resources are moved to the purgeable list.
-  bool DeleteAllDataForOrigin(const GURL& origin);
+  // Deletes all data for |origin|, namely, unique origin, registrations and
+  // resource records. Resources are moved to the purgeable list. Returns OK if
+  // they are successfully deleted or not found in the database. Otherwise,
+  // returns an error.
+  Status DeleteAllDataForOrigin(const GURL& origin);
 
   bool is_disabled() const { return is_disabled_; }
   bool was_corruption_detected() const { return was_corruption_detected_; }
@@ -169,40 +195,70 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
       const char* id_key,
       int64* next_avail_id);
 
-  bool ReadRegistrationData(int64 registration_id,
-                            const GURL& origin,
-                            RegistrationData* registration);
-  bool ReadResourceRecords(int64 version_id,
-                           std::vector<ResourceRecord>* resources);
-  bool DeleteResourceRecords(int64 version_id,
-                             leveldb::WriteBatch* batch);
-  bool ReadResourceIds(const char* id_key_prefix,
-                       std::set<int64>* ids);
-  bool WriteResourceIds(const char* id_key_prefix,
-                        const std::set<int64>& ids);
-  bool DeleteResourceIds(const char* id_key_prefix,
-                         const std::set<int64>& ids);
+  // Reads registration data for |registration_id| from the database. Returns OK
+  // if successfully reads. Otherwise, returns an error.
+  Status ReadRegistrationData(
+      int64 registration_id,
+      const GURL& origin,
+      RegistrationData* registration);
+
+  // Reads resource records for |version_id| from the database. Returns OK if
+  // it's successfully read or not found in the database. Otherwise, returns an
+  // error.
+  Status ReadResourceRecords(
+      int64 version_id,
+      std::vector<ResourceRecord>* resources);
+
+  // Deletes resource records for |version_id| from the database. Returns OK if
+  // they are successfully deleted or not found in the database. Otherwise,
+  // returns an error.
+  Status DeleteResourceRecords(
+      int64 version_id,
+      leveldb::WriteBatch* batch);
+
+  // Reads resource ids for |id_key_prefix| from the database. Returns OK if
+  // it's successfully read or not found in the database. Otherwise, returns an
+  // error.
+  Status ReadResourceIds(
+      const char* id_key_prefix,
+      std::set<int64>* ids);
+
+  // Write resource ids for |id_key_prefix| into the database. Returns OK on
+  // success. Otherwise, returns writes nothing and returns an error.
+  Status WriteResourceIds(
+      const char* id_key_prefix,
+      const std::set<int64>& ids);
+
+  // Deletes resource ids for |id_key_prefix| from the database. Returns OK if
+  // it's successfully deleted or not found in the database. Otherwise, returns
+  // an error.
+  Status DeleteResourceIds(
+      const char* id_key_prefix,
+      const std::set<int64>& ids);
 
   // Reads the current schema version from the database. If the database hasn't
   // been written anything yet, sets |db_version| to 0 and returns OK.
   Status ReadDatabaseVersion(int64* db_version);
 
-  // Write a batch into the database.
+  // Writes a batch into the database.
   // NOTE: You must call this when you want to put something into the database
   // because this initializes the database if needed.
-  bool WriteBatch(leveldb::WriteBatch* batch);
+  Status WriteBatch(leveldb::WriteBatch* batch);
 
   // Bumps the next available id if |used_id| is greater than or equal to the
   // cached one.
-  void BumpNextRegistrationIdIfNeeded(int64 used_id,
-                                      leveldb::WriteBatch* batch);
-  void BumpNextVersionIdIfNeeded(int64 used_id,
-                                 leveldb::WriteBatch* batch);
+  void BumpNextRegistrationIdIfNeeded(
+      int64 used_id,
+      leveldb::WriteBatch* batch);
+  void BumpNextVersionIdIfNeeded(
+      int64 used_id,
+      leveldb::WriteBatch* batch);
 
   bool IsOpen();
 
-  void HandleError(const tracked_objects::Location& from_here,
-                   const leveldb::Status& status);
+  void HandleError(
+      const tracked_objects::Location& from_here,
+      const leveldb::Status& status);
 
   base::FilePath path_;
   scoped_ptr<leveldb::Env> env_;

content/browser/service_worker/service_worker_storage.cc

@@ -100,28 +100,6 @@ void ReadInitialDataFromDB(
       FROM_HERE, base::Bind(callback, base::Owned(data.release()), status));
 }
 
-void ReadRegistrationFromDB(
-    ServiceWorkerDatabase* database,
-    scoped_refptr<base::SequencedTaskRunner> original_task_runner,
-    int64 registration_id,
-    const GURL& origin,
-    const ReadRegistrationCallback& callback) {
-  DCHECK(database);
-  ServiceWorkerDatabase::RegistrationData data;
-  std::vector<ServiceWorkerDatabase::ResourceRecord> resources;
-
-  // TODO(nhiroki): The database should return more detailed status like
-  // ServiceWorkerStatusCode instead of bool value.
-  ServiceWorkerDatabase::Status status = ServiceWorkerDatabase::STATUS_OK;
-  if (!database->ReadRegistration(registration_id, origin, &data, &resources)) {
-    status = database->is_disabled()
-        ? ServiceWorkerDatabase::STATUS_ERROR_FAILED
-        : ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND;
-  }
-  original_task_runner->PostTask(
-      FROM_HERE, base::Bind(callback, data, resources, status));
-}
-
 void DeleteRegistrationFromDB(
     ServiceWorkerDatabase* database,
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
@@ -129,18 +107,18 @@ void DeleteRegistrationFromDB(
     const GURL& origin,
     const DeleteRegistrationCallback& callback) {
   DCHECK(database);
-  if (!database->DeleteRegistration(registration_id, origin)) {
+  ServiceWorkerDatabase::Status status =
+      database->DeleteRegistration(registration_id, origin);
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
     original_task_runner->PostTask(
-        FROM_HERE, base::Bind(callback, false,
-                              ServiceWorkerDatabase::STATUS_ERROR_FAILED));
+        FROM_HERE, base::Bind(callback, false, status));
     return;
   }
 
   // TODO(nhiroki): Add convenient method to ServiceWorkerDatabase to check the
   // unique origin list.
   std::vector<ServiceWorkerDatabase::RegistrationData> registrations;
-  ServiceWorkerDatabase::Status status =
-      database->GetRegistrationsForOrigin(origin, &registrations);
+  status = database->GetRegistrationsForOrigin(origin, &registrations);
   if (status != ServiceWorkerDatabase::STATUS_OK) {
     original_task_runner->PostTask(
         FROM_HERE, base::Bind(callback, false, status));
@@ -149,26 +127,7 @@ void DeleteRegistrationFromDB(
 
   bool deletable = registrations.empty();
   original_task_runner->PostTask(
-      FROM_HERE, base::Bind(callback, deletable,
-                            ServiceWorkerDatabase::STATUS_OK));
-}
-
-void UpdateToActiveStateInDB(
-    ServiceWorkerDatabase* database,
-    scoped_refptr<base::SequencedTaskRunner> original_task_runner,
-    int64 registration_id,
-    const GURL& origin,
-    const ServiceWorkerStorage::StatusCallback& callback) {
-  DCHECK(database);
-
-  // TODO(nhiroki): The database should return more detailed status like
-  // ServiceWorkerStatusCode instead of bool value.
-  ServiceWorkerStatusCode status = SERVICE_WORKER_OK;
-  if (!database->UpdateVersionToActive(registration_id, origin)) {
-    status = database->is_disabled() ? SERVICE_WORKER_ERROR_FAILED
-                                     : SERVICE_WORKER_ERROR_NOT_FOUND;
-  }
-  original_task_runner->PostTask(FROM_HERE, base::Bind(callback, status));
+      FROM_HERE, base::Bind(callback, deletable, status));
 }
 
 }  // namespace
@@ -327,14 +286,20 @@ void ServiceWorkerStorage::FindRegistrationForId(
     return;
   }
 
-  database_task_runner_->PostTask(
+  ServiceWorkerDatabase::RegistrationData* data =
+      new ServiceWorkerDatabase::RegistrationData;
+  ResourceList* resources = new ResourceList;
+  PostTaskAndReplyWithResult(
+      database_task_runner_,
       FROM_HERE,
-      base::Bind(&ReadRegistrationFromDB,
-                 database_.get(),
-                 base::MessageLoopProxy::current(),
+      base::Bind(&ServiceWorkerDatabase::ReadRegistration,
+                 base::Unretained(database_.get()),
                  registration_id, origin,
-                 base::Bind(&ServiceWorkerStorage::DidReadRegistrationForId,
-                            weak_factory_.GetWeakPtr(), callback)));
+                 base::Unretained(data),
+                 base::Unretained(resources)),
+      base::Bind(&ServiceWorkerStorage::DidReadRegistrationForId,
+                 weak_factory_.GetWeakPtr(),
+                 callback, base::Owned(data), base::Owned(resources)));
 }
 
 void ServiceWorkerStorage::GetAllRegistrations(
@@ -408,13 +373,15 @@ void ServiceWorkerStorage::UpdateToActiveState(
     return;
   }
 
-  database_task_runner_->PostTask(
+  PostTaskAndReplyWithResult(
+      database_task_runner_,
       FROM_HERE,
-      base::Bind(&UpdateToActiveStateInDB,
-                 database_.get(),
-                 base::MessageLoopProxy::current(),
+      base::Bind(&ServiceWorkerDatabase::UpdateVersionToActive,
+                 base::Unretained(database_.get()),
                  registration->id(),
-                 registration->script_url().GetOrigin(),
+                 registration->script_url().GetOrigin()),
+      base::Bind(&ServiceWorkerStorage::DidUpdateToActiveState,
+                 weak_factory_.GetWeakPtr(),
                  callback));
 }
 
@@ -624,18 +591,21 @@ void ServiceWorkerStorage::DidGetRegistrationsForDocument(
 
 void ServiceWorkerStorage::DidReadRegistrationForId(
     const FindRegistrationCallback& callback,
-    const ServiceWorkerDatabase::RegistrationData& registration,
-    const ResourceList& resources,
+    ServiceWorkerDatabase::RegistrationData* registration,
+    ResourceList* resources,
     ServiceWorkerDatabase::Status status) {
+  DCHECK(registration);
+  DCHECK(resources);
+
   if (status == ServiceWorkerDatabase::STATUS_OK) {
-    callback.Run(SERVICE_WORKER_OK, CreateRegistration(registration));
+    callback.Run(SERVICE_WORKER_OK, CreateRegistration(*registration));
     return;
   }
 
   if (status == ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND) {
     // Look for somthing currently being installed.
     scoped_refptr<ServiceWorkerRegistration> installing_registration =
-        FindInstallingRegistrationForId(registration.registration_id);
+        FindInstallingRegistrationForId(registration->registration_id);
     if (installing_registration) {
       callback.Run(SERVICE_WORKER_OK, installing_registration);
       return;
@@ -645,6 +615,7 @@ void ServiceWorkerStorage::DidReadRegistrationForId(
     return;
   }
 
+  // TODO(nhiroki): Handle database error (http://crbug.com/371675).
   callback.Run(DatabaseStatusToStatusCode(status),
                scoped_refptr<ServiceWorkerRegistration>());
   return;
@@ -699,23 +670,36 @@ void ServiceWorkerStorage::DidGetAllRegistrations(
 void ServiceWorkerStorage::DidStoreRegistration(
     const GURL& origin,
     const StatusCallback& callback,
-    bool success) {
-  if (!success) {
-    callback.Run(SERVICE_WORKER_ERROR_FAILED);
+    ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    // TODO(nhiroki): Handle database error (http://crbug.com/371675).
+    callback.Run(DatabaseStatusToStatusCode(status));
     return;
   }
   registered_origins_.insert(origin);
   callback.Run(SERVICE_WORKER_OK);
 }
 
+void ServiceWorkerStorage::DidUpdateToActiveState(
+    const StatusCallback& callback,
+    ServiceWorkerDatabase::Status status) {
+  // TODO(nhiroki): Handle database error (http://crbug.com/371675).
+  callback.Run(DatabaseStatusToStatusCode(status));
+}
+
 void ServiceWorkerStorage::DidDeleteRegistration(
     const GURL& origin,
     const StatusCallback& callback,
     bool origin_is_deletable,
     ServiceWorkerDatabase::Status status) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    // TODO(nhiroki): Handle database error (http://crbug.com/371675).
+    callback.Run(DatabaseStatusToStatusCode(status));
+    return;
+  }
   if (origin_is_deletable)
     registered_origins_.erase(origin);
-  callback.Run(DatabaseStatusToStatusCode(status));
+  callback.Run(SERVICE_WORKER_OK);
 }
 
 scoped_refptr<ServiceWorkerRegistration>

content/browser/service_worker/service_worker_storage.h

@@ -151,8 +151,8 @@ class CONTENT_EXPORT ServiceWorkerStorage {
       ServiceWorkerDatabase::Status status);
   void DidReadRegistrationForId(
       const FindRegistrationCallback& callback,
-      const ServiceWorkerDatabase::RegistrationData& registration,
-      const ResourceList& resources,
+      ServiceWorkerDatabase::RegistrationData* registration,
+      ResourceList* resources,
       ServiceWorkerDatabase::Status status);
   void DidGetAllRegistrations(
       const GetAllRegistrationInfosCallback& callback,
@@ -161,7 +161,10 @@ class CONTENT_EXPORT ServiceWorkerStorage {
   void DidStoreRegistration(
       const GURL& origin,
       const StatusCallback& callback,
-      bool success);
+      ServiceWorkerDatabase::Status status);
+  void DidUpdateToActiveState(
+      const StatusCallback& callback,
+      ServiceWorkerDatabase::Status status);
   void DidDeleteRegistration(
       const GURL& origin,
       const StatusCallback& callback,