ServiceWorker: Store registration data in ServiceWorkerDatabase

content/browser/service_worker/service_worker_database.cc

Index: content/browser/service_worker/service_worker_database.cc
diff --git a/content/browser/service_worker/service_worker_database.cc b/content/browser/service_worker/service_worker_database.cc
index 469223ba3e980c6f6c42b4324d261894bd1bb7c4..2ff9008fe887da758fa47d6c1a5f3fbfce0e8630 100644
--- a/content/browser/service_worker/service_worker_database.cc
+++ b/content/browser/service_worker/service_worker_database.cc
@@ -10,6 +10,10 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "content/browser/service_worker/service_worker_database.pb.h"
 #include "third_party/leveldatabase/src/helpers/memenv/memenv.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/env.h"
@@ -22,41 +26,95 @@
 // - int64 value is serialized as a string by base::Int64ToString().
 //
 // Version 1 (in sorted order)
-//   key: "DB_VERSION"
+//   key: "INITDATA_DB_VERSION"
 //   value: "1"
 //
-//   key: "NEXT_REGISTRATION_ID"
+//   key: "INITDATA_NEXT_REGISTRATION_ID"
 //   value: <int64 'next_available_registration_id'>
 //
-//   key: "NEXT_RESOURCE_ID"
+//   key: "INITDATA_NEXT_RESOURCE_ID"
 //   value: <int64 'next_available_resource_id'>
 //
-//   key: "NEXT_VERSION_ID"
+//   key: "INITDATA_NEXT_VERSION_ID"
 //   value: <int64 'next_available_version_id'>
+//
+//   key: "INITDATA_UNIQUE_ORIGIN:" + <GURL 'origin' serialized by GURL::spec()>
+//   value: <empty>
+//
+//   key: "REG:" + (1) + '\x00' + (2)
+//     (1) <GURL 'origin' serialized by GURL::spec()>
+//     (2) <int64 'registration_id'>
+//     (ex. "REG:http://example.com\x00123456")
+//   value: <ServiceWorkerRegistrationData serialized as a string>
 
 namespace content {
 
 namespace {
 
-const char kDatabaseVersionKey[] = "DB_VERSION";
-const char kNextRegIdKey[] = "NEXT_REGISTRATION_ID";
-const char kNextResIdKey[] = "NEXT_RESOURCE_ID";
-const char kNextVerIdKey[] = "NEXT_VERSION_ID";
+const char kDatabaseVersionKey[] = "INITDATA_DB_VERSION";
+const char kNextRegIdKey[] = "INITDATA_NEXT_REGISTRATION_ID";
+const char kNextResIdKey[] = "INITDATA_NEXT_RESOURCE_ID";
+const char kNextVerIdKey[] = "INITDATA_NEXT_VERSION_ID";
+const char kUniqueOriginKey[] = "INITDATA_UNIQUE_ORIGIN:";
+
+const char kRegKeyPrefix[] = "REG:";
+const char kKeySeparator = '\x00';
 
 const int64 kCurrentSchemaVersion = 1;
 
-}  // namespace
+bool RemovePrefix(const std::string& str,
+                  const std::string& prefix,
+                  std::string* out) {
+  DCHECK(out);
+  if (!StartsWithASCII(str, prefix, true))
+    return false;
+  *out = str.substr(prefix.size());
+  return true;
+}
+
+std::string CreateRegistrationKey(int64 registration_id,
+                                  const GURL& origin) {
+  return base::StringPrintf("%s%s%c%s",
+                            kRegKeyPrefix,
+                            origin.spec().c_str(),
+                            kKeySeparator,
+                            base::Int64ToString(registration_id).c_str());
+}
+
+std::string CreateUniqueOriginKey(const GURL& origin) {
+  return base::StringPrintf("%s%s", kUniqueOriginKey, origin.spec().c_str());
+}
+
+void PutRegistrationDataToBatch(const ServiceWorkerRegistrationData& data,
+                                leveldb::WriteBatch* batch) {
+  DCHECK(batch);
+  std::string value;
+  bool success = data.SerializeToString(&value);
+  DCHECK(success);
+  GURL origin = GURL(data.scope_url()).GetOrigin();

[michaeln, 2014/04/25 00:48:21]

Maybe this class should entirely encapsulate the use of protobuffers as the
serialization format. The generated classes have some gotchas associated with
them.
- can't have GURL data members
- doesn't construct with default values
Gotcha's that can make things a bit messy.
I think the set of fields we have defined are fundamental and can't really go
away, if something looks like its optional lets remove it instead.

We could limit the use of the generated classes to these two methods:
PutRegistrationDataToBatch and ParseRegistrationData. The ServiceWorkerDatabase
public api could function in terms of normal c++ structs and the generated
classes would be used for serialization/deserialization only.

wdyt?

[nhiroki, 2014/04/25 02:06:11]

I see. Will make them as required.
At first I didn't want to have two similar struct/class, but now I'm thinking
that can help to avoid such a mess. Okay, I'll try to resurrect
ServiceWorkerDatabase::RegistrationData.

[kinuko, 2014/04/25 03:43:27]

Fyi, it's possible to populate a field with a default value.

I'm not super positive about having similar structs (because protobuf-generated
struct is usually expressive/useful enough in many cases, and that's how most
google3 code is written), though GURL issue might be a bit annoying in chromium.
I don't object to it, but I want to make sure if we're really strongly motivated
not to use generated classes.

[nhiroki, 2014/04/25 11:01:05]

And there is base::Time issue.
Hmm... either way seems to have both good and bad points. Since the original API
uses non-protobuf-generated struct and I'd prefer to minimize changes in this CL
as far as possible, can I use non-protobuf-generated struct for now? If using
generated struct looks better, I'll rewrite them.

[michaeln, 2014/04/26 00:29:02]

I think the serialization format is an impl detail and there's enough extra
baggage with types and how to construct with default values (like having to
understand an entirely different language). Google3'ers probably makes a lot
heavier use of protobuffers then chromium/blink'ers do.

I'd definitely prefer to use simple structs with chromium friendly types in
them.

+  batch->Put(CreateRegistrationKey(data.registration_id(), origin), value);
+}
 
-ServiceWorkerDatabase::RegistrationData::RegistrationData()
-    : registration_id(-1),
-      version_id(-1),
-      is_active(false),
-      has_fetch_handler(false) {
+void PutUniqueOriginToBatch(const GURL& origin,
+                            leveldb::WriteBatch* batch) {
+  // Value should be empty.
+  batch->Put(CreateUniqueOriginKey(origin), "");
 }
 
-ServiceWorkerDatabase::RegistrationData::~RegistrationData() {
+bool ParseRegistrationData(const std::string& serialized,
+                           ServiceWorkerRegistrationData* out) {
+  DCHECK(out);
+  ServiceWorkerRegistrationData data;
+  if (!data.ParseFromString(serialized))
+    return false;
+  if (GURL(data.scope_url()).GetOrigin() != GURL(data.script_url()).GetOrigin())
+    return false;
+  *out = data;
+  return true;
 }
 
+}  // namespace
+
 ServiceWorkerDatabase::ServiceWorkerDatabase(const base::FilePath& path)
     : path_(path),
       is_disabled_(false),
@@ -95,6 +153,175 @@ bool ServiceWorkerDatabase::GetNextAvailableIds(
   return true;
 }
 
+bool ServiceWorkerDatabase::GetOriginsWithRegistrations(
+    std::set<GURL>* origins) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK(origins);
+
+  if (!LazyOpen(false) || is_disabled_)
+    return false;
+
+  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 false;
+    }
+
+    std::string key = itr->key().ToString();
+    if (!StartsWithASCII(key, kUniqueOriginKey, true))
+      break;
+
+    std::string origin;
+    bool success = RemovePrefix(key, kUniqueOriginKey, &origin);

[michaeln, 2014/04/25 00:48:21]

This function is funky? Either it doesn't need to check the prefix or this
method doesn't need to do the StartWithASCII check above.

What if this was RemoveExpectedPrefix() and nuke the StartsWithAscii() above...
or something. 

Maybe this class maybe could use a set of helpers to MakeThisOrThatKey(...) and
ParseThisOrThatKey(...) for each kind of key and use those in place startswith
and prefix manipulations.

[nhiroki, 2014/04/25 11:01:05]

Done.

+    DCHECK(success);
+    origins->insert(GURL(origin));
+  }
+  return true;
+}
+
+bool ServiceWorkerDatabase::GetRegistrationsForOrigin(
+    const GURL& origin,
+    std::vector<ServiceWorkerRegistrationData>* registrations) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK(registrations);
+
+  if (!LazyOpen(false) || is_disabled_)
+    return false;
+
+  // Create a key prefix for registrations.
+  std::string prefix = base::StringPrintf(
+      "%s%s%c", kRegKeyPrefix, origin.spec().c_str(), kKeySeparator);
+
+  scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
+  for (itr->Seek(prefix); itr->Valid(); itr->Next()) {
+    if (!itr->status().ok()) {
+      HandleError(FROM_HERE, itr->status());
+      registrations->clear();
+      return false;
+    }
+
+    std::string key = itr->key().ToString();
+    if (!StartsWithASCII(key, prefix, true))
+      break;
+
+    ServiceWorkerRegistrationData registration;
+    if (!ParseRegistrationData(itr->value().ToString(), &registration)) {
+      HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
+      registrations->clear();
+      return false;
+    }
+    registrations->push_back(registration);
+  }
+  return true;
+}
+
+bool ServiceWorkerDatabase::ReadRegistration(
+    int64 registration_id,
+    const GURL& origin,
+    ServiceWorkerRegistrationData* registration,
+    std::vector<ServiceWorkerResourceRecord>* resources) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  DCHECK(registration);
+  DCHECK(resources);
+
+  if (!LazyOpen(false) || is_disabled_)
+    return false;
+
+  ServiceWorkerRegistrationData value;
+  if (!ReadRegistrationData(registration_id, origin, &value))
+    return false;
+
+  // TODO(nhiroki): Read ResourceRecords tied with this registration.
+
+  *registration = value;
+  return true;
+}
+
+bool ServiceWorkerDatabase::WriteRegistration(
+    const ServiceWorkerRegistrationData& registration,
+    const std::vector<ServiceWorkerResourceRecord>& resources) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  if (!LazyOpen(true) || is_disabled_)
+    return false;
+
+  leveldb::WriteBatch batch;
+  if (!BumpNextAvailableIdIfNeeded(
+          kNextRegIdKey, registration.registration_id(), &batch) ||
+      !BumpNextAvailableIdIfNeeded(
+          kNextVerIdKey, registration.version_id(), &batch)) {
+    return false;
+  }
+
+  PutUniqueOriginToBatch(GURL(registration.scope_url()).GetOrigin(), &batch);

[michaeln, 2014/04/25 00:48:21]

is leveldb smart about noticing when a write doesn't change anything?

[nhiroki, 2014/04/25 02:06:11]

AFAICS, leveldb always writes log record even if there isn't change.

If we want to check the existence of the origin in advance, we need perform one
read operation and may omit one write operation as follows:

<current>
origin exists    : 1 write
origin not exist : 1 write

<check in advance>
origin exists    : 1 read
origin not exist : 1 read & 1 write

The check sounds reasonable. Okay, I'll change here to check it in advance.

[nhiroki, 2014/04/25 11:01:05]

Let me address this in a following patch (added TODO comment).

+  PutRegistrationDataToBatch(registration, &batch);
+
+  // TODO(nhiroki): Write |resources| into the database.
+  return WriteBatch(&batch);
+}
+
+bool ServiceWorkerDatabase::UpdateVersionToActive(int64 registration_id,
+                                                  const GURL& origin) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  if (!LazyOpen(false) || is_disabled_)
+    return false;
+
+  ServiceWorkerRegistrationData registration;
+  if (!ReadRegistrationData(registration_id, origin, &registration))
+    return false;
+
+  registration.set_is_active(true);
+
+  leveldb::WriteBatch batch;
+  PutRegistrationDataToBatch(registration, &batch);
+  return WriteBatch(&batch);
+}
+
+bool ServiceWorkerDatabase::UpdateLastCheckTime(int64 registration_id,
+                                                const GURL& origin,
+                                                const base::Time& time) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  if (!LazyOpen(false) || is_disabled_)
+    return false;
+
+  ServiceWorkerRegistrationData registration;
+  if (!ReadRegistrationData(registration_id, origin, &registration))
+    return false;
+
+  DCHECK_LT(registration.last_update_check_time(), time.ToInternalValue());
+  registration.set_last_update_check_time(time.ToInternalValue());
+
+  leveldb::WriteBatch batch;
+  PutRegistrationDataToBatch(registration, &batch);
+  return WriteBatch(&batch);
+}
+
+bool ServiceWorkerDatabase::DeleteRegistration(int64 registration_id,
+                                               const GURL& origin) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+  if (!LazyOpen(false) || is_disabled_)
+    return false;
+
+  leveldb::WriteBatch batch;
+
+  // Remove |origin| from unique origins if a registration specified by
+  // |registration_id| is the only one for |origin|.
+  // TODO(nhiroki): Check the uniqueness by more efficient way.
+  std::vector<ServiceWorkerRegistrationData> registrations;
+  if (!GetRegistrationsForOrigin(origin, &registrations))
+    return false;
+  if (registrations.size() == 1 &&
+      registrations[0].registration_id() == registration_id) {
+    batch.Delete(CreateUniqueOriginKey(origin));
+  }
+
+  batch.Delete(CreateRegistrationKey(registration_id, origin));
+
+  // TODO(nhiroki): Delete ResourceRecords tied with this registration.
+  return WriteBatch(&batch);
+}
+
 bool ServiceWorkerDatabase::LazyOpen(bool create_if_needed) {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   if (IsOpen())
@@ -173,6 +400,32 @@ bool ServiceWorkerDatabase::ReadNextAvailableId(
   return true;
 }
 
+bool ServiceWorkerDatabase::ReadRegistrationData(
+    int64 registration_id,
+    const GURL& origin,
+    ServiceWorkerRegistrationData* registration) {
+  DCHECK(registration);
+
+  std::string key = CreateRegistrationKey(registration_id, origin);
+
+  std::string value;
+  leveldb::Status status = db_->Get(leveldb::ReadOptions(), key, &value);
+  if (!status.ok()) {
+    if (!status.IsNotFound())
+      HandleError(FROM_HERE, status);
+    return false;
+  }
+
+  ServiceWorkerRegistrationData parsed;
+  if (!ParseRegistrationData(value, &parsed)) {
+    HandleError(FROM_HERE, leveldb::Status::Corruption("failed to parse"));
+    return false;
+  }
+
+  *registration = parsed;
+  return true;
+}
+
 bool ServiceWorkerDatabase::WriteBatch(leveldb::WriteBatch* batch) {
   DCHECK(batch);
   DCHECK(!is_disabled_);
@@ -184,6 +437,17 @@ bool ServiceWorkerDatabase::WriteBatch(leveldb::WriteBatch* batch) {
   return true;
 }
 
+bool ServiceWorkerDatabase::BumpNextAvailableIdIfNeeded(

[michaeln, 2014/04/25 00:48:21]

As coded this function will have problems if an id is bumped twice in a batch
(which will be expected when we get to <resourceid,url> storage). It'd be nice
to cache the next values in this class anyway, read them once and never read
again, so bump becomes something like...

if (next_xxx_ <= used_id) {
  next_xxx_ = used_id + 1;
  Write(key, next_xxx_;
}

I think we could seed cached values in LazyOpen() too. When starting with an
empty db, don't bother reading them ever. Also don't bother writing any values
until Bump() is called. When starting with an existing db, no key means next_id
is "0". 

wdyt?

[nhiroki, 2014/04/25 11:01:05]

Good idea! Addressed it.

+    const char* id_key, int64 used_id, leveldb::WriteBatch* batch) {
+  DCHECK(batch);
+  int64 next_available_id;
+  if (!ReadNextAvailableId(id_key, &next_available_id))
+    return false;
+  if (next_available_id <= used_id)
+    batch->Put(id_key, base::Int64ToString(used_id + 1));
+  return true;
+}
+
 bool ServiceWorkerDatabase::IsOpen() {
   return db_.get() != NULL;
 }