Implement QM::GetOriginsModifiedSince for browser data deleter support

webkit/quota/quota_database.cc

Index: webkit/quota/quota_database.cc
diff --git a/webkit/quota/quota_database.cc b/webkit/quota/quota_database.cc
index 592a039bd447e7612fc681e3475ac25a193933e0..d1cd3bb4535000ef8bcb8cc86aeea5c5501748ff 100644
--- a/webkit/quota/quota_database.cc
+++ b/webkit/quota/quota_database.cc
@@ -21,12 +21,12 @@ namespace {
 
 // Definitions for database schema.
 
-const int kCurrentVersion = 2;
-const int kCompatibleVersion = 2;
+const int kCurrentVersion = 3;
+const int kCompatibleVersion = 3;
 
 const char kOriginsTable[] = "Origins";
 const char kHostQuotaTable[] = "HostQuotaTable";
-const char kOriginLastAccessTable[] = "OriginLastAccessTable";
+const char kOriginInfoTable[] = "OriginInfoTable";
 const char kGlobalQuotaKeyPrefix[] = "GlobalQuota-";
 const char kIsOriginTableBootstrapped[] = "IsOriginTableBootstrapped";
 
@@ -37,13 +37,14 @@ const struct {
   { kHostQuotaTable,
     "(host TEXT NOT NULL,"
     " type INTEGER NOT NULL,"
-    " quota INTEGER,"
+    " quota INTEGER DEFAULT 0,"
     " UNIQUE(host, type))" },
-  { kOriginLastAccessTable,
+  { kOriginInfoTable,
     "(origin TEXT NOT NULL,"
     " type INTEGER NOT NULL,"
-    " used_count INTEGER,"
-    " last_access_time INTEGER,"
+    " used_count INTEGER DEFAULT 0,"
+    " last_access_time INTEGER DEFAULT 0,"
+    " last_modified_time INTEGER DEFAULT 0,"
     " UNIQUE(origin, type))" },
 };
 
@@ -57,9 +58,17 @@ const struct {
     kHostQuotaTable,
     "(host)",
     false },
-  { "OriginLastAccessIndex",
-    kOriginLastAccessTable,
-    "(origin, last_access_time)",
+  { "OriginInfoIndex",
+    kOriginInfoTable,
+    "(origin)",
+    false },
+  { "OriginLastAccessTimeIndex",
+    kOriginInfoTable,
+    "(last_access_time)",
+    false },
+  { "OriginLastModifiedTimeIndex",
+    kOriginInfoTable,
+    "(last_modified_time)",
     false },
 };
 
@@ -185,14 +194,14 @@ bool QuotaDatabase::SetOriginLastAccessTime(
   if (FindOriginUsedCount(origin, type, &used_count)) {
     ++used_count;
     const char* kSql =
-        "UPDATE OriginLastAccessTable"
+        "UPDATE OriginInfoTable"
         " SET used_count = ?, last_access_time = ?"
         " WHERE origin = ? AND type = ?";
     if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
       return false;
   } else  {
     const char* kSql =
-        "INSERT INTO OriginLastAccessTable"
+        "INSERT INTO OriginInfoTable"
         " (used_count, last_access_time, origin, type)"
         " VALUES (?, ?, ?, ?)";
     if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
@@ -210,6 +219,39 @@ bool QuotaDatabase::SetOriginLastAccessTime(
   return true;
 }
 
+bool QuotaDatabase::SetOriginLastModifiedTime(

[Mike West, 2011/06/16 09:39:33]

It's likely that last modified and last accessed will be changing
simultaneously.  You'll still need separate methods for things like `touch`, but
most of the time the values will be the same, right?  Since it requires going
out to the database to change both values, it probably makes sense to have a
mechanism for setting both with one SQL query in the common case.

[kinuko, 2011/06/16 12:09:06]

Hmm in the current QuotaManager design we have separate notification methods for
'read (access)' and 'write (modify)', and as far as we want to keep this design
I would be tempted to keep having separate methods for each.

We can change the quota manager<->client messaging design as well, but I'm not
very convinced to do so (yet?) because:
* At least for 'access time' we'll want a separate call since most 'read'
operation won't come with 'write'.
* As for 'modify time' it might be true that we should also update 'access'
time, but there're still some unclear situations--- for example for a long write
which spans multiple write tasks we may want to make multiple 'modify'
notifications (we do this in file write), but probably won't update 'used_count'
for each.

As for database write cost, in general these 'SetXxxTime' calls can be made very
frequently, and to alleviate the cost we try batching multiple database writes
in a fewer transactions.

Does that make sense?  Or if you have more comments/suggestions I'd love to
hear.

+    const GURL& origin, StorageType type, base::Time last_modified_time) {
+  if (!LazyOpen(true))
+    return false;
+
+  sql::Statement statement;
+
+  int dummy;
+  if (FindOriginUsedCount(origin, type, &dummy)) {

[Mike West, 2011/06/16 09:39:33]

This idiom seems heavy.  You do a SQL query to determine if the value exists in
the database, then either update or insert accordingly.  Does the version of
SQLite we bundle support `INSERT OR REPLACE`?  If it does, I'm pretty sure you
can wrap most of this logic up into a single (albeit more complex) SQL query. 
See
http://stackoverflow.com/questions/418898/sqlite-upsert-not-insert-or-replace...
for example.

The same would be true for `SetOriginLastAccessTime` above.

[kinuko, 2011/06/16 12:09:06]

(I think I've looked at the same stackoverflow thread... :))

As for SetOriginLastModifiedTime we want to preserve two column values
(used_count and last_access_time) from an existing entry for REPLACE case, as
far as we want to have separate methods for last-mod and last-access time.  In
that case I need to write 'select' subclause twice for each column right?  I
didn't want to do that but I might be missing something.

As for SetOriginLastAccessTime we want to increment used_count if there already
is an entry, and I couldn't find easier/neat sql-only solution.  If there's a
simpler notion I'd love to follow.

(Btw I found that it looks possible to revise SetHostQuota using INSERT OR
REPLACE, so I updated the code.)

+    const char* kSql =
+        "UPDATE OriginInfoTable"
+        " SET last_modified_time = ?"
+        " WHERE origin = ? AND type = ?";
+    if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
+      return false;
+  } else {
+    const char* kSql =
+        "INSERT INTO OriginInfoTable"
+        " (last_modified_time, origin, type)  VALUES (?, ?, ?)";
+    if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
+      return false;
+  }
+
+  statement.BindInt64(0, last_modified_time.ToInternalValue());
+  statement.BindString(1, origin.spec());
+  statement.BindInt(2, static_cast<int>(type));
+  if (!statement.Run())
+    return false;
+
+  ScheduleCommit();
+  return true;
+}
+
 bool QuotaDatabase::RegisterOrigins(const std::set<GURL>& origins,
                                     StorageType type,
                                     base::Time last_access_time) {
@@ -220,17 +262,18 @@ bool QuotaDatabase::RegisterOrigins(const std::set<GURL>& origins,
   for (itr_type itr = origins.begin(), end = origins.end();
        itr != end; ++itr) {
     const char* kSql =
-        "INSERT OR IGNORE INTO OriginLastAccessTable"
-        " (used_count, last_access_time, origin, type)"
-        " VALUES (?, ?, ?, ?)";
+        "INSERT OR IGNORE INTO OriginInfoTable"
+        " (used_count, last_access_time, last_modified_time, origin, type)"
+        " VALUES (?, ?, ?, ?, ?)";
     sql::Statement statement;
     if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
       return false;
 
-    statement.BindInt(0, 0);  // used_count
+    statement.BindInt(0, 0);    // used_count
     statement.BindInt64(1, last_access_time.ToInternalValue());
-    statement.BindString(2, itr->spec());
-    statement.BindInt(3, static_cast<int>(type));
+    statement.BindInt64(2, 0);  // last modified time

[Mike West, 2011/06/16 09:39:33]

Here, last_modified_time is inserted as the third column.  On line 626, it's
read out as the fifth column.  I'd suggest keeping the column order consistent.

[kinuko, 2011/06/16 12:09:06]

Done.

+    statement.BindString(3, itr->spec());
+    statement.BindInt(4, static_cast<int>(type));
     if (!statement.Run())
       return false;
   }
@@ -261,13 +304,13 @@ bool QuotaDatabase::DeleteHostQuota(
   return true;
 }
 
-bool QuotaDatabase::DeleteOriginLastAccessTime(
+bool QuotaDatabase::DeleteOriginInfo(
     const GURL& origin, StorageType type) {
   if (!LazyOpen(false))
     return false;
 
   const char* kSql =
-      "DELETE FROM OriginLastAccessTable"
+      "DELETE FROM OriginInfoTable"
       " WHERE origin = ? AND type = ?";
 
   sql::Statement statement;
@@ -304,7 +347,7 @@ bool QuotaDatabase::GetLRUOrigin(
   if (!LazyOpen(false))
     return false;
 
-  const char* kSql = "SELECT origin FROM OriginLastAccessTable"
+  const char* kSql = "SELECT origin FROM OriginInfoTable"
                      " WHERE type = ?"
                      " ORDER BY last_access_time ASC";
 
@@ -328,6 +371,28 @@ bool QuotaDatabase::GetLRUOrigin(
   return statement.Succeeded();
 }
 
+bool QuotaDatabase::GetOriginsModifiedSince(
+    StorageType type, std::set<GURL>* origins, base::Time modified_since) {
+  DCHECK(origins);
+  if (!LazyOpen(false))
+    return false;
+
+  const char* kSql = "SELECT origin FROM OriginInfoTable"
+                     " WHERE type = ? AND last_modified_time > ?";
+
+  sql::Statement statement;
+  if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
+    return false;
+  statement.BindInt(0, static_cast<int>(type));
+  statement.BindInt64(1, modified_since.ToInternalValue());
+
+  origins->clear();
+  while (statement.Step())
+    origins->insert(GURL(statement.ColumnString(0)));
+
+  return statement.Succeeded();
+}
+
 bool QuotaDatabase::IsOriginDatabaseBootstrapped() {
   if (!LazyOpen(true))
     return false;
@@ -369,7 +434,7 @@ bool QuotaDatabase::FindOriginUsedCount(
     return false;
 
   const char* kSql =
-      "SELECT used_count FROM OriginLastAccessTable"
+      "SELECT used_count FROM OriginInfoTable"
       " WHERE origin = ? AND type = ?";
 
   sql::Statement statement;
@@ -540,24 +605,25 @@ bool QuotaDatabase::DumpQuotaTable(QuotaTableCallback* callback) {
   return statement.Succeeded();
 }
 
-bool QuotaDatabase::DumpLastAccessTimeTable(
-    LastAccessTimeTableCallback* callback) {
-  scoped_ptr<LastAccessTimeTableCallback> callback_deleter(callback);
+bool QuotaDatabase::DumpOriginInfoTable(
+    OriginInfoTableCallback* callback) {
+  scoped_ptr<OriginInfoTableCallback> callback_deleter(callback);
 
   if (!LazyOpen(true))
     return false;
 
-  const char* kSql = "SELECT * FROM OriginLastAccessTable";
+  const char* kSql = "SELECT * FROM OriginInfoTable";
   sql::Statement statement;
   if (!PrepareCachedStatement(db_.get(), SQL_FROM_HERE, kSql, &statement))
     return false;
 
   while (statement.Step()) {
-    LastAccessTimeTableEntry entry = {
+    OriginInfoTableEntry entry = {
       GURL(statement.ColumnString(0)),
       static_cast<StorageType>(statement.ColumnInt(1)),
       statement.ColumnInt(2),
-      base::Time::FromInternalValue(statement.ColumnInt64(3))
+      base::Time::FromInternalValue(statement.ColumnInt64(3)),
+      base::Time::FromInternalValue(statement.ColumnInt64(4))
     };
 
     if (!callback->Run(entry))
@@ -576,8 +642,8 @@ bool operator<(const QuotaDatabase::QuotaTableEntry& lhs,
   return lhs.quota < rhs.quota;
 }
 
-bool operator<(const QuotaDatabase::LastAccessTimeTableEntry& lhs,
-               const QuotaDatabase::LastAccessTimeTableEntry& rhs) {
+bool operator<(const QuotaDatabase::OriginInfoTableEntry& lhs,
+               const QuotaDatabase::OriginInfoTableEntry& rhs) {
   if (lhs.origin < rhs.origin) return true;
   if (rhs.origin < lhs.origin) return false;
   if (lhs.type < rhs.type) return true;