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

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

This change makes the following functions return status code instead of boolean.
- GetRegistrationsForOrigin()
- GetAllRegistrations()
- LazyOpen()
- ReadDatabaseVersion()

Other remaining functions will be fixed in the following CLs.


BUG=372704
TEST=should pass all existing tests

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

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

content/browser/service_worker/service_worker_database.cc

@@ -211,16 +211,16 @@ bool ParseResourceRecord(const std::string& serialized,
   return true;
 }
 
-ServiceWorkerStatusCode LevelDBStatusToServiceWorkerStatusCode(
+ServiceWorkerDatabase::Status LevelDBStatusToStatus(
     const leveldb::Status& status) {
   if (status.ok())
-    return SERVICE_WORKER_OK;
+    return ServiceWorkerDatabase::STATUS_OK;
   else if (status.IsNotFound())
-    return SERVICE_WORKER_ERROR_NOT_FOUND;
+    return ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND;
   else if (status.IsCorruption())
-    return SERVICE_WORKER_ERROR_DB_CORRUPTED;
+    return ServiceWorkerDatabase::STATUS_ERROR_CORRUPTED;
   else
-    return SERVICE_WORKER_ERROR_FAILED;
+    return ServiceWorkerDatabase::STATUS_ERROR_FAILED;
 }
 
 }  // namespace
@@ -251,7 +251,7 @@ ServiceWorkerDatabase::~ServiceWorkerDatabase() {
   db_.reset();
 }
 
-ServiceWorkerStatusCode ServiceWorkerDatabase::GetNextAvailableIds(
+ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetNextAvailableIds(
     int64* next_avail_registration_id,
     int64* next_avail_version_id,
     int64* next_avail_resource_id) {
@@ -260,52 +260,49 @@ ServiceWorkerStatusCode ServiceWorkerDatabase::GetNextAvailableIds(
   DCHECK(next_avail_version_id);
   DCHECK(next_avail_resource_id);
 
-  if (!LazyOpen(false)) {
-    if (is_disabled_)
-      return SERVICE_WORKER_ERROR_FAILED;
-    // Database has never been used.
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status)) {
     *next_avail_registration_id = 0;
     *next_avail_version_id = 0;
     *next_avail_resource_id = 0;
-    return SERVICE_WORKER_OK;
+    return STATUS_OK;
   }
+  if (status != STATUS_OK)
+    return status;
 
-  ServiceWorkerStatusCode status =
-      ReadNextAvailableId(kNextRegIdKey, &next_avail_registration_id_);
-  if (status != SERVICE_WORKER_OK)
+  status = ReadNextAvailableId(kNextRegIdKey, &next_avail_registration_id_);
+  if (status != STATUS_OK)
     return status;
   status = ReadNextAvailableId(kNextVerIdKey, &next_avail_version_id_);
-  if (status != SERVICE_WORKER_OK)
+  if (status != STATUS_OK)
     return status;
   status = ReadNextAvailableId(kNextResIdKey, &next_avail_resource_id_);
-  if (status != SERVICE_WORKER_OK)
+  if (status != STATUS_OK)
     return status;
 
   *next_avail_registration_id = next_avail_registration_id_;
   *next_avail_version_id = next_avail_version_id_;
   *next_avail_resource_id = next_avail_resource_id_;
-  return SERVICE_WORKER_OK;
+  return STATUS_OK;
 }
 
-ServiceWorkerStatusCode ServiceWorkerDatabase::GetOriginsWithRegistrations(
-    std::set<GURL>* origins) {
+ServiceWorkerDatabase::Status
+ServiceWorkerDatabase::GetOriginsWithRegistrations(std::set<GURL>* origins) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  DCHECK(origins);
+  DCHECK(origins->empty());
 
-  if (!LazyOpen(false)) {
-    if (is_disabled_)
-      return SERVICE_WORKER_ERROR_FAILED;
-    // Database has never been used.
-    origins->clear();
-    return SERVICE_WORKER_OK;
-  }
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return STATUS_OK;
+  if (status != STATUS_OK)
+    return status;
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(kUniqueOriginKey); itr->Valid(); itr->Next()) {
     if (!itr->status().ok()) {
       HandleError(FROM_HERE, itr->status());
       origins->clear();
-      return LevelDBStatusToServiceWorkerStatusCode(itr->status());
+      return LevelDBStatusToStatus(itr->status());
     }
 
     std::string origin;
@@ -313,22 +310,20 @@ ServiceWorkerStatusCode ServiceWorkerDatabase::GetOriginsWithRegistrations(
       break;
     origins->insert(GURL(origin));
   }
-  return SERVICE_WORKER_OK;
+  return STATUS_OK;
 }
 
-bool ServiceWorkerDatabase::GetRegistrationsForOrigin(
+ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetRegistrationsForOrigin(
     const GURL& origin,
     std::vector<RegistrationData>* registrations) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  DCHECK(registrations);
+  DCHECK(registrations->empty());
 
-  if (!LazyOpen(false)) {
-    if (is_disabled_)
-      return false;
-    // Database has never been used.
-    registrations->clear();
-    return true;
-  }
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return STATUS_OK;
+  if (status != STATUS_OK)
+    return status;
 
   // Create a key prefix for registrations.
   std::string prefix = base::StringPrintf(
@@ -339,7 +334,7 @@ bool ServiceWorkerDatabase::GetRegistrationsForOrigin(
     if (!itr->status().ok()) {
       HandleError(FROM_HERE, itr->status());
       registrations->clear();
-      return false;
+      return LevelDBStatusToStatus(itr->status());
     }
 
     if (!RemovePrefix(itr->key().ToString(), prefix, NULL))
@@ -349,32 +344,30 @@ bool ServiceWorkerDatabase::GetRegistrationsForOrigin(
     if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
       HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
       registrations->clear();
-      return false;
+      return STATUS_ERROR_CORRUPTED;
     }
     registrations->push_back(registration);
   }
-  return true;
+  return STATUS_OK;
 }
 
-bool ServiceWorkerDatabase::GetAllRegistrations(
+ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetAllRegistrations(
     std::vector<RegistrationData>* registrations) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  DCHECK(registrations);
+  DCHECK(registrations->empty());
 
-  if (!LazyOpen(false)) {
-    if (is_disabled_)
-      return false;
-    // Database has never been used.
-    registrations->clear();
-    return true;
-  }
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return STATUS_OK;
+  if (status != STATUS_OK)
+    return status;
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   for (itr->Seek(kRegKeyPrefix); itr->Valid(); itr->Next()) {
     if (!itr->status().ok()) {
       HandleError(FROM_HERE, itr->status());
       registrations->clear();
-      return false;
+      return LevelDBStatusToStatus(itr->status());
     }
 
     if (!RemovePrefix(itr->key().ToString(), kRegKeyPrefix, NULL))
@@ -384,11 +377,11 @@ bool ServiceWorkerDatabase::GetAllRegistrations(
     if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
       HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
       registrations->clear();
-      return false;
+      return STATUS_ERROR_CORRUPTED;
     }
     registrations->push_back(registration);
   }
-  return true;
+  return STATUS_OK;
 }
 
 bool ServiceWorkerDatabase::ReadRegistration(
@@ -400,7 +393,8 @@ bool ServiceWorkerDatabase::ReadRegistration(
   DCHECK(registration);
   DCHECK(resources);
 
-  if (!LazyOpen(false) || is_disabled_)
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status) || status != STATUS_OK)
     return false;
 
   RegistrationData value;
@@ -418,7 +412,7 @@ bool ServiceWorkerDatabase::WriteRegistration(
     const RegistrationData& registration,
     const std::vector<ResourceRecord>& resources) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  if (!LazyOpen(true) || is_disabled_)
+  if (LazyOpen(true) != STATUS_OK)
     return false;
 
   leveldb::WriteBatch batch;
@@ -474,7 +468,8 @@ bool ServiceWorkerDatabase::WriteRegistration(
 bool ServiceWorkerDatabase::UpdateVersionToActive(int64 registration_id,
                                                   const GURL& origin) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  if (!LazyOpen(false) || is_disabled_)
+  ServiceWorkerDatabase::Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status) || status != STATUS_OK)
     return false;
 
   RegistrationData registration;
@@ -492,7 +487,8 @@ bool ServiceWorkerDatabase::UpdateLastCheckTime(int64 registration_id,
                                                 const GURL& origin,
                                                 const base::Time& time) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  if (!LazyOpen(false) || is_disabled_)
+  ServiceWorkerDatabase::Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status) || status != STATUS_OK)
     return false;
 
   RegistrationData registration;
@@ -509,7 +505,10 @@ bool ServiceWorkerDatabase::UpdateLastCheckTime(int64 registration_id,
 bool ServiceWorkerDatabase::DeleteRegistration(int64 registration_id,
                                                const GURL& origin) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  if (!LazyOpen(false) || is_disabled_)
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return STATUS_OK;
+  if (status != STATUS_OK || !origin.is_valid())
     return false;
 
   leveldb::WriteBatch batch;
@@ -518,7 +517,7 @@ bool ServiceWorkerDatabase::DeleteRegistration(int64 registration_id,
   // |registration_id| is the only one for |origin|.
   // TODO(nhiroki): Check the uniqueness by more efficient way.
   std::vector<RegistrationData> registrations;
-  if (!GetRegistrationsForOrigin(origin, &registrations))
+  if (GetRegistrationsForOrigin(origin, &registrations) != STATUS_OK)
     return false;
   if (registrations.size() == 1 &&
       registrations[0].registration_id == registration_id) {
@@ -571,7 +570,10 @@ bool ServiceWorkerDatabase::ClearPurgeableResourceIds(
 
 bool ServiceWorkerDatabase::DeleteAllDataForOrigin(const GURL& origin) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  if (!LazyOpen(true) || is_disabled_ || !origin.is_valid())
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return STATUS_OK;
+  if (status != STATUS_OK || !origin.is_valid())
     return false;
 
   leveldb::WriteBatch batch;
@@ -580,7 +582,7 @@ bool ServiceWorkerDatabase::DeleteAllDataForOrigin(const GURL& origin) {
   batch.Delete(CreateUniqueOriginKey(origin));
 
   std::vector<RegistrationData> registrations;
-  if (!GetRegistrationsForOrigin(origin, &registrations))
+  if (GetRegistrationsForOrigin(origin, &registrations) != STATUS_OK)
     return false;
 
   // Delete registrations and resource records.
@@ -594,54 +596,66 @@ bool ServiceWorkerDatabase::DeleteAllDataForOrigin(const GURL& origin) {
   return WriteBatch(&batch);
 }
 
-bool ServiceWorkerDatabase::LazyOpen(bool create_if_needed) {
+ServiceWorkerDatabase::Status ServiceWorkerDatabase::LazyOpen(
+    bool create_if_missing) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
-  if (IsOpen())
-    return true;
 
   // Do not try to open a database if we tried and failed once.
   if (is_disabled_)
-    return false;
+    return STATUS_ERROR_FAILED;
+  if (IsOpen())
+    return STATUS_OK;
 
   // When |path_| is empty, open a database in-memory.
   bool use_in_memory_db = path_.empty();
 
-  if (!create_if_needed) {
+  if (!create_if_missing) {
     // Avoid opening a database if it does not exist at the |path_|.
     if (use_in_memory_db ||
         !base::PathExists(path_) ||
         base::IsDirectoryEmpty(path_)) {
-      return false;
+      return STATUS_ERROR_NOT_FOUND;
     }
   }
 
   leveldb::Options options;
-  options.create_if_missing = create_if_needed;
+  options.create_if_missing = create_if_missing;
   if (use_in_memory_db) {
     env_.reset(leveldb::NewMemEnv(leveldb::Env::Default()));
     options.env = env_.get();
   }
 
   leveldb::DB* db = NULL;
-  leveldb::Status status =
+  leveldb::Status db_status =
       leveldb::DB::Open(options, path_.AsUTF8Unsafe(), &db);
-  if (!status.ok()) {
+  if (!db_status.ok()) {
     DCHECK(!db);
     // TODO(nhiroki): Should we retry to open the database?
-    HandleError(FROM_HERE, status);
-    return false;
+    HandleError(FROM_HERE, db_status);
+    return LevelDBStatusToStatus(db_status);
   }
   db_.reset(db);
 
   int64 db_version;
-  if (!ReadDatabaseVersion(&db_version))
-    return false;
+  Status status = ReadDatabaseVersion(&db_version);
+  if (status != STATUS_OK)
+    return status;
+  DCHECK_LE(0, db_version);
   if (db_version > 0)
     is_initialized_ = true;
-  return true;
+  return STATUS_OK;
+}
+
+bool ServiceWorkerDatabase::IsNewOrNonexistentDatabase(
+    ServiceWorkerDatabase::Status status) {
+  if (status == STATUS_ERROR_NOT_FOUND)
+    return true;
+  if (status == STATUS_OK && !is_initialized_)
+    return true;
+  return false;
 }
 
-ServiceWorkerStatusCode ServiceWorkerDatabase::ReadNextAvailableId(
+ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadNextAvailableId(
     const char* id_key,
     int64* next_avail_id) {
   DCHECK(id_key);
@@ -652,22 +666,22 @@ ServiceWorkerStatusCode ServiceWorkerDatabase::ReadNextAvailableId(
   if (status.IsNotFound()) {
     // Nobody has gotten the next resource id for |id_key|.
     *next_avail_id = 0;
-    return SERVICE_WORKER_OK;
+    return STATUS_OK;
   }
 
   if (!status.ok()) {
     HandleError(FROM_HERE, status);
-    return LevelDBStatusToServiceWorkerStatusCode(status);
+    return LevelDBStatusToStatus(status);
   }
 
   int64 parsed;
   if (!base::StringToInt64(value, &parsed)) {
     HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return SERVICE_WORKER_ERROR_DB_CORRUPTED;
+    return STATUS_ERROR_CORRUPTED;
   }
 
   *next_avail_id = parsed;
-  return SERVICE_WORKER_OK;
+  return STATUS_OK;
 }
 
 bool ServiceWorkerDatabase::ReadRegistrationData(
@@ -762,9 +776,12 @@ bool ServiceWorkerDatabase::ReadResourceIds(const char* id_key_prefix,
                                             std::set<int64>* ids) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(id_key_prefix);
-  DCHECK(ids);
+  DCHECK(ids->empty());
 
-  if (!LazyOpen(false) || is_disabled_)
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return true;
+  if (status != STATUS_OK)
     return false;
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
@@ -795,7 +812,7 @@ bool ServiceWorkerDatabase::WriteResourceIds(const char* id_key_prefix,
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(id_key_prefix);
 
-  if (!LazyOpen(true) || is_disabled_)
+  if (LazyOpen(true) != STATUS_OK)
     return false;
   if (ids.empty())
     return true;
@@ -814,8 +831,12 @@ bool ServiceWorkerDatabase::DeleteResourceIds(const char* id_key_prefix,
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   DCHECK(id_key_prefix);
 
-  if (!LazyOpen(true) || is_disabled_)
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return true;
+  if (status != STATUS_OK)
     return false;
+
   if (ids.empty())
     return true;
 
@@ -827,34 +848,35 @@ bool ServiceWorkerDatabase::DeleteResourceIds(const char* id_key_prefix,
   return WriteBatch(&batch);
 }
 
-bool ServiceWorkerDatabase::ReadDatabaseVersion(int64* db_version) {
+ServiceWorkerDatabase::Status ServiceWorkerDatabase::ReadDatabaseVersion(
+    int64* db_version) {
   std::string value;
   leveldb::Status status =
       db_->Get(leveldb::ReadOptions(), kDatabaseVersionKey, &value);
   if (status.IsNotFound()) {
     // The database hasn't been initialized yet.
     *db_version = 0;
-    return true;
+    return STATUS_OK;
   }
   if (!status.ok()) {
     HandleError(FROM_HERE, status);
-    return false;
+    return LevelDBStatusToStatus(status);
   }
 
   int64 parsed;
   if (!base::StringToInt64(value, &parsed)) {
     HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
-    return false;
+    return STATUS_ERROR_CORRUPTED;
   }
 
   const int kFirstValidVersion = 1;
   if (parsed < kFirstValidVersion || kCurrentSchemaVersion < parsed) {
     HandleError(FROM_HERE, leveldb::Status::Corruption("invalid DB version"));
-    return false;
+    return STATUS_ERROR_CORRUPTED;
   }
 
   *db_version = parsed;
-  return true;
+  return STATUS_OK;
 }
 
 bool ServiceWorkerDatabase::WriteBatch(leveldb::WriteBatch* batch) {

content/browser/service_worker/service_worker_database.h

@@ -39,6 +39,13 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
   explicit ServiceWorkerDatabase(const base::FilePath& path);
   ~ServiceWorkerDatabase();
 
+  enum Status {
+    STATUS_OK,
+    STATUS_ERROR_NOT_FOUND,
+    STATUS_ERROR_CORRUPTED,
+    STATUS_ERROR_FAILED,
+  };
+
   struct CONTENT_EXPORT RegistrationData {
     // These values are immutable for the life of a registration.
     int64 registration_id;
@@ -62,19 +69,28 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
     GURL url;
   };
 
-  // For use during initialization.
-  ServiceWorkerStatusCode GetNextAvailableIds(
+  // Reads next available ids from the database. Returns OK if they are
+  // successfully read. Fills the arguments with an initial value and returns
+  // OK if they are not found in the database. Otherwise, returns an error.
+  Status GetNextAvailableIds(
       int64* next_avail_registration_id,
       int64* next_avail_version_id,
       int64* next_avail_resource_id);
-  ServiceWorkerStatusCode GetOriginsWithRegistrations(
-      std::set<GURL>* origins);
 
-  // For use when first handling a request in an origin with registrations.
-  bool GetRegistrationsForOrigin(const GURL& origin,
-                                 std::vector<RegistrationData>* registrations);
+  // Reads origins that have one or more than one registration from the
+  // database. Returns OK if they are successfully read or not found.
+  // Otherwise, returns an error.
+  Status GetOriginsWithRegistrations(std::set<GURL>* origins);
+
+  // Reads registrations for |origin| from the database. Returns OK if they are
+  // successfully read or not found. Otherwise, returns an error.
+  Status GetRegistrationsForOrigin(
+      const GURL& origin,
+      std::vector<RegistrationData>* registrations);
 
-  bool GetAllRegistrations(std::vector<RegistrationData>* registrations);
+  // Reads all registrations from the database. Returns OK if successfully read
+  // or not found. Otherwise, returns an error.
+  Status GetAllRegistrations(std::vector<RegistrationData>* registrations);
 
   // Saving, retrieving, and updating registration data.
   // (will bump next_avail_xxxx_ids as needed)
@@ -136,14 +152,23 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
 
  private:
   // Opens the database at the |path_|. This is lazily called when the first
-  // database API is called. Returns true if the database was opened. Returns
-  // false if the opening failed or was not neccessary, that is, the database
-  // does not exist and |create_if_needed| is false.
-  bool LazyOpen(bool create_if_needed);
-
-  ServiceWorkerStatusCode ReadNextAvailableId(
+  // database API is called. Returns OK if the database is successfully opened.
+  // Returns NOT_FOUND if the database does not exist and |create_if_missing| is
+  // false. Otherwise, returns an error.
+  Status LazyOpen(bool create_if_missing);
+
+  // Helper for LazyOpen(). |status| must be the return value from LazyOpen()
+  // and this must be called just after LazyOpen() is called. Returns true if
+  // the database is new or nonexistent, that is, it has never been used.
+  bool IsNewOrNonexistentDatabase(Status status);
+
+  // Reads the next available id for |id_key|. Returns OK if it's successfully
+  // read. Fills |next_avail_id| with an initial value and returns OK if it's
+  // not found in the database. Otherwise, returns an error.
+  Status ReadNextAvailableId(
       const char* id_key,
       int64* next_avail_id);
+
   bool ReadRegistrationData(int64 registration_id,
                             const GURL& origin,
                             RegistrationData* registration);
@@ -159,8 +184,8 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
                          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 true.
-  bool ReadDatabaseVersion(int64* db_version);
+  // been written anything yet, sets |db_version| to 0 and returns OK.
+  Status ReadDatabaseVersion(int64* db_version);
 
   // Write a batch into the database.
   // NOTE: You must call this when you want to put something into the database
@@ -204,9 +229,6 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, OpenDatabase_InMemory);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, DatabaseVersion);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, GetNextAvailableIds);
-  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, Registration_Basic);
-  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, Registration_Overwrite);
-  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, Registration_Multiple);
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerDatabase);
 };

content/browser/service_worker/service_worker_database_unittest.cc

@@ -80,35 +80,39 @@ TEST(ServiceWorkerDatabaseTest, OpenDatabase) {
       CreateDatabase(database_dir.path()));
 
   // Should be false because the database does not exist at the path.
-  EXPECT_FALSE(database->LazyOpen(false));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND,
+            database->LazyOpen(false));
 
-  EXPECT_TRUE(database->LazyOpen(true));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->LazyOpen(true));
 
   database.reset(CreateDatabase(database_dir.path()));
-  EXPECT_TRUE(database->LazyOpen(false));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->LazyOpen(false));
 }
 
 TEST(ServiceWorkerDatabaseTest, OpenDatabase_InMemory) {
   scoped_ptr<ServiceWorkerDatabase> database(CreateDatabaseInMemory());
 
   // Should be false because the database does not exist in memory.
-  EXPECT_FALSE(database->LazyOpen(false));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND,
+            database->LazyOpen(false));
 
-  EXPECT_TRUE(database->LazyOpen(true));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->LazyOpen(true));
   database.reset(CreateDatabaseInMemory());
 
   // Should be false because the database is not persistent.
-  EXPECT_FALSE(database->LazyOpen(false));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND,
+            database->LazyOpen(false));
 }
 
 TEST(ServiceWorkerDatabaseTest, DatabaseVersion) {
   scoped_ptr<ServiceWorkerDatabase> database(CreateDatabaseInMemory());
-  EXPECT_TRUE(database->LazyOpen(true));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->LazyOpen(true));
 
   // Opening a new database does not write anything, so its schema version
   // should be 0.
   int64 db_version = -1;
-  EXPECT_TRUE(database->ReadDatabaseVersion(&db_version));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->ReadDatabaseVersion(&db_version));
   EXPECT_EQ(0u, db_version);
 
   // First writing triggers database initialization and bumps the schema
@@ -117,7 +121,8 @@ TEST(ServiceWorkerDatabaseTest, DatabaseVersion) {
   ServiceWorkerDatabase::RegistrationData data;
   ASSERT_TRUE(database->WriteRegistration(data, resources));
 
-  EXPECT_TRUE(database->ReadDatabaseVersion(&db_version));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->ReadDatabaseVersion(&db_version));
   EXPECT_LT(0, db_version);
 }
 
@@ -131,14 +136,14 @@ TEST(ServiceWorkerDatabaseTest, GetNextAvailableIds) {
 
   // The database has never been used, so returns initial values.
   AvailableIds ids;
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetNextAvailableIds(
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->GetNextAvailableIds(
       &ids.reg_id, &ids.ver_id, &ids.res_id));
   EXPECT_EQ(0, ids.reg_id);
   EXPECT_EQ(0, ids.ver_id);
   EXPECT_EQ(0, ids.res_id);
 
-  ASSERT_TRUE(database->LazyOpen(true));
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetNextAvailableIds(
+  ASSERT_EQ(ServiceWorkerDatabase::STATUS_OK, database->LazyOpen(true));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->GetNextAvailableIds(
       &ids.reg_id, &ids.ver_id, &ids.res_id));
   EXPECT_EQ(0, ids.reg_id);
   EXPECT_EQ(0, ids.ver_id);
@@ -153,7 +158,7 @@ TEST(ServiceWorkerDatabaseTest, GetNextAvailableIds) {
   data1.version_id = 200;
   ASSERT_TRUE(database->WriteRegistration(data1, resources));
 
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetNextAvailableIds(
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->GetNextAvailableIds(
       &ids.reg_id, &ids.ver_id, &ids.res_id));
   EXPECT_EQ(101, ids.reg_id);
   EXPECT_EQ(201, ids.ver_id);
@@ -171,7 +176,7 @@ TEST(ServiceWorkerDatabaseTest, GetNextAvailableIds) {
   // Close and reopen the database to verify the stored values.
   database.reset(CreateDatabase(database_dir.path()));
 
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetNextAvailableIds(
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, database->GetNextAvailableIds(
       &ids.reg_id, &ids.ver_id, &ids.res_id));
   EXPECT_EQ(101, ids.reg_id);
   EXPECT_EQ(201, ids.ver_id);
@@ -182,7 +187,8 @@ TEST(ServiceWorkerDatabaseTest, GetOriginsWithRegistrations) {
   scoped_ptr<ServiceWorkerDatabase> database(CreateDatabaseInMemory());
 
   std::set<GURL> origins;
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetOriginsWithRegistrations(&origins));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetOriginsWithRegistrations(&origins));
   EXPECT_TRUE(origins.empty());
 
   std::vector<Resource> resources;
@@ -220,7 +226,8 @@ TEST(ServiceWorkerDatabaseTest, GetOriginsWithRegistrations) {
   ASSERT_TRUE(database->WriteRegistration(data4, resources));
 
   origins.clear();
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetOriginsWithRegistrations(&origins));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetOriginsWithRegistrations(&origins));
   EXPECT_EQ(3U, origins.size());
   EXPECT_TRUE(ContainsKey(origins, origin1));
   EXPECT_TRUE(ContainsKey(origins, origin2));
@@ -231,7 +238,8 @@ TEST(ServiceWorkerDatabaseTest, GetOriginsWithRegistrations) {
   ASSERT_TRUE(database->DeleteRegistration(data4.registration_id, origin3));
 
   origins.clear();
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetOriginsWithRegistrations(&origins));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetOriginsWithRegistrations(&origins));
   EXPECT_EQ(3U, origins.size());
   EXPECT_TRUE(ContainsKey(origins, origin1));
   EXPECT_TRUE(ContainsKey(origins, origin2));
@@ -241,7 +249,8 @@ TEST(ServiceWorkerDatabaseTest, GetOriginsWithRegistrations) {
   ASSERT_TRUE(database->DeleteRegistration(data3.registration_id, origin3));
 
   origins.clear();
-  EXPECT_EQ(SERVICE_WORKER_OK, database->GetOriginsWithRegistrations(&origins));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetOriginsWithRegistrations(&origins));
   EXPECT_EQ(2U, origins.size());
   EXPECT_TRUE(ContainsKey(origins, origin1));
   EXPECT_TRUE(ContainsKey(origins, origin2));
@@ -255,7 +264,8 @@ TEST(ServiceWorkerDatabaseTest, GetRegistrationsForOrigin) {
   GURL origin3("https://example.org");
 
   std::vector<RegistrationData> registrations;
-  EXPECT_TRUE(database->GetRegistrationsForOrigin(origin1, &registrations));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetRegistrationsForOrigin(origin1, &registrations));
   EXPECT_TRUE(registrations.empty());
 
   std::vector<Resource> resources;
@@ -290,7 +300,8 @@ TEST(ServiceWorkerDatabaseTest, GetRegistrationsForOrigin) {
   ASSERT_TRUE(database->WriteRegistration(data4, resources));
 
   registrations.clear();
-  EXPECT_TRUE(database->GetRegistrationsForOrigin(origin3, &registrations));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetRegistrationsForOrigin(origin3, &registrations));
   EXPECT_EQ(2U, registrations.size());
   VerifyRegistrationData(data3, registrations[0]);
   VerifyRegistrationData(data4, registrations[1]);
@@ -300,7 +311,8 @@ TEST(ServiceWorkerDatabaseTest, GetAllRegistrations) {
   scoped_ptr<ServiceWorkerDatabase> database(CreateDatabaseInMemory());
 
   std::vector<RegistrationData> registrations;
-  EXPECT_TRUE(database->GetAllRegistrations(&registrations));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetAllRegistrations(&registrations));
   EXPECT_TRUE(registrations.empty());
 
   std::vector<Resource> resources;
@@ -338,7 +350,8 @@ TEST(ServiceWorkerDatabaseTest, GetAllRegistrations) {
   ASSERT_TRUE(database->WriteRegistration(data4, resources));
 
   registrations.clear();
-  EXPECT_TRUE(database->GetAllRegistrations(&registrations));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetAllRegistrations(&registrations));
   EXPECT_EQ(4U, registrations.size());
   VerifyRegistrationData(data1, registrations[0]);
   VerifyRegistrationData(data2, registrations[1]);
@@ -728,14 +741,15 @@ TEST(ServiceWorkerDatabaseTest, DeleteAllDataForOrigin) {
 
   // |origin1| should be removed from the unique origin list.
   std::set<GURL> unique_origins;
-  EXPECT_EQ(SERVICE_WORKER_OK,
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
             database->GetOriginsWithRegistrations(&unique_origins));
   EXPECT_EQ(1u, unique_origins.size());
   EXPECT_TRUE(ContainsKey(unique_origins, origin2));
 
   // The registrations for |origin1| should be removed.
   std::vector<RegistrationData> registrations;
-  EXPECT_TRUE(database->GetRegistrationsForOrigin(origin1, &registrations));
+  EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK,
+            database->GetRegistrationsForOrigin(origin1, &registrations));
   EXPECT_TRUE(registrations.empty());
 
   // The registration for |origin2| should not be removed.

content/browser/service_worker/service_worker_registration_status.cc

@@ -40,7 +40,6 @@ void GetServiceWorkerRegistrationStatusResponse(
     case SERVICE_WORKER_ERROR_FAILED:
     case SERVICE_WORKER_ERROR_PROCESS_NOT_FOUND:
     case SERVICE_WORKER_ERROR_EXISTS:
-    case SERVICE_WORKER_ERROR_DB_CORRUPTED:
       // Unexpected, or should have bailed out before calling this, or we don't
       // have a corresponding blink error code yet.
       break;  // Fall through to NOTREACHED().

content/browser/service_worker/service_worker_storage.cc

@@ -27,14 +27,14 @@ namespace {
 
 typedef base::Callback<void(
     ServiceWorkerStorage::InitialData* data,
-    ServiceWorkerStatusCode status)> InitializeCallback;
+    ServiceWorkerDatabase::Status status)> InitializeCallback;
 typedef base::Callback<void(
     const ServiceWorkerDatabase::RegistrationData& data,
     const std::vector<ServiceWorkerDatabase::ResourceRecord>& resources,
-    ServiceWorkerStatusCode status)> ReadRegistrationCallback;
+    ServiceWorkerDatabase::Status status)> ReadRegistrationCallback;
 typedef base::Callback<void(
     bool origin_is_deletable,
-    ServiceWorkerStatusCode status)> DeleteRegistrationCallback;
+    ServiceWorkerDatabase::Status status)> DeleteRegistrationCallback;
 
 void RunSoon(const tracked_objects::Location& from_here,
              const base::Closure& closure) {
@@ -65,6 +65,18 @@ const int kMaxMemDiskCacheSize = 10 * 1024 * 1024;
 
 void EmptyCompletionCallback(int) {}
 
+ServiceWorkerStatusCode DatabaseStatusToStatusCode(
+    ServiceWorkerDatabase::Status status) {
+  switch (status) {
+    case ServiceWorkerDatabase::STATUS_OK:
+      return SERVICE_WORKER_OK;
+    case ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND:
+      return SERVICE_WORKER_ERROR_NOT_FOUND;
+    default:
+      return SERVICE_WORKER_ERROR_FAILED;
+  }
+}
+
 void ReadInitialDataFromDB(
     ServiceWorkerDatabase* database,
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
@@ -73,11 +85,11 @@ void ReadInitialDataFromDB(
   scoped_ptr<ServiceWorkerStorage::InitialData> data(
       new ServiceWorkerStorage::InitialData());
 
-  ServiceWorkerStatusCode status =
+  ServiceWorkerDatabase::Status status =
       database->GetNextAvailableIds(&data->next_registration_id,
                                     &data->next_version_id,
                                     &data->next_resource_id);
-  if (status != SERVICE_WORKER_OK) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
     original_task_runner->PostTask(
         FROM_HERE, base::Bind(callback, base::Owned(data.release()), status));
     return;
@@ -100,10 +112,11 @@ void ReadRegistrationFromDB(
 
   // TODO(nhiroki): The database should return more detailed status like
   // ServiceWorkerStatusCode instead of bool value.
-  ServiceWorkerStatusCode status = SERVICE_WORKER_OK;
+  ServiceWorkerDatabase::Status status = ServiceWorkerDatabase::STATUS_OK;
   if (!database->ReadRegistration(registration_id, origin, &data, &resources)) {
-    status = database->is_disabled() ? SERVICE_WORKER_ERROR_FAILED
-                                     : SERVICE_WORKER_ERROR_NOT_FOUND;
+    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));
@@ -118,22 +131,26 @@ void DeleteRegistrationFromDB(
   DCHECK(database);
   if (!database->DeleteRegistration(registration_id, origin)) {
     original_task_runner->PostTask(
-        FROM_HERE, base::Bind(callback, false, SERVICE_WORKER_ERROR_FAILED));
+        FROM_HERE, base::Bind(callback, false,
+                              ServiceWorkerDatabase::STATUS_ERROR_FAILED));
     return;
   }
 
   // TODO(nhiroki): Add convenient method to ServiceWorkerDatabase to check the
   // unique origin list.
   std::vector<ServiceWorkerDatabase::RegistrationData> registrations;
-  if (!database->GetRegistrationsForOrigin(origin, &registrations)) {
+  ServiceWorkerDatabase::Status status =
+      database->GetRegistrationsForOrigin(origin, &registrations);
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
     original_task_runner->PostTask(
-        FROM_HERE, base::Bind(callback, false, SERVICE_WORKER_ERROR_FAILED));
+        FROM_HERE, base::Bind(callback, false, status));
     return;
   }
 
   bool deletable = registrations.empty();
   original_task_runner->PostTask(
-      FROM_HERE, base::Bind(callback, deletable, SERVICE_WORKER_OK));
+      FROM_HERE, base::Bind(callback, deletable,
+                            ServiceWorkerDatabase::STATUS_OK));
 }
 
 void UpdateToActiveStateInDB(
@@ -499,21 +516,20 @@ bool ServiceWorkerStorage::LazyInitialize(const base::Closure& callback) {
 
 void ServiceWorkerStorage::DidReadInitialData(
     InitialData* data,
-    ServiceWorkerStatusCode status) {
+    ServiceWorkerDatabase::Status status) {
   DCHECK(data);
   DCHECK_EQ(INITIALIZING, state_);
 
-  if (status == SERVICE_WORKER_OK) {
+  if (status == ServiceWorkerDatabase::STATUS_OK) {
     next_registration_id_ = data->next_registration_id;
     next_version_id_ = data->next_version_id;
     next_resource_id_ = data->next_resource_id;
     registered_origins_.swap(data->origins);
     state_ = INITIALIZED;
   } else {
-    // TODO(nhiroki): If status==SERVICE_WORKER_ERROR_DB_CORRUPTED, do
-    // corruption recovery (http://crbug.com/371675).
-    DLOG(WARNING) << "Failed to initialize: "
-                  << ServiceWorkerStatusToString(status);
+    // TODO(nhiroki): If status==STATUS_ERROR_CORRUPTED, do corruption recovery
+    // (http://crbug.com/371675).
+    DLOG(WARNING) << "Failed to initialize: " << status;
     state_ = DISABLED;
   }
 
@@ -528,9 +544,10 @@ void ServiceWorkerStorage::DidGetRegistrationsForPattern(
     const GURL& scope,
     const FindRegistrationCallback& callback,
     RegistrationList* registrations,
-    bool success) {
+    ServiceWorkerDatabase::Status status) {
   DCHECK(registrations);
-  if (!success) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    // TODO(nhiroki): Handle database error (http://crbug.com/371675).
     callback.Run(SERVICE_WORKER_ERROR_FAILED,
                  scoped_refptr<ServiceWorkerRegistration>());
     return;
@@ -565,9 +582,10 @@ void ServiceWorkerStorage::DidGetRegistrationsForDocument(
     const GURL& document_url,
     const FindRegistrationCallback& callback,
     RegistrationList* registrations,
-    bool success) {
+    ServiceWorkerDatabase::Status status) {
   DCHECK(registrations);
-  if (!success) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    // TODO(nhiroki): Handle database error (http://crbug.com/371675).
     callback.Run(SERVICE_WORKER_ERROR_FAILED,
                  scoped_refptr<ServiceWorkerRegistration>());
     return;
@@ -608,13 +626,13 @@ void ServiceWorkerStorage::DidReadRegistrationForId(
     const FindRegistrationCallback& callback,
     const ServiceWorkerDatabase::RegistrationData& registration,
     const ResourceList& resources,
-    ServiceWorkerStatusCode status) {
-  if (status == SERVICE_WORKER_OK) {
-    callback.Run(status, CreateRegistration(registration));
+    ServiceWorkerDatabase::Status status) {
+  if (status == ServiceWorkerDatabase::STATUS_OK) {
+    callback.Run(SERVICE_WORKER_OK, CreateRegistration(registration));
     return;
   }
 
-  if (status == SERVICE_WORKER_ERROR_NOT_FOUND) {
+  if (status == ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND) {
     // Look for somthing currently being installed.
     scoped_refptr<ServiceWorkerRegistration> installing_registration =
         FindInstallingRegistrationForId(registration.registration_id);
@@ -627,16 +645,18 @@ void ServiceWorkerStorage::DidReadRegistrationForId(
     return;
   }
 
-  callback.Run(status, scoped_refptr<ServiceWorkerRegistration>());
+  callback.Run(DatabaseStatusToStatusCode(status),
+               scoped_refptr<ServiceWorkerRegistration>());
   return;
 }
 
 void ServiceWorkerStorage::DidGetAllRegistrations(
     const GetAllRegistrationInfosCallback& callback,
     RegistrationList* registrations,
-    bool success) {
+    ServiceWorkerDatabase::Status status) {
   DCHECK(registrations);
-  if (!success) {
+  if (status != ServiceWorkerDatabase::STATUS_OK) {
+    // TODO(nhiroki): Handle database error (http://crbug.com/371675).
     callback.Run(std::vector<ServiceWorkerRegistrationInfo>());
     return;
   }
@@ -692,10 +712,10 @@ void ServiceWorkerStorage::DidDeleteRegistration(
     const GURL& origin,
     const StatusCallback& callback,
     bool origin_is_deletable,
-    ServiceWorkerStatusCode status) {
+    ServiceWorkerDatabase::Status status) {
   if (origin_is_deletable)
     registered_origins_.erase(origin);
-  callback.Run(status);
+  callback.Run(DatabaseStatusToStatusCode(status));
 }
 
 scoped_refptr<ServiceWorkerRegistration>

content/browser/service_worker/service_worker_storage.h

@@ -138,26 +138,26 @@ class CONTENT_EXPORT ServiceWorkerStorage {
       const base::Closure& callback);
   void DidReadInitialData(
       InitialData* data,
-      ServiceWorkerStatusCode status);
+      ServiceWorkerDatabase::Status status);
   void DidGetRegistrationsForPattern(
       const GURL& scope,
       const FindRegistrationCallback& callback,
       RegistrationList* registrations,
-      bool succcess);
+      ServiceWorkerDatabase::Status status);
   void DidGetRegistrationsForDocument(
       const GURL& scope,
       const FindRegistrationCallback& callback,
       RegistrationList* registrations,
-      bool succcess);
+      ServiceWorkerDatabase::Status status);
   void DidReadRegistrationForId(
       const FindRegistrationCallback& callback,
       const ServiceWorkerDatabase::RegistrationData& registration,
       const ResourceList& resources,
-      ServiceWorkerStatusCode status);
+      ServiceWorkerDatabase::Status status);
   void DidGetAllRegistrations(
       const GetAllRegistrationInfosCallback& callback,
       RegistrationList* registrations,
-      bool success);
+      ServiceWorkerDatabase::Status status);
   void DidStoreRegistration(
       const GURL& origin,
       const StatusCallback& callback,
@@ -166,7 +166,7 @@ class CONTENT_EXPORT ServiceWorkerStorage {
       const GURL& origin,
       const StatusCallback& callback,
       bool origin_is_deletable,
-      ServiceWorkerStatusCode status);
+      ServiceWorkerDatabase::Status status);
 
   scoped_refptr<ServiceWorkerRegistration> CreateRegistration(
       const ServiceWorkerDatabase::RegistrationData& data);

content/common/service_worker/service_worker_status_code.cc

@@ -30,8 +30,6 @@ const char* ServiceWorkerStatusToString(ServiceWorkerStatusCode status) {
       return "ServiceWorker failed to activate";
     case SERVICE_WORKER_ERROR_IPC_FAILED:
       return "IPC connection was closed or IPC error has occured";
-    case SERVICE_WORKER_ERROR_DB_CORRUPTED:
-      return "Database has been corrupted";
   }
   NOTREACHED();
   return "";

content/common/service_worker/service_worker_status_code.h

@@ -42,9 +42,6 @@ enum ServiceWorkerStatusCode {
   // Sending an IPC to the worker failed (often due to child process is
   // terminated).
   SERVICE_WORKER_ERROR_IPC_FAILED,
-
-  // Database corruption has occured.
-  SERVICE_WORKER_ERROR_DB_CORRUPTED,
 };
 
 CONTENT_EXPORT const char* ServiceWorkerStatusToString(