Improve IndexedDB's quota support

content/browser/in_process_webkit/indexed_db_dispatcher_host.cc

Index: content/browser/in_process_webkit/indexed_db_dispatcher_host.cc
diff --git a/content/browser/in_process_webkit/indexed_db_dispatcher_host.cc b/content/browser/in_process_webkit/indexed_db_dispatcher_host.cc
index 16ca3a7781e50943c23d8640cff2e737cb9a4e1a..dcee92f9a2c30d2412b966f748c7165fcf6010d2 100644
--- a/content/browser/in_process_webkit/indexed_db_dispatcher_host.cc
+++ b/content/browser/in_process_webkit/indexed_db_dispatcher_host.cc
@@ -28,7 +28,6 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebVector.h"
 #include "webkit/glue/webkit_glue.h"
-#include "webkit/quota/quota_manager.h"
 
 using WebKit::WebDOMStringList;
 using WebKit::WebExceptionCode;
@@ -155,7 +154,8 @@ int32 IndexedDBDispatcherHost::Add(WebIDBDatabase* idb_database,
     return 0;
   }
   int32 idb_database_id = database_dispatcher_host_->map_.Add(idb_database);
-  database_dispatcher_host_->url_map_[idb_database_id] = origin_url;
+  Context()->NewConnection(origin_url);
+  database_dispatcher_host_->database_url_map_[idb_database_id] = origin_url;
   return idb_database_id;
 }
 
@@ -179,18 +179,21 @@ int32 IndexedDBDispatcherHost::Add(WebIDBObjectStore* idb_object_store) {
   return object_store_dispatcher_host_->map_.Add(idb_object_store);
 }
 
-int32 IndexedDBDispatcherHost::Add(WebIDBTransaction* idb_transaction) {
+int32 IndexedDBDispatcherHost::Add(WebIDBTransaction* idb_transaction,
+                                   const GURL& url) {
   if (!transaction_dispatcher_host_.get()) {
     delete idb_transaction;
     return 0;
   }
   int32 id = transaction_dispatcher_host_->map_.Add(idb_transaction);
   idb_transaction->setCallbacks(new IndexedDBTransactionCallbacks(this, id));
+  transaction_dispatcher_host_->transaction_url_map_[id] = url;
   return id;
 }
 
 void IndexedDBDispatcherHost::OnIDBFactoryOpen(
     const IndexedDBHostMsg_FactoryOpen_Params& params) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
   FilePath base_path = webkit_context_->data_path();
   FilePath indexed_db_path;
   if (!base_path.empty()) {
@@ -226,6 +229,8 @@ void IndexedDBDispatcherHost::OnIDBFactoryOpen(
     backingStoreType = WebKit::WebIDBFactory::SQLiteBackingStore;
   }
 
+  // TODO(dgrogan): Don't let a non-existing database be opened (and therefore
+  // created) if this origin is already over quota.
   Context()->GetIDBFactory()->open(
       params.name,
       new IndexedDBCallbacks<WebIDBDatabase>(this, params.response_id,
@@ -255,6 +260,12 @@ void IndexedDBDispatcherHost::OnIDBFactoryDeleteDatabase(
       webkit_glue::FilePathToWebString(indexed_db_path));
 }
 
+void IndexedDBDispatcherHost::TransactionComplete(int32 transaction_id) {
+  Context()->TransactionComplete(
+      transaction_dispatcher_host_->transaction_url_map_[transaction_id]);

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

where are transaction_url_map_ entries erased?

+  transaction_dispatcher_host_->transaction_size_map_.erase(transaction_id);

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

maybe let TransactionDispatcherHost::OnDestroyed take care of erasing from both
the transaction_url and transaction_size map in all cases

+}
+
 //////////////////////////////////////////////////////////////////////
 // Helper templates.
 //
@@ -371,6 +382,10 @@ void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnCreateObjectStore(
       params.name, params.key_path, params.auto_increment,
       *idb_transaction, *ec);
   *object_store_id = *ec ? 0 : parent_->Add(object_store);
+  if (parent_->Context()->IsOverQuota(

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

Is there a WebExceptionCode that means 'quota error'? I'm wondering if this
should fail with a non-zero *ec prior to creating the object store.

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

QUOTA_EXCEEDED_ERR (22)?

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

Same comment as below about what's in the spec.  In short: I'm not sure.

+      database_url_map_[params.idb_database_id])) {
+    idb_transaction->abort();
+  }
 }
 
 void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnDeleteObjectStore(
@@ -404,7 +419,8 @@ void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnSetVersion(
   *ec = 0;
   idb_database->setVersion(
       version,
-      new IndexedDBCallbacks<WebIDBTransaction>(parent_, response_id),
+      new IndexedDBCallbacks<WebIDBTransaction>(parent_, response_id,
+          database_url_map_[idb_database_id]),
       *ec);
 }
 
@@ -430,7 +446,8 @@ void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnTransaction(
   WebIDBTransaction* transaction = database->transaction(
       object_stores, mode, timeout, *ec);
   DCHECK(!transaction != !*ec);

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

unrelated: curious DCHECK :)

-  *idb_transaction_id = *ec ? 0 : parent_->Add(transaction);
+  *idb_transaction_id =
+      *ec ? 0 : parent_->Add(transaction, database_url_map_[idb_database_id]);
 }
 
 void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnOpen(
@@ -445,10 +462,9 @@ void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnClose(
   WebIDBDatabase* database = parent_->GetOrTerminateProcess(
       &map_, idb_database_id);
   database->close();
-  parent_->Context()->quota_manager_proxy()->NotifyStorageAccessed(
-      quota::QuotaClient::kIndexedDatabase, url_map_[idb_database_id],
-      quota::kStorageTypeTemporary);
-  url_map_.erase(idb_database_id);
+  const GURL origin_url = database_url_map_[idb_database_id];
+  database_url_map_.erase(idb_database_id);
+  parent_->Context()->ConnectionClosed(origin_url);

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

Is OnClose called in all cases or can this method be skipped and OnDestroyed get
called if the object is gc'd?

Would it make sense to move this cleanup code to OnDestroyed? Since that is
where parent_->DestroyObject(&map_, id) is called to remove the WebIDBDatabase
instance from the primary map and delete the instance, i'm wondering if that's a
more appropiate place for this cleanup too.

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

Yes it would.  Moved.

[michaeln, 2011/08/05 01:21:47]

Any reason not to move the ConnectionClosed() call to OnDestroyed as well? If it
bother you that the context method is called 'closed', maybe rename it to
ConnectionDestroyed(). The important part is to eventually report deltas in
usage, doing so on close/destroyed time just helps pick up compaction action
that happens asyncly to transaction action. Presuming OnDestroyed is more robust
for map cleanup, why not for this cleanup purpose too? The cleanup logic should
stay together.

[dgrogan, 2011/08/05 03:06:58]

Moved.  Though I'm not sure that OnDestroyed is more robust.  I have no
observations to support that or anything, I really just don't know.

 }
 
 void IndexedDBDispatcherHost::DatabaseDispatcherHost::OnDestroyed(
@@ -704,6 +720,12 @@ void IndexedDBDispatcherHost::ObjectStoreDispatcherHost::OnPut(
       new IndexedDBCallbacks<WebIDBKey>(parent_, params.response_id));
   idb_object_store->put(params.serialized_value, params.key, params.put_mode,
                         callbacks.release(), *idb_transaction, *ec);
+  if (*ec)
+    return;
+  int64 size = UTF16ToUTF8(params.serialized_value.data()).size();
+  WebIDBTransactionIDToSizeMap* map =
+      &parent_->transaction_dispatcher_host_->transaction_size_map_;
+  (*map)[params.transaction_id] += size;
 }
 
 void IndexedDBDispatcherHost::ObjectStoreDispatcherHost::OnDelete(
@@ -761,6 +783,12 @@ void IndexedDBDispatcherHost::ObjectStoreDispatcherHost::OnCreateIndex(
   WebIDBIndex* index = idb_object_store->createIndex(
       params.name, params.key_path, params.unique, *idb_transaction, *ec);
   *index_id = *ec ? 0 : parent_->Add(index);
+  WebIDBObjectIDToURLMap* transaction_url_map =

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

Getting a handle to this variable is just for readability and to make 80-char
linebreaks easier.

+      &parent_->transaction_dispatcher_host_->transaction_url_map_;
+  if (parent_->Context()->IsOverQuota(

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

Is there a WebExceptionCode that means 'quota error'? I'm wondering if this
should fail with a non-zero *ec prior to creating the index.

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

I did it this way because of the following text from the spec.  It's not
precisely on point because we're not asynchronously running into problems. 
Though if we were asynchronously running into problems, we would already have
created and returned an IDBIndex object so the whole paragraph would be moot. 
So I think the way it was intended to be written covers this case.

In some implementations it's possible for the implementation to asynchronously
run into problems creating the index after the createIndex function has
returned. For example in implementations where metadata about the newly created
index is queued up to be inserted into the database asynchronously, or where the
implementation might need to ask the user for permission for quota reasons. Such
implementations must still create and return a IDBIndex object. Instead, once
the implementation realizes that creating the index has failed, it must abort
the transaction using the steps for aborting a transaction. 

+      (*transaction_url_map)[params.transaction_id])) {
+    idb_transaction->abort();
+  }
 }
 
 void IndexedDBDispatcherHost::ObjectStoreDispatcherHost::OnIndex(
@@ -1033,6 +1061,12 @@ void IndexedDBDispatcherHost::
   if (!idb_transaction)
     return;
 
+  if (parent_->Context()->WouldBeOverQuota(
+      transaction_url_map_[transaction_id],
+      transaction_size_map_[transaction_id])) {
+    idb_transaction->abort();

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

When trans->abort() is called, what does application script see as a result? Is
there any exception/error code associated with the event or callback that is
seen at that script level?

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

I don't think so.  This was bothering me as well, that we don't tell the web
author why their transaction suddenly aborted.  The previous quota layout test
didn't indicate that there was any way to know.  I think, as specced, this is
the best we can do.  But _something_ needs to change so that authors know that
the failure is quota related.

[michaeln, 2011/08/04 00:23:27]

Probably worthy of a TODO? We definitely want to indicate to the caller
QUOTA_ERROR in some way.

[dgrogan, 2011/08/04 19:47:50]

Done.

+    return;
+  }
   idb_transaction->didCompleteTaskEvents();
 }