Improve IndexedDB's quota support

content/browser/in_process_webkit/indexed_db_context.cc

Index: content/browser/in_process_webkit/indexed_db_context.cc
diff --git a/content/browser/in_process_webkit/indexed_db_context.cc b/content/browser/in_process_webkit/indexed_db_context.cc
index 8e091c1678572dca7c3192b74addfff3e3a338cf..20f22d2bd5080f3b28953442f90ae83986c70484 100644
--- a/content/browser/in_process_webkit/indexed_db_context.cc
+++ b/content/browser/in_process_webkit/indexed_db_context.cc
@@ -8,6 +8,7 @@
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
 #include "base/string_util.h"
+#include "base/task.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/in_process_webkit/indexed_db_quota_client.h"
@@ -18,10 +19,12 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebIDBFactory.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
+#include "webkit/database/database_util.h"
 #include "webkit/glue/webkit_glue.h"
 #include "webkit/quota/quota_manager.h"
 #include "webkit/quota/special_storage_policy.h"
 
+using webkit_database::DatabaseUtil;
 using WebKit::WebIDBDatabase;
 using WebKit::WebIDBFactory;
 using WebKit::WebSecurityOrigin;
@@ -103,7 +106,7 @@ FilePath IndexedDBContext::GetIndexedDBFilePath(
 void IndexedDBContext::DeleteIndexedDBFile(const FilePath& file_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
   // TODO(pastarmovj): Close all database connections that use that file.
-  file_util::Delete(file_path, false);
+  file_util::Delete(file_path, true /*recursive*/);
 }
 
 void IndexedDBContext::DeleteIndexedDBForOrigin(const string16& origin_id) {
@@ -115,6 +118,17 @@ void IndexedDBContext::DeleteIndexedDBForOrigin(const string16& origin_id) {
   DeleteIndexedDBFile(GetIndexedDBFilePath(origin_id));
 }
 
+// TODO(dgrogan): Once we can delete IndexedDB directories out from underneath
+// open webkit instances, merge this and DeleteIndexedDBForOrigin.
+void IndexedDBContext::EvictOrigin(const GURL& origin_url) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
+  if (connection_count_.find(origin_url) == connection_count_.end()) {
+    string16 origin_id = DatabaseUtil::GetOriginIdentifier(origin_url);
+    FilePath idb_directory = GetIndexedDBFilePath(origin_id);
+    DeleteIndexedDBFile(idb_directory);
+  }
+}
+
 bool IndexedDBContext::IsUnlimitedStorageGranted(
     const GURL& origin) const {
   return special_storage_policy_->IsStorageUnlimited(origin);
@@ -137,6 +151,148 @@ void IndexedDBContext::GetAllOriginIdentifiers(
   }
 }
 
+int64 IndexedDBContext::InitializeDiskUsage(const GURL& origin_url) {
+  int64 disk_usage = ReadUsageFromDisk(origin_url);

[dgrogan, 2011/08/03 01:41:29]

This deviates from "the Michael design."  This function is run when the QM
requests issues a GetOriginUsage.  The locally-cached value is overwritten to
ensure that it's in sync with what the QM thinks.

From what I've seen GetOriginUsage is called before anything else primes the
cache (origin_size_map_) anyway.

[michaeln, 2011/08/03 03:40:40]

Seems reasonable, although if you do have a cached value, why not use it instead
of the more expensive computation?
The way the QM is coded now, i think you'll only get queried once per origin for
usage. Not sure if that will remain true going forward.

[kinuko, 2011/08/03 09:12:22]

(fwiw: I'm planning to make QM purge its cache periodically.)

[dgrogan, 2011/08/04 23:44:55]

I remember why this deviated.  The test code writes files in the leveldb
directory of a certain size and then simulates a quota_manager.GetOriginUsage
call.  When the test wants to change the fake leveldb file size it doesn't go
through indexeddb, it just writes a different file size.  So IndexedDBContext
doesn't know that it needs to re-stat the directory and returns the cached size,
causing the test to fail.

That itself isn't a great reason to have this method reset the cache, but I do
like the safety net this provides: if there's some broken logic here about when
to update the cache or notify the QM of a delta, a GetOriginUsage call will
bypass all that stuff and get a true value.

WDYT?

+  origin_size_map_[origin_url] = disk_usage;
+  return disk_usage;
+}
+
+int64 IndexedDBContext::ReadUsageFromDisk(const GURL& origin_url) const {
+  string16 origin_id = DatabaseUtil::GetOriginIdentifier(origin_url);
+  FilePath file_path = GetIndexedDBFilePath(origin_id);
+  return file_util::ComputeDirectorySize(file_path);
+}
+
+void IndexedDBContext::ConnectionClosed(const GURL& origin_url) {
+  if (quota_manager_proxy()) {
+    quota_manager_proxy()->NotifyStorageAccessed(
+        quota::QuotaClient::kIndexedDatabase, origin_url,
+        quota::kStorageTypeTemporary);
+  }
+  connection_count_[origin_url] --;

[kinuko, 2011/08/03 09:12:22]

nit: extra space before -- (here and at line 185)

[dgrogan, 2011/08/03 21:45:47]

Done.

+  if (connection_count_[origin_url] == 0) {
+    QueryDiskAndUpdateQuotaUsage(origin_url);
+    connection_count_.erase(origin_url);
+  }
+}
+
+void IndexedDBContext::NewConnection(const GURL& origin_url) {
+  if (quota_manager_proxy()) {
+    quota_manager_proxy()->NotifyStorageAccessed(
+        quota::QuotaClient::kIndexedDatabase, origin_url,
+        quota::kStorageTypeTemporary);
+  }
+  connection_count_[origin_url] ++;

[michaeln, 2011/08/03 03:40:40]

Seems like you may want to ensure you have a cached value for this origin in
origin_size_map_ at this point. Looks like other methods assume its populated,
but the only thing that guarantees it is an external caller having called
InitializeDiskUsage prior to those things happening.

[dgrogan, 2011/08/04 23:44:55]

Done.

+  QueryAvailableQuota(origin_url);
+}
+
+bool IndexedDBContext::WouldBeOverQuota(const GURL& origin_url,
+                                        int64 additional_bytes) {
+  if (origin_allocated_quota_map_.find(origin_url) ==
+      origin_allocated_quota_map_.end()) {
+    // We haven't heard back from the QuotaManager yet, just let it through.

[dgrogan, 2011/08/03 01:41:29]

This happened a few times in the existing browser tests but I couldn't repro it
when actually running chrome.

+    return false;
+  }
+  bool over_quota = additional_bytes + origin_size_map_[origin_url] >

[michaeln, 2011/08/03 03:40:40]

Is this comparing apples to oranges?

The lhs is in terms of IndexDB usage only, while the rhs is total usage across
all storage types. Seems like you want the rhs to be the remaining
space_available for the origin.

return additional_bytes > space_available;

[dgrogan, 2011/08/03 21:45:47]

No.  I'm glad you noticed this.

+      origin_allocated_quota_map_[origin_url];
+  return over_quota;
+}
+
+bool IndexedDBContext::IsOverQuota(const GURL& origin_url) {
+  return WouldBeOverQuota(origin_url, 0 /*additional_bytes*/);

[michaeln, 2011/08/03 03:40:40]

return space_available <= 0;

[dgrogan, 2011/08/03 21:45:47]

I did space_available < 1

+}
+
+void IndexedDBContext::TransactionComplete(const GURL& origin_url) {
+  QueryDiskAndUpdateQuotaUsage(origin_url);
+  QueryAvailableQuota(origin_url);
+}
+
+void IndexedDBContext::QueryDiskAndUpdateQuotaUsage(const GURL& origin_url) {
+  int64 former_disk_usage = origin_size_map_[origin_url];
+  int64 current_disk_usage = ReadUsageFromDisk(origin_url);
+  int64 difference = current_disk_usage - former_disk_usage;
+  if (difference) {
+    quota_manager_proxy()->NotifyStorageModified(
+        quota::QuotaClient::kIndexedDatabase,
+        origin_url,
+        quota::kStorageTypeTemporary,
+        difference);
+    origin_size_map_[origin_url] = current_disk_usage;
+  }
+}
+
+void IndexedDBContext::GotUpdatedQuota(const GURL& origin_url, int64 usage,
+                                       int64 quota) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
+  origin_allocated_quota_map_[origin_url] = quota;

[michaeln, 2011/08/03 03:40:40]

do u really want space_available_map_[origin_url] = quota - usage;

[dgrogan, 2011/08/03 21:45:47]

Yes I do.

+}
+
+void GotUpdatedQuota(IndexedDBContext* context, const GURL& origin_url,

[michaeln, 2011/08/03 03:40:40]

this should be static or in an anon namespace if its really needed

[dgrogan, 2011/08/03 21:45:47]

deleted

+                            int64 usage, int64 quota) {
+  context->GotUpdatedQuota(origin_url, usage, quota);
+}
+
+class IndexedDBGetUsageAndQuotaCallback :
+    public quota::QuotaManager::GetUsageAndQuotaCallback {
+ public:
+  IndexedDBGetUsageAndQuotaCallback(IndexedDBContext* context,
+                                    const GURL& origin_url)
+      : context_(context),
+        origin_url_(origin_url) {
+  }
+
+  void Run(quota::QuotaStatusCode status, int64 usage, int64 quota) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    DCHECK(status == quota::kQuotaStatusOk || status == quota::kQuotaErrorAbort)
+        << "status was " << status;
+    if (status == quota::kQuotaErrorAbort) {
+      // We seem to no longer care to wait around for the answer.
+      return;
+    }
+    BrowserThread::PostTask(BrowserThread::WEBKIT, FROM_HERE,
+        NewRunnableFunction(&GotUpdatedQuota,

[michaeln, 2011/08/03 03:40:40]

would a runnable object method work here inplace of the intermediary function?

[dgrogan, 2011/08/03 21:45:47]

Done.

+                            context_,
+                            origin_url_,
+                            usage,
+                            quota));
+  }
+
+  virtual void RunWithParams(
+        const Tuple3<quota::QuotaStatusCode, int64, int64>& params) {
+    Run(params.a, params.b, params.c);
+  }
+
+ private:
+  scoped_refptr<IndexedDBContext> context_;
+  const GURL origin_url_;
+};
+
+void RequestQuota(

[michaeln, 2011/08/03 03:40:40]

ditto comment about static or anon namespace

[dgrogan, 2011/08/03 21:45:47]

Now in an anon namespace.

+    scoped_refptr<quota::QuotaManagerProxy> proxy, const GURL& gurl,
+    IndexedDBGetUsageAndQuotaCallback* idb_quota_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!proxy->quota_manager()) {

[dgrogan, 2011/08/03 21:45:47]

If I switched to NewRunnableMethod, I'm worried about this check being deleted.

+    // QuotaManager was deleted on the IO thread just before this RequestQuota
+    // task was run.
+    return;
+  }
+  proxy->quota_manager()->GetUsageAndQuota(
+      gurl,
+      quota::kStorageTypeTemporary,
+      idb_quota_callback);
+}
+
+void IndexedDBContext::QueryAvailableQuota(const GURL& origin_url) {
+  IndexedDBGetUsageAndQuotaCallback* callback =
+      new IndexedDBGetUsageAndQuotaCallback(this, origin_url);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      NewRunnableFunction(&RequestQuota,

[michaeln, 2011/08/03 03:40:40]

ditto question about runnable object method

[dgrogan, 2011/08/03 21:45:47]

It seems like I'd want to post a
RunnableMethod(quota_manager_proxy()->quota_manager(),
&QuotaManager::GetUsageAndQuota,...) but quota_manager() DCHECKS that it's being
accessed on the IO thread, which this is not on.

+                          scoped_refptr<quota::QuotaManagerProxy>(
+                              quota_manager_proxy()),
+                          origin_url,
+                          callback));
+}
+
 quota::QuotaManagerProxy* IndexedDBContext::quota_manager_proxy() {
   return quota_manager_proxy_;
 }