Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7470008:
Improve IndexedDB's quota support (Closed)
|
Created:
9 years, 5 months ago by dgrogan Modified:
9 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, darin-cc_chromium.org, Paweł Hajdan Jr., hans, gregsimon Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImprove IndexedDB's quota support
* Check available quota before storing anything
* Inform quota manager of storage updates
* Evict an origin when quota manager requests
BUG=83652
TEST=llvm/Debug/unit_tests --gtest_filter=IndexedDBQuotaClientTest.* && llvm/Debug/browser_tests --gtest_filter=IndexedDBBrowser*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95691
Patch Set 1 #Patch Set 2 : style problems #
Total comments: 27
Patch Set 3 : michael's new approach #Patch Set 4 : don't allow creation of indices or object stores if over quota #Patch Set 5 : add TODO about opening database when over quota #
Total comments: 53
Patch Set 6 : improve names and comments, better map cleanup #
Total comments: 12
Patch Set 7 : fixed easy comments #
Total comments: 17
Patch Set 8 : merged delete methods; simplify GetUsageCallback #
Total comments: 16
Patch Set 9 : finally responded to everything #
Total comments: 2
Patch Set 10 : remove unused method declaration #
Total comments: 10
Patch Set 11 : respond to comments, add clean up when renderer process goes away #
Total comments: 2
Patch Set 12 : remove NOTREACHED #Messages
Total messages: 25 (0 generated)
Let me know if you want me to split this up. I was hoping to get it into m14, but that looks pretty slim now. Though, if this doesn't get in IndexedDB would probably have to come out so it may end up getting patched in. http://codereview.chromium.org/7470008/diff/2001/webkit/appcache/appcache_quo... File webkit/appcache/appcache_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/webkit/appcache/appcache_quo... webkit/appcache/appcache_quota_client.cc:104: // DCHECK(!host.empty()); This was causing our existing browser tests to fail. I think host is empty when local files are being served?
On 2011/07/26 01:57:07, dgrogan wrote: > Let me know if you want me to split this up. > > Though, if this doesn't get in IndexedDB would probably have to come out LevelDB, not IndexedDB.
http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:227: // safely delete them here. Kinuko, can you confirm that this is true?
http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:227: // safely delete them here. On 2011/07/26 02:02:16, dgrogan wrote: > Kinuko, can you confirm that this is true? It's half true-- it won't be called while it is in use for eviction, but after we make a change like this BrowsingDataDeleter will also be calling this one, which might be kicked by UI action. http://codereview.chromium.org/7129018/ ...or the manager could avoid calling this whenever the origin is in use. Maybe we should do that?
http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:227: // safely delete them here. On 2011/07/26 06:35:28, kinuko wrote: > On 2011/07/26 02:02:16, dgrogan wrote: > > Kinuko, can you confirm that this is true? > > It's half true-- it won't be called while it is in use for eviction, but after > we make a change like this BrowsingDataDeleter will also be calling this one, > which might be kicked by UI action. > http://codereview.chromium.org/7129018/ > > ...or the manager could avoid calling this whenever the origin is in use. Maybe > we should do that? That would be great for IDB.
http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_callbacks.h (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_callbacks.h:84: GURL origin_url() const { return origin_url_; } can this be a const GURL& return value? http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_context.cc:105: // TODO(pastarmovj): Close all database connections that use that file. I see there is a close() method on WebIDBDatabase and that IndexedDBDispatcherHost::DatabaseDispatcherHost in the browser process has a collection of WebIDBDatabases... Would calling the close method on those instances related to 'origin' accomplish anything useful? http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_context.cc:120: void IndexedDBContext::EvictOrigin(const string16& origin_id) { This and the method above are functionally equivalent, the only difference is one has a DCHECK for 'chrome-extension' that the other doesn't? http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:204: // TODO(jorlow): This doesn't support file:/// urls properly. We probably need see DatabaseUtil::GetOriginFromIdentifier http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:709: idb_transaction->abort(); could idb_transaction have been deleted prior to this function call... ditto question about the function below? http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:722: idb_transaction_(idb_transaction) { nit: indent is off http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:775: // TODO(dgrogan): Figure out how accurate this is in terms of disk space used. Eavesdropping on OnPut/OnDelete/OnClear/OnIndex/OnDeleteIndex look like a reasonable way to determine if something has changed in the transaction, but i'm very skeptical of this technique to determine by how much its change by... presumably puts can overwrite existing values? I think it would be good to think thru some other approaches to measuring how much. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1097: WebIDBObjectStore* object_store = idb_transaction->objectStore(name, *ec); this looks like a mutation too http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1110: transaction_size_map_.end()) { i see... no Puts(), so no size map entry, so nothing todo http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1129: parent_->DestroyObject(&map_, object_id); this might be a good place to erase from the transaction_size_map_ http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.h:48: void TransactionComplete(int32 transaction_id); Would it make sense to make this method private? I guess that's not in keeping with how its used by IndexedDBTransactionCallbacks. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:52: scoped_ptr<DeletionCallback> callback_; It would be good to ensure that this callback object got deleted on the IO thread. Calling .reset() in Completed() and in an Aborted() OVERRIDE should take care of that. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:227: // safely delete them here. On 2011/07/26 17:46:15, dgrogan wrote: > On 2011/07/26 06:35:28, kinuko wrote: > > On 2011/07/26 02:02:16, dgrogan wrote: > > > Kinuko, can you confirm that this is true? > > > > It's half true-- it won't be called while it is in use for eviction, but after > > we make a change like this BrowsingDataDeleter will also be calling this one, > > which might be kicked by UI action. > > http://codereview.chromium.org/7129018/ > > > > ...or the manager could avoid calling this whenever the origin is in use. > Maybe > > we should do that? > > That would be great for IDB. I think we want to support deleting resources while they are in use. If a user gestures to delete all in the browsing data remover ui, we don't want to put up an alert that says "not all data could be deleted, close some tabs and try again". What happens in the appcache and sqldb case is that pages using the deleted resources get errors when trying to subsquently access the deleted resources. I'm not sure what happens in the case of the filesystem? http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:20: void IndexedDBTransactionCallbacks::onAbort() { It's also 'complete' in these other cases, just not successfully. How does the transaction_size_ map get cleaned up in those other cases?
http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:227: // safely delete them here. On 2011/07/27 01:31:35, michaeln wrote: > On 2011/07/26 17:46:15, dgrogan wrote: > > On 2011/07/26 06:35:28, kinuko wrote: > > > On 2011/07/26 02:02:16, dgrogan wrote: > > > > Kinuko, can you confirm that this is true? > > > > > > It's half true-- it won't be called while it is in use for eviction, but > after > > > we make a change like this BrowsingDataDeleter will also be calling this > one, > > > which might be kicked by UI action. > > > http://codereview.chromium.org/7129018/ > > > > > > ...or the manager could avoid calling this whenever the origin is in use. > > Maybe > > > we should do that? > > > > That would be great for IDB. > > I think we want to support deleting resources while they are in use. If a user > gestures to delete all in the browsing data remover ui, we don't want to put up > an alert that says "not all data could be deleted, close some tabs and try > again". > > What happens in the appcache and sqldb case is that pages using the deleted > resources get errors when trying to subsquently access the deleted resources. > I'm not sure what happens in the case of the filesystem? Currently the filesystem impl doesn't fully support deletion-in-use cases. It was in my TODO list but somehow has been forgotten. I'll file a bug if we're going along this direction. (We'd stop accepting new operations, cancel ongoing writes and delete data after all the inflight operations are done.)
Just big-picture stuff. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_context.cc:105: // TODO(pastarmovj): Close all database connections that use that file. On 2011/07/27 01:31:35, michaeln wrote: > I see there is a close() method on WebIDBDatabase and that > IndexedDBDispatcherHost::DatabaseDispatcherHost in the browser process has a > collection of WebIDBDatabases... Would calling the close method on those > instances related to 'origin' accomplish anything useful? Yes, that's how they will be closed, but more work needs to be done before we can call it on delete. We currently don't call it except in response to a javascript close(). There are some non-trivial semantics dictated by the spec surrounding open, close, delete, setVersion, and multiple connections to the backend. It's why we haven't yet exposed a database.delete() function. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:775: // TODO(dgrogan): Figure out how accurate this is in terms of disk space used. On 2011/07/27 01:31:35, michaeln wrote: > Eavesdropping on OnPut/OnDelete/OnClear/OnIndex/OnDeleteIndex look like a > reasonable way to determine if something has changed in the transaction, but i'm > very skeptical of this technique to determine by how much its change by... > presumably puts can overwrite existing values? Your concerns are valid. The issue is that LevelDB only appends to files in direct response to API calls. It records a note to delete values and overwrite values. The original values are left intact until a compaction, which happens according to LevelDB's internal heuristics. I erred mostly on the side of overcounting by not counting overwrites, deletes, or deleting an entire object store. I say "mostly" because the disk space used to temporarily store the delete/overwrite markers is not counted against the quota. My naive theory is that an application can trigger a call to IndexedDBQuotaClient.GetUsage() via the javascript quota API, which checks the size of the leveldb directory on disk, and future reported deltas would be counted against that. If that's not accurate, the worst case is that the application would think it has run out of quota before it actually does, but would get an accurate count when the browser is restarted. > I think it would be good to think thru some other approaches to measuring how > much. You're right, we need to get an approximate number of bytes of file system space used from leveldb, which it mostly supports. I fell back to the current IPC-sniffing chrome-side-only implementation as the deadline for m14 drew near :( http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_quota_client.cc:227: // safely delete them here. On 2011/07/27 01:31:35, michaeln wrote: > On 2011/07/26 17:46:15, dgrogan wrote: > > On 2011/07/26 06:35:28, kinuko wrote: > > > On 2011/07/26 02:02:16, dgrogan wrote: > > > > Kinuko, can you confirm that this is true? > > > > > > It's half true-- it won't be called while it is in use for eviction, but > after > > > we make a change like this BrowsingDataDeleter will also be calling this > one, > > > which might be kicked by UI action. > > > http://codereview.chromium.org/7129018/ > > > > > > ...or the manager could avoid calling this whenever the origin is in use. > > Maybe > > > we should do that? > > > > That would be great for IDB. > > I think we want to support deleting resources while they are in use. If a user > gestures to delete all in the browsing data remover ui, we don't want to put up > an alert that says "not all data could be deleted, close some tabs and try > again". Sorry, I only meant it would be great for IDB because we don't currently support delete-in-use and we would have to scramble if we had to support it in the next week or two. So I guess we need to figure out if and when we want FileAPI and IndexedDB to handle delete-in-use? > What happens in the appcache and sqldb case is that pages using the deleted > resources get errors when trying to subsquently access the deleted resources. > I'm not sure what happens in the case of the filesystem?
http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_callbacks.h (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_callbacks.h:84: GURL origin_url() const { return origin_url_; } On 2011/07/27 01:31:35, michaeln wrote: > can this be a const GURL& return value? Done. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_context.cc:120: void IndexedDBContext::EvictOrigin(const string16& origin_id) { On 2011/07/27 01:31:35, michaeln wrote: > This and the method above are functionally equivalent, the only difference is > one has a DCHECK for 'chrome-extension' that the other doesn't? Correct. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:204: // TODO(jorlow): This doesn't support file:/// urls properly. We probably need On 2011/07/27 01:31:35, michaeln wrote: > see DatabaseUtil::GetOriginFromIdentifier Thanks. I'll fix it in another CL. http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:775: // TODO(dgrogan): Figure out how accurate this is in terms of disk space used. On 2011/07/27 22:56:01, dgrogan wrote: > On 2011/07/27 01:31:35, michaeln wrote: > > Eavesdropping on OnPut/OnDelete/OnClear/OnIndex/OnDeleteIndex look like a > > reasonable way to determine if something has changed in the transaction, but > i'm > > very skeptical of this technique to determine by how much its change by... > > presumably puts can overwrite existing values? > My naive theory is that an application can trigger a call to > IndexedDBQuotaClient.GetUsage() via the javascript quota API, which checks the > size of the leveldb directory on disk, and future reported deltas would be > counted against that. So, I realized that's not how the quota manager works, it just queries indexeddb for size on disk at browser startup. > If that's not accurate, the worst case is that the > application would think it has run out of quota before it actually does, but > would get an accurate count when the browser is restarted. This seems to be the situation. I think it's my best option for m14. WDYT? http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... File content/browser/in_process_webkit/indexed_db_dispatcher_host.h (right): http://codereview.chromium.org/7470008/diff/2001/content/browser/in_process_w... content/browser/in_process_webkit/indexed_db_dispatcher_host.h:48: void TransactionComplete(int32 transaction_id); On 2011/07/27 01:31:35, michaeln wrote: > Would it make sense to make this method private? I guess that's not in keeping > with how its used by IndexedDBTransactionCallbacks. Right, IndexedDBTransactionCallbacks needs to call it.
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_callbacks.cc (left): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_callbacks.cc:32: dispatcher_host()->Context()->quota_manager_proxy()->NotifyStorageAccessed( This stuff was moved to IndexedDBContext, in a function called NewConnection or similar. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:155: int64 disk_usage = ReadUsageFromDisk(origin_url); 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:193: // We haven't heard back from the QuotaManager yet, just let it through. This happened a few times in the existing browser tests but I couldn't repro it when actually running chrome. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:786: WebIDBObjectIDToURLMap* transaction_url_map = Getting a handle to this variable is just for readability and to make 80-char linebreaks easier.
Michael, Kinuko, please take another look.
thnx for revamping this! http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:155: int64 disk_usage = ReadUsageFromDisk(origin_url); On 2011/08/03 01:41:29, dgrogan wrote: > 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. Seems reasonable, although if you do have a cached value, why not use it instead of the more expensive computation? > From what I've seen GetOriginUsage is called before anything else primes the > cache (origin_size_map_) anyway. 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:185: connection_count_[origin_url] ++; 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:196: bool over_quota = additional_bytes + origin_size_map_[origin_url] > 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; http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:202: return WouldBeOverQuota(origin_url, 0 /*additional_bytes*/); return space_available <= 0; http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:227: origin_allocated_quota_map_[origin_url] = quota; do u really want space_available_map_[origin_url] = quota - usage; http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:230: void GotUpdatedQuota(IndexedDBContext* context, const GURL& origin_url, this should be static or in an anon namespace if its really needed http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:253: NewRunnableFunction(&GotUpdatedQuota, would a runnable object method work here inplace of the intermediary function? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:270: void RequestQuota( ditto comment about static or anon namespace http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:289: NewRunnableFunction(&RequestQuota, ditto question about runnable object method http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:99: On a child process crash the call sequence will not go thru the graceful OnClosed/OnDestroyed code paths. If there are context->NewConnection() calls that have not yet been balanced by a context->ConnectionClosed() call, IndexedDBContext's bookeeping will get confused. The connection_count_ for those origins will never drop to zero. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:214: uint64 quota = kDefaultQuota; Is this old quota code needed anymore? Is the value of this param used by GetIDBFactory()->open(... quota) anymore? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:265: transaction_dispatcher_host_->transaction_url_map_[transaction_id]); where are transaction_url_map_ entries erased? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:266: transaction_dispatcher_host_->transaction_size_map_.erase(transaction_id); maybe let TransactionDispatcherHost::OnDestroyed take care of erasing from both the transaction_url and transaction_size map in all cases http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:385: if (parent_->Context()->IsOverQuota( 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:448: DCHECK(!transaction != !*ec); unrelated: curious DCHECK :) http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:467: parent_->Context()->ConnectionClosed(origin_url); 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:788: if (parent_->Context()->IsOverQuota( 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1057: TransactionDispatcherHost::OnDidCompleteTaskEvents(int transaction_id) { I'm not familiar with these methods or the sequence of events. When does this get called in the act of a transaction? One-time at the end or multiple times as progress is made? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1067: idb_transaction->abort(); 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? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1075: parent_->DestroyObject(&map_, object_id); would this be a good place to erase from the transaction_size_map_ so entries are cleared on the onAbort() and onTimeout() cases too? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:20: void IndexedDBTransactionCallbacks::onAbort() { It's also 'complete' in this onAbort case, just not successfully. How does the transaction_size_ map get cleaned up in this case and in onTimeout? http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_quota_client.cc:51: scoped_ptr<DeletionCallback> callback_; It would be good to ensure that this callback object got deleted on the IO thread. Calling .reset() in Completed() and in an Aborted() OVERRIDE should take care of that. The callback object may hold references/ptrs to non-thread safe objects that need to be released/deleted on the IO thread.
Have added some minor comments; for quota handling as michael commented you'll
want to keep available space ('quota - usage') instead of the quota, since usage
is globally calculated across different storage APIs.
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (right):
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:155: int64 disk_usage =
ReadUsageFromDisk(origin_url);
On 2011/08/03 03:40:40, michaeln wrote:
> On 2011/08/03 01:41:29, dgrogan wrote:
> > 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.
>
> Seems reasonable, although if you do have a cached value, why not use it
instead
> of the more expensive computation?
>
> > From what I've seen GetOriginUsage is called before anything else primes the
> > cache (origin_size_map_) anyway.
>
> 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.
>
(fwiw: I'm planning to make QM purge its cache periodically.)
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:172:
connection_count_[origin_url] --;
nit: extra space before -- (here and at line 185)
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right):
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:385: if
(parent_->Context()->IsOverQuota(
On 2011/08/03 03:40:40, michaeln wrote:
> 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.
QUOTA_EXCEEDED_ERR (22)?
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.cc (right):
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:289:
NewRunnableFunction(&RequestQuota,
Could this be implemented using NewRunnableMethod? I might not fully understand
why we need a separate function.
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.cc:290:
scoped_refptr<quota::QuotaManagerProxy>(
I guess you could use make_scoped_refptr() ?
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context.h (right):
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context.h:15: #include <map>
nit: this must be above other chromium headers
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc
(right):
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc:98: const
GURL& origin_url) {
weird indent
http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc:101:
&IndexedDBQuotaClientTest::OnDeleteOriginComplete));
weird indent
I didn't address everything yet. The stuff remaining is stuff that I'm not sure about and need to understand better before responding. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:172: connection_count_[origin_url] --; On 2011/08/03 09:12:22, kinuko wrote: > nit: extra space before -- (here and at line 185) Done. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:196: bool over_quota = additional_bytes + origin_size_map_[origin_url] > On 2011/08/03 03:40:40, michaeln wrote: > Is this comparing apples to oranges? No. I'm glad you noticed this. > 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; http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:202: return WouldBeOverQuota(origin_url, 0 /*additional_bytes*/); On 2011/08/03 03:40:40, michaeln wrote: > return space_available <= 0; I did space_available < 1 http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:227: origin_allocated_quota_map_[origin_url] = quota; On 2011/08/03 03:40:40, michaeln wrote: > do u really want space_available_map_[origin_url] = quota - usage; Yes I do. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:230: void GotUpdatedQuota(IndexedDBContext* context, const GURL& origin_url, On 2011/08/03 03:40:40, michaeln wrote: > this should be static or in an anon namespace if its really needed deleted http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:253: NewRunnableFunction(&GotUpdatedQuota, On 2011/08/03 03:40:40, michaeln wrote: > would a runnable object method work here inplace of the intermediary function? Done. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:270: void RequestQuota( On 2011/08/03 03:40:40, michaeln wrote: > ditto comment about static or anon namespace Now in an anon namespace. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:274: if (!proxy->quota_manager()) { If I switched to NewRunnableMethod, I'm worried about this check being deleted. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:289: NewRunnableFunction(&RequestQuota, On 2011/08/03 03:40:40, michaeln wrote: > ditto question about runnable object method 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:385: if (parent_->Context()->IsOverQuota( On 2011/08/03 09:12:22, kinuko wrote: > On 2011/08/03 03:40:40, michaeln wrote: > > 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. > > QUOTA_EXCEEDED_ERR (22)? Same comment as below about what's in the spec. In short: I'm not sure. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:788: if (parent_->Context()->IsOverQuota( On 2011/08/03 03:40:40, michaeln wrote: > 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. 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1067: idb_transaction->abort(); On 2011/08/03 03:40:40, michaeln wrote: > 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? 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:20: void IndexedDBTransactionCallbacks::onAbort() { On 2011/08/03 03:40:40, michaeln wrote: > It's also 'complete' in this onAbort case, just not successfully. How does the > transaction_size_ map get cleaned up in this case and in onTimeout? This is handled in patch 6. OnTimeout is deprecated, the timeout stuff was, or at least should have been, removed from the spec for anything asynchronous. http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:289: NewRunnableFunction(&RequestQuota, On 2011/08/03 09:12:22, kinuko wrote: > Could this be implemented using NewRunnableMethod? I might not fully understand > why we need a separate function. Either using NewRunnableMethod won't really work here, or I don't know how to make it work. (What's your emoticon for sheepish?) See the response to Michael's similar comment. http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:290: scoped_refptr<quota::QuotaManagerProxy>( On 2011/08/03 09:12:22, kinuko wrote: > I guess you could use make_scoped_refptr() ? Ah, thanks. Didn't know about that. http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.h (right): http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:15: #include <map> On 2011/08/03 09:12:22, kinuko wrote: > nit: this must be above other chromium headers Done. http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_quota_client.cc:51: scoped_ptr<DeletionCallback> callback_; On 2011/08/03 03:40:40, michaeln wrote: > It would be good to ensure that this callback object got deleted on the IO > thread. Calling .reset() in Completed() and in an Aborted() OVERRIDE should take > care of that. The callback object may hold references/ptrs to non-thread safe > objects that need to be released/deleted on the IO thread. Done. http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc (right): http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc:98: const GURL& origin_url) { On 2011/08/03 09:12:22, kinuko wrote: > weird indent Done. http://codereview.chromium.org/7470008/diff/18001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_quota_client_unittest.cc:101: &IndexedDBQuotaClientTest::OnDeleteOriginComplete)); On 2011/08/03 09:12:22, kinuko wrote: > weird indent Done.
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1067: idb_transaction->abort(); > 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. Probably worthy of a TODO? We definitely want to indicate to the caller QUOTA_ERROR in some way. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:20: void IndexedDBTransactionCallbacks::onAbort() { > This is handled in patch 6. OnTimeout is deprecated, the timeout stuff was, or > at least should have been, removed from the spec for anything asynchronous. Is onTimeout() called by in the current impl? If not, can you put a NOT_REACHED() in there? http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:106: void IndexedDBContext::DeleteIndexedDBFile(const FilePath& file_path) { I don't see any external callers of this method, it would be good to make it private (or even remove it and replace the callsites with a call to file_util::Delete()). The reason being the origin info is buried in the file_path and to report storage modifications we'd have to crack that path and pull it out... but thats just busy work... would be better if external callers expressed what they would like deleted in terms of 'origin' instead of 'file_paths'. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:129: } I think a call to quota->NotifyStorageModified(origin, -size) should be here and in the similar DeleteIndexdDBForOrigin method. It's unfortunate to be adding a new method so similar to the existing DeleteIndexedDbForOrigin method. Consider merging them now to avoid the code duplication. The only callsite for DeleteIndexedDBForOrigin() is in extension_data_delete.cc. I think that caller can just as easily pass in the 'origin_url' as it can the 'origin_id'... the extension_url == origin_url. The Delete verb is more consistent with the corresponding stuff in other storage silos than the Evict verb. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:196: return over_quota; nit: could return the comparison result w/o the local var http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:201: return WouldBeOverQuota(origin_url, 1 /*additional_bytes*/); nit: instead of comments, consider self-documenting code const int kOneAdditionalByte = 1; return WouldBeOverQuota(origin_url, kOneAdditionalByte); http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:229: class IndexedDBGetUsageAndQuotaCallback : can this be in the anon namespace too, or maybe we can simplify this away... see comment below http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:266: void RequestQuota( I think some of this can be simplified by posting runnable method tasks on the IndexedDBContext object, boiling it down to two methods and no new classes. void IndexedDBContext::QueryAvailableQuota(origin_url) { if (!IO_THREAD) { DCHECK(WEBKIT_THREAD); if (quota_manager_proxy()) PostTask(IO_THREAD, NewRunnableMethod(this, QueryAvailableQuota, origin_url)); return; } if (!quota_manager_proxy()->quota_manager()) return; AddRef(); // balanced in the OnQuotaCallback proxy->quota_manager()->GetUsageAndQuota( origin_url, quota::kStorageTypeTemporary, NewCallback(this, &IndexedDBContext::OnQuotaCallback)); } void IndexedDBContext::OnQuotaCallback(origin, usage, quota) { if (!WEBKIT_THREAD) { DCHECK(IO_THREAD); PostTask(WEBKIT_THREAD, NewRunnableMethod(this, OnQuotaCallback, origin, usage, quota)); Release(); // balanced in QueryAvailableQuota return; } space_available_map_[origin_url] = quota - usage; } http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.h (right): http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:75: void NewConnection(const GURL& origin_url); nit: maybe ConnectionOpened() for symmetry with ConnectionClosed() http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:83: void QueryDiskAndUpdateQuotaUsage(const GURL& origin_url); can more of these new methods be made private? http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:114: void QueryAvailableQuota(const GURL& origin_url); style: define members within a section types methods data members
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:214: uint64 quota = kDefaultQuota; On 2011/08/03 03:40:40, michaeln wrote: > Is this old quota code needed anymore? Is the value of this param used by > GetIDBFactory()->open(... quota) anymore? Not by default, only if --indexeddb-use-sqlite is specified. In that case the user knows what they're doing and quota can probably be left out. So, this should be removed, but maybe in a future patch. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1067: idb_transaction->abort(); On 2011/08/04 00:23:27, michaeln wrote: > > 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. > > Probably worthy of a TODO? We definitely want to indicate to the caller > QUOTA_ERROR in some way. Done. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:106: void IndexedDBContext::DeleteIndexedDBFile(const FilePath& file_path) { On 2011/08/04 00:23:27, michaeln wrote: > I don't see any external callers of this method, it would be good to make it > private (or even remove it and replace the callsites with a call to > file_util::Delete()). The reason being the origin info is buried in the > file_path and to report storage modifications we'd have to crack that path and > pull it out... but thats just busy work... would be better if external callers > expressed what they would like deleted in terms of 'origin' instead of > 'file_paths'. Done. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:129: } On 2011/08/04 00:23:27, michaeln wrote: > I think a call to quota->NotifyStorageModified(origin, -size) should be here and > in the similar DeleteIndexdDBForOrigin method. Good idea. > It's unfortunate to be adding a new method so similar to the existing > DeleteIndexedDbForOrigin method. Consider merging them now to avoid the code > duplication. The only callsite for DeleteIndexedDBForOrigin() is in > extension_data_delete.cc. I think that caller can just as easily pass in the > 'origin_url' as it can the 'origin_id'... the extension_url == origin_url. > > The Delete verb is more consistent with the corresponding stuff in other storage > silos than the Evict verb. Merged and renamed. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:201: return WouldBeOverQuota(origin_url, 1 /*additional_bytes*/); On 2011/08/04 00:23:27, michaeln wrote: > nit: instead of comments, consider self-documenting code > const int kOneAdditionalByte = 1; > return WouldBeOverQuota(origin_url, kOneAdditionalByte); Done. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:229: class IndexedDBGetUsageAndQuotaCallback : On 2011/08/04 00:23:27, michaeln wrote: > can this be in the anon namespace too, or maybe we can simplify this away... see > comment below I made it a nested class. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:266: void RequestQuota( On 2011/08/04 00:23:27, michaeln wrote: > I think some of this can be simplified by posting runnable method tasks on the > IndexedDBContext object, boiling it down to two methods and no new classes. I got rid of this RequestQuota method by following your example. > void IndexedDBContext::QueryAvailableQuota(origin_url) { > if (!IO_THREAD) { > DCHECK(WEBKIT_THREAD); > if (quota_manager_proxy()) > PostTask(IO_THREAD, > NewRunnableMethod(this, > QueryAvailableQuota, > origin_url)); > return; > } > if (!quota_manager_proxy()->quota_manager()) > return; > AddRef(); // balanced in the OnQuotaCallback > proxy->quota_manager()->GetUsageAndQuota( > origin_url, quota::kStorageTypeTemporary, > NewCallback(this, &IndexedDBContext::OnQuotaCallback)); > } > > void IndexedDBContext::OnQuotaCallback(origin, usage, quota) { But I ran into a problem here: origin isn't one of the arguments. The quota manager doesn't tell the callback which origin it's talking about. So I left the class so that it can store the origin. > if (!WEBKIT_THREAD) { > DCHECK(IO_THREAD); > PostTask(WEBKIT_THREAD, > NewRunnableMethod(this, > OnQuotaCallback, > origin, usage, quota)); > Release(); // balanced in QueryAvailableQuota > return; > } > space_available_map_[origin_url] = quota - usage; > } http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.h (right): http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:75: void NewConnection(const GURL& origin_url); On 2011/08/04 00:23:27, michaeln wrote: > nit: maybe ConnectionOpened() for symmetry with ConnectionClosed() Done. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:83: void QueryDiskAndUpdateQuotaUsage(const GURL& origin_url); On 2011/08/04 00:23:27, michaeln wrote: > can more of these new methods be made private? This one can. http://codereview.chromium.org/7470008/diff/28001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.h:114: void QueryAvailableQuota(const GURL& origin_url); On 2011/08/04 00:23:27, michaeln wrote: > style: define members within a section > types > methods > data members Done.
http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:108: void IndexedDBContext::DeleteIndexedDBForOrigin(const GURL& origin_url) { Thnx for consolidating the various Delete methods! http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:116: QueryDiskAndUpdateQuotaUsage(origin_url); The QueryDiskAndUpdateQuotaUsage method relies on origin_size_map_ having been populated for this origin, but even if the map is seeded in ConnectionOpened, there is no guarantee of that having happened. Consider an extension being deleted prior to any storage activity having occurred. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:154: void IndexedDBContext::ConnectionClosed(const GURL& origin_url) { please put the methods in the .cc file in the same order as the .h file. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:174: QueryAvailableQuota(origin_url); what about guaranteeing that the size_map for the origin is seeded at this point? http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:246: const Tuple3<quota::QuotaStatusCode, int64, int64>& params) { doh... i see origin_url is not available here http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:267: AddRef(); // Balanced in IndexedDBGetUsageAndQuotaCallback::Run. AddRef (and balancing Release) is not needed since the custome callback class holds a reference to 'this'. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:476: parent_->Context()->ConnectionClosed(origin_url); i'm still wondering about the OnClose vs OnDestroyed question from yesterday, seems like the latter may be more robust also there's the somewhat related issue with calling ConnectionClosed in the child process crashed case http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_quota_client.cc:70: usage_ = indexed_db_context_->InitializeDiskUsage(origin_url_); Its a little odd to see this function calling an 'initialize' method. Feels like GetOriginDiskUsage() would be better. Currently, sometime around the first storage access system wide, the QM will query each client for each origin that is using storage of some form via the GetOriginUsage stuff. Origins that are not yet using storage will not be queried until sometime shortly after it's first access, like opening an indexedDB. The trigger for this query is a call to QM->GetUsageAndQuota(origin). I don't think you can depend on these queries arriving in advance of indexedDB operations. Consider if the first call to QM->GetUsageAndQuota for a particular origin originates from the indexedDB system.
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:155: int64 disk_usage = ReadUsageFromDisk(origin_url); On 2011/08/03 03:40:40, michaeln wrote: > Seems reasonable, although if you do have a cached value, why not use it instead > of the more expensive computation? 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? > > From what I've seen GetOriginUsage is called before anything else primes the > > cache (origin_size_map_) anyway. > > 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:185: connection_count_[origin_url] ++; On 2011/08/03 03:40:40, michaeln wrote: > 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. Done. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:467: parent_->Context()->ConnectionClosed(origin_url); On 2011/08/03 03:40:40, michaeln wrote: > 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? Yes it would. Moved. > 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1057: TransactionDispatcherHost::OnDidCompleteTaskEvents(int transaction_id) { On 2011/08/03 03:40:40, michaeln wrote: > I'm not familiar with these methods or the sequence of events. When does this > get called in the act of a transaction? One-time at the end or multiple times as > progress is made? This is called after the success event of each operation in a transaction is dispatched to JS. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:20: void IndexedDBTransactionCallbacks::onAbort() { On 2011/08/04 00:23:27, michaeln wrote: > > This is handled in patch 6. OnTimeout is deprecated, the timeout stuff was, > or > > at least should have been, removed from the spec for anything asynchronous. > > Is onTimeout() called by in the current impl? If not, can you put a > NOT_REACHED() in there? Done. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:116: QueryDiskAndUpdateQuotaUsage(origin_url); On 2011/08/04 22:11:55, michaeln wrote: > The QueryDiskAndUpdateQuotaUsage method relies on origin_size_map_ having been > populated for this origin, but even if the map is seeded in ConnectionOpened, > there is no guarantee of that having happened. > > Consider an extension being deleted prior to any storage activity having > occurred. Good point. I added a EnsureDiskUsageCacheInitialized call to QueryDiskAndUpdateQuotaUsage. This would still cause a delta to be sent to the QM, before it issued a GetOriginUsage, but I think that should be ok. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:154: void IndexedDBContext::ConnectionClosed(const GURL& origin_url) { On 2011/08/04 22:11:55, michaeln wrote: > please put the methods in the .cc file in the same order as the .h file. Sorry. Reordered. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:174: QueryAvailableQuota(origin_url); On 2011/08/04 22:11:55, michaeln wrote: > what about guaranteeing that the size_map for the origin is seeded at this > point? Sorry, hadn't gotten there yet, but it should be there now. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:267: AddRef(); // Balanced in IndexedDBGetUsageAndQuotaCallback::Run. On 2011/08/04 22:11:55, michaeln wrote: > AddRef (and balancing Release) is not needed since the custome callback class > holds a reference to 'this'. Ah, right. Removed. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:476: parent_->Context()->ConnectionClosed(origin_url); On 2011/08/04 22:11:55, michaeln wrote: > i'm still wondering about the OnClose vs OnDestroyed question from yesterday, > seems like the latter may be more robust Agreed, switched to OnDestroyed. > also there's the somewhat related issue with calling ConnectionClosed in the > child process crashed case Right. If the child process crashes: 1 IndexedDBContext will not let that host be deleted 2 deltas from compactions that are ongoing after a transaction finishes could be missed 3 Their entries in these url and size maps will remain None of these particularly worry me. 1 should be taken care of when we can delete leveldb files out from underneath webkit. 2 could cause the QM to overestimate that origin's usage. 3 would cause memory leaks, but it's hard to imagine a significant amount. Though, I don't know how to eventually handle these. Is there some message we can listen for that indicates a renderer process crashed? If so maybe we could use that to help clean up? http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_quota_client.cc (right): http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_quota_client.cc:70: usage_ = indexed_db_context_->InitializeDiskUsage(origin_url_); On 2011/08/04 22:11:55, michaeln wrote: > Its a little odd to see this function calling an 'initialize' method. Feels like > GetOriginDiskUsage() would be better. You're right. Changed. > Currently, sometime around the first storage access system wide, the QM will > query each client for each origin that is using storage of some form via the > GetOriginUsage stuff. Origins that are not yet using storage will not be queried > until sometime shortly after it's first access, like opening an indexedDB. The > trigger for this query is a call to QM->GetUsageAndQuota(origin). > > I don't think you can depend on these queries arriving in advance of indexedDB > operations. Consider if the first call to QM->GetUsageAndQuota for a particular > origin originates from the indexedDB system. This patch adds a EnsureDiskUsageCacheInitialized method that hopefully handles this. http://codereview.chromium.org/7470008/diff/31003/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/31003/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:62: class IndexedDBContext::IndexedDBGetUsageAndQuotaCallback : I recall seeing something that said nested class implementations should come before the outer class's. Maybe that was just for webkit? http://codereview.chromium.org/7470008/diff/31003/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:283: int64 IndexedDBContext::ResetDiskUsageCache(const GURL& origin_url) { The guts of the old "InitializeDiskUsage" are here.
sooo clooose... can you run some try jobs? http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:467: parent_->Context()->ConnectionClosed(origin_url); On 2011/08/04 23:44:55, dgrogan wrote: > On 2011/08/03 03:40:40, michaeln wrote: > > 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? > > Yes it would. Moved. 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. http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:1057: TransactionDispatcherHost::OnDidCompleteTaskEvents(int transaction_id) { > This is called after the success event of each operation in a transaction is > dispatched to JS. Thnx for the explanation. Sounds like it makes for a great place to monitor size accumulation! http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:476: ////////////////////////////////////////////////////////////////////// > On 2011/08/04 23:44:56, dgrogan wrote: > Though, I don't know how to eventually handle these. Is there some message we > can listen for that indicates a renderer process crashed? If so maybe we could > use that to help clean up? I didn't realize that part of the puzzle was missing :) It's real easy. See ResetDispatcherHosts(), thats your signal that the renderer is gone (crashed or not). Notice that it calls database_dispatcher_host_.reset(), that has delete semantics (hooray!). IndexedDBDispatcherHost::DatabaseDispatcherHost::~DatabaseDispatcherHost() { foreach (origin in database_url_map_) parent_->Context()->ConnectionClosed(origin); } note: that logic assumes that u also relocate the usual call to ConnectionClose() to the OnDestroyed method (the pair of database_url_map_and connection cleanup has to stay together) http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:149: connection_count_.find(origin_url) == connection_count_.end()) { don't u need a call to EnsureDiskUsageCacheInitialized prior to calling file_util::Delete, otherwise the notify method will not have the 'old' size available to it http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:178: return ResetDiskUsageCache(origin_url); given expected usage pattern (not frequently called), stat'ing files should be fine, but if that usage pattern changes, might be nice to utilize the cached value somewhere down the road http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:192: void IndexedDBContext::ConnectionClosed(const GURL& origin_url) { maybe DCHECK(connection_count_[origin] > 0); http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:205: void IndexedDBContext::TransactionComplete(const GURL& origin_url) { maybe DCHECK(connection_count_[origin] > 0); http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:241: EnsureDiskUsageCacheInitialized(origin_url); provided you ensure the cache is initted prior to calling file_util::Delete in DeleteIndexedDBForOrigin (in addition to the ConnectionOpened callsite), doesn't look like you need this Ensure call here... might replace it with a DCHECK for the presence of a cached value
http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_dispatcher_host.cc (right): http://codereview.chromium.org/7470008/diff/11011/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:467: parent_->Context()->ConnectionClosed(origin_url); On 2011/08/05 01:21:47, michaeln wrote: > 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. 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. http://codereview.chromium.org/7470008/diff/31001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_dispatcher_host.cc:476: ////////////////////////////////////////////////////////////////////// On 2011/08/05 01:21:47, michaeln wrote: > See ResetDispatcherHosts(), thats your signal that the renderer is gone (crashed > or not). Notice that it calls database_dispatcher_host_.reset(), that has delete > semantics (hooray!). Oh, excellent, thanks. > IndexedDBDispatcherHost::DatabaseDispatcherHost::~DatabaseDispatcherHost() { > foreach (origin in database_url_map_) > parent_->Context()->ConnectionClosed(origin); > } This seems to imply that there is one IndexedDBDispatcherHost instance per renderer process, is that correct? > note: that logic assumes that u also relocate the usual call to > ConnectionClose() to the OnDestroyed method (the pair of database_url_map_and > connection cleanup has to stay together) I don't understand. What would (or wouldn't, whichever the case may be) happen if the ConnectionClosed call didn't move? http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_context.cc (right): http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:149: connection_count_.find(origin_url) == connection_count_.end()) { On 2011/08/05 01:21:47, michaeln wrote: > don't u need a call to EnsureDiskUsageCacheInitialized prior to calling > file_util::Delete, otherwise the notify method will not have the 'old' size > available to it Done. http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:178: return ResetDiskUsageCache(origin_url); On 2011/08/05 01:21:47, michaeln wrote: > given expected usage pattern (not frequently called), stat'ing files should be > fine, but if that usage pattern changes, might be nice to utilize the cached > value somewhere down the road Agreed. http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:192: void IndexedDBContext::ConnectionClosed(const GURL& origin_url) { On 2011/08/05 01:21:47, michaeln wrote: > maybe DCHECK(connection_count_[origin] > 0); Done. http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:205: void IndexedDBContext::TransactionComplete(const GURL& origin_url) { On 2011/08/05 01:21:47, michaeln wrote: > maybe DCHECK(connection_count_[origin] > 0); Done. http://codereview.chromium.org/7470008/diff/23005/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_context.cc:241: EnsureDiskUsageCacheInitialized(origin_url); On 2011/08/05 01:21:47, michaeln wrote: > provided you ensure the cache is initted prior to calling file_util::Delete in > DeleteIndexedDBForOrigin (in addition to the ConnectionOpened callsite), doesn't > look like you need this Ensure call here... might replace it with a DCHECK for > the presence of a cached value Good point. It's a no-op with a performance hit here.
lgtm > 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. Presuming there are no existing leask, OnDestroyed looks more robust to me because that is where the existing structures are deleted. Suppose OnDestoyed is called w/o OnClosed having been called (nothing indicates to me that OnClosed is guaranteed to have been called prior to OnDestroyed), if the cleanup were in OnClosed it could get skipped. > This seems to imply that there is one IndexedDBDispatcherHost instance per > renderer process, is that correct? Correct. > > note: that logic assumes that u also relocate the usual call to > > ConnectionClose() to the OnDestroyed method (the pair of database_url_map_and > > connection cleanup has to stay together) > > I don't understand. What would (or wouldn't, whichever the case may be) happen > if the ConnectionClosed call didn't move? If those two lines of cleanup were not co-located in the same place, the presence of the map entry at dtor time would not be indicative of whether or not the connection count had been decremented, so there would be no way to tell if you should or should not invoke ConnectionClosed() at dtor time. With those two cleanup steps tied at the hip, you can tell, and entry in the map means the 'closed' method has not yet been called. http://codereview.chromium.org/7470008/diff/36001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/36001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:32: NOTREACHED(); is this truely not reached? if so the following line of code can be deleted, if not then you can remove the NOTREACHED ... and if this is truely not reached, then there's a small pile of stuff (webkit virtual methods and impls, ipc message defs, ipc message handlers) that can be removed from the project. It would be good to remove that dead-code (in a later CL). The act of doing so could have a side benefit of showing you more about how the chrome side of this thing fits together.
On 2011/08/05 17:15:36, michaeln wrote: > lgtm > > > 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. > > Presuming there are no existing leask, I guess this is where my uncertainty comes in. We know we have leaks in IndexedDB but don't know where. > If those two lines of cleanup were not co-located in the same place, the > presence of the map entry at dtor time would not be indicative of whether or not > the connection count had been decremented, so there would be no way to tell if > you should or should not invoke ConnectionClosed() at dtor time. With those two > cleanup steps tied at the hip, you can tell, and entry in the map means the > 'closed' method has not yet been called. Ah, of course, thanks for the explanation. > http://codereview.chromium.org/7470008/diff/36001/content/browser/in_process_... > File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc > (right): > > http://codereview.chromium.org/7470008/diff/36001/content/browser/in_process_... > content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:32: > NOTREACHED(); > is this truely not reached? I'm not 100.0% sure it is not reached, so I suppose I should remove the NOTREACHED for this CL and remove all the onTimeout stuff in a different CL.
http://codereview.chromium.org/7470008/diff/36001/content/browser/in_process_... File content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc (right): http://codereview.chromium.org/7470008/diff/36001/content/browser/in_process_... content/browser/in_process_webkit/indexed_db_transaction_callbacks.cc:32: NOTREACHED(); On 2011/08/05 17:15:37, michaeln wrote: > is this truely not reached? if so the following line of code can be deleted, if > not then you can remove the NOTREACHED Removed.
Change committed as 95691 |
