diff options
author | nhiroki@chromium.org <nhiroki@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-19 02:20:31 +0000 |
---|---|---|
committer | nhiroki@chromium.org <nhiroki@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-19 02:20:31 +0000 |
commit | 844203eb1033ed9874619dd854191b0bfcd5f1ef (patch) | |
tree | 2c4444eace4736af969a697883ba101c966ee445 /content | |
parent | 5a7100d501f0408ae8a4e83a57b68d8ac2ba6231 (diff) | |
download | chromium_src-844203eb1033ed9874619dd854191b0bfcd5f1ef.zip chromium_src-844203eb1033ed9874619dd854191b0bfcd5f1ef.tar.gz chromium_src-844203eb1033ed9874619dd854191b0bfcd5f1ef.tar.bz2 |
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
Diffstat (limited to 'content')
8 files changed, 248 insertions, 176 deletions
diff --git a/content/browser/service_worker/service_worker_database.cc b/content/browser/service_worker/service_worker_database.cc index db8f735..c0ce667 100644 --- a/content/browser/service_worker/service_worker_database.cc +++ b/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(), ®istration)) { 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(), ®istration)) { 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, ®istrations)) + if (GetRegistrationsForOrigin(origin, ®istrations) != 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, ®istrations)) + if (GetRegistrationsForOrigin(origin, ®istrations) != 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) { diff --git a/content/browser/service_worker/service_worker_database.h b/content/browser/service_worker/service_worker_database.h index 4716940..57654ca 100644 --- a/content/browser/service_worker/service_worker_database.h +++ b/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); }; diff --git a/content/browser/service_worker/service_worker_database_unittest.cc b/content/browser/service_worker/service_worker_database_unittest.cc index 0e2e26f..bc41c7d 100644 --- a/content/browser/service_worker/service_worker_database_unittest.cc +++ b/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, ®istrations)); + EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, + database->GetRegistrationsForOrigin(origin1, ®istrations)); 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, ®istrations)); + EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, + database->GetRegistrationsForOrigin(origin3, ®istrations)); 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(®istrations)); + EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, + database->GetAllRegistrations(®istrations)); 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(®istrations)); + EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, + database->GetAllRegistrations(®istrations)); 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, ®istrations)); + EXPECT_EQ(ServiceWorkerDatabase::STATUS_OK, + database->GetRegistrationsForOrigin(origin1, ®istrations)); EXPECT_TRUE(registrations.empty()); // The registration for |origin2| should not be removed. diff --git a/content/browser/service_worker/service_worker_registration_status.cc b/content/browser/service_worker/service_worker_registration_status.cc index f17f274..ee5ea90 100644 --- a/content/browser/service_worker/service_worker_registration_status.cc +++ b/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(). diff --git a/content/browser/service_worker/service_worker_storage.cc b/content/browser/service_worker/service_worker_storage.cc index 7754de2..877b0ad 100644 --- a/content/browser/service_worker/service_worker_storage.cc +++ b/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, ®istrations)) { + ServiceWorkerDatabase::Status status = + database->GetRegistrationsForOrigin(origin, ®istrations); + 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> diff --git a/content/browser/service_worker/service_worker_storage.h b/content/browser/service_worker/service_worker_storage.h index f2e68c3..a0183db 100644 --- a/content/browser/service_worker/service_worker_storage.h +++ b/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); diff --git a/content/common/service_worker/service_worker_status_code.cc b/content/common/service_worker/service_worker_status_code.cc index 51582b3..bdf28a9 100644 --- a/content/common/service_worker/service_worker_status_code.cc +++ b/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 ""; diff --git a/content/common/service_worker/service_worker_status_code.h b/content/common/service_worker/service_worker_status_code.h index 6d3175b..a0245e4 100644 --- a/content/common/service_worker/service_worker_status_code.h +++ b/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( |