This is the first of a two-step landing. First, all IDB*BackendInterface methods that take String-based references to objectStores and indexes will get new methods along side the old ones, that take int64_t-based references instead. Then after cleaning up the other ports (namely, chromium) we'll fix up the frontend to proxy these new methods.
the bug for the followup work is bug 100426
Created attachment 170759 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 170759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170759&action=review First batch of comments. > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:198 > + for (IDBDatabaseMetadata::ObjectStoreMap::const_iterator it = m_metadata.objectStores.begin(); it != m_metadata.objectStores.end(); ++it) { Maybe rename containsObjectStore to findObjectStore that returns an ID or InvalidID, and then this can be a findObjectStore() followed by a remove() > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:243 > void IDBDatabaseBackendImpl::deleteObjectStoreInternal(ScriptExecutionContext*, PassRefPtr<IDBDatabaseBackendImpl> database, PassRefPtr<IDBObjectStoreBackendImpl> objectStore, PassRefPtr<IDBTransactionBackendImpl> transaction) This could change to take objectStoreId instead of objectStore. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:434 > +PassRefPtr<IDBTransactionBackendInterface> IDBDatabaseBackendImpl::transaction(unsigned short mode) Given that we're going to need the "scope" of the transaction for correctly scheduling, I don't think removing it here is a good idea, even though it's currently unused. Converting to a list of IDs would be okay. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:73 > + int64_t getObjectStoreId(const String& name); Going away, but this could be private right? > Source/WebCore/Modules/indexeddb/IDBMetadata.h:-41 > -struct IDBObjectStoreMetadata; Why reorder? > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:453 > ASSERT(!indexBackend != !ec); // If we didn't get an index, we should have gotten an exception code. And vice versa. Given above change, this can be ASSERT(!ec && indexBackend) right? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:287 > + // FIXME: We manually convert each name to an indexId, even if the name has already been deleted, because we have to remove exactly that many preemptive events, but this will go away when https://bugs.webkit.org/show_bug.cgi?id=100426 lands. Make sure to include a test with 100426 that tests put-record/delete-index/create-index-with-same-name-different-keypath > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:500 > + Extra blank line. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-524 > - ExceptionCode unused; I've been waiting for an excuse to remove that space! > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:47 > +PassRefPtr<IDBTransaction> IDBTransaction::create(ScriptExecutionContext* context, PassRefPtr<IDBTransactionBackendInterface> backend, PassRefPtr<DOMStringList> objectStoreNames, IDBTransaction::Mode mode, IDBDatabase* db) In order to clean up the binding code it would be nice to remove uses of DOMStringList where possible in favor of Vector<String>. Can we avoid adding new uses of DOMStringList and have callers use Vector<String> instead? (DOMStringList already has a cast operator for this.) > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:164 > ASSERT(!objectStoreBackend != !ec); // If we didn't get a store, we should have gotten an exception code. And vice versa. Given above, this can be ASSERT(!ec && objectStoreBackend) right? > Source/WebCore/Modules/indexeddb/IDBTransaction.h:148 > + RefPtr<DOMStringList> m_objectStoreNames; Per above, would be nice to avoid DOMStringList in favor of Vector<String> It seems like this can eventually go away in favor of Set<int64_t> of IDs in scope use the Map<String, int64_t> in the database metadata?
Comment on attachment 170759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170759&action=review Made it to the end - only a few nits. > Source/WebCore/ChangeLog:32 > + (getIndexId/getIndexIds/getObjectStoreId) that will go away with I personally find it helpful to annotate temporary code with FIXMEs so the redundancy is more obvious when reading it. Plus there's a warm fuzzy feeling when you get to delete it. > Source/WebKit/chromium/src/IDBObjectStoreBackendProxy.cpp:175 > void IDBObjectStoreBackendProxy::deleteIndex(const String& name, IDBTransactionBackendInterface* transaction, ExceptionCode& ec) Add blank line above. > Source/WebKit/chromium/src/IDBTransactionBackendProxy.cpp:64 > +PassRefPtr<WebCore::IDBObjectStoreBackendInterface> IDBTransactionBackendProxy::objectStore(int64_t indexId, ExceptionCode& ec) Add blank line above.
Created attachment 171013 [details] Patch Address review comments
Comment on attachment 170759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170759&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:243 >> void IDBDatabaseBackendImpl::deleteObjectStoreInternal(ScriptExecutionContext*, PassRefPtr<IDBDatabaseBackendImpl> database, PassRefPtr<IDBObjectStoreBackendImpl> objectStore, PassRefPtr<IDBTransactionBackendImpl> transaction) > > This could change to take objectStoreId instead of objectStore. In the interest of not expanding the scope of this any further, I'll catch that in the next patch! >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:434 >> +PassRefPtr<IDBTransactionBackendInterface> IDBDatabaseBackendImpl::transaction(unsigned short mode) > > Given that we're going to need the "scope" of the transaction for correctly scheduling, I don't think removing it here is a good idea, even though it's currently unused. Converting to a list of IDs would be okay. whoops, forgot about that. Put it back. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:73 >> + int64_t getObjectStoreId(const String& name); > > Going away, but this could be private right? nope, gets called by the transaction backend sometimes. >> Source/WebCore/Modules/indexeddb/IDBMetadata.h:-41 >> -struct IDBObjectStoreMetadata; > > Why reorder? Because the hashmaps were dependent on pre-declared structs - basically I couldn't implement the containsObjectStore/containsIndex without reordering >> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:47 >> +PassRefPtr<IDBTransaction> IDBTransaction::create(ScriptExecutionContext* context, PassRefPtr<IDBTransactionBackendInterface> backend, PassRefPtr<DOMStringList> objectStoreNames, IDBTransaction::Mode mode, IDBDatabase* db) > > In order to clean up the binding code it would be nice to remove uses of DOMStringList where possible in favor of Vector<String>. Can we avoid adding new uses of DOMStringList and have callers use Vector<String> instead? (DOMStringList already has a cast operator for this.) Good point, done. >> Source/WebCore/Modules/indexeddb/IDBTransaction.h:148 >> + RefPtr<DOMStringList> m_objectStoreNames; > > Per above, would be nice to avoid DOMStringList in favor of Vector<String> > > It seems like this can eventually go away in favor of Set<int64_t> of IDs in scope use the Map<String, int64_t> in the database metadata? yes. using Vector<String> for now
Created attachment 171339 [details] Patch
Created attachment 171346 [details] Patch
Comment on attachment 171346 [details] Patch Attachment 171346 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14620637
Created attachment 171477 [details] Patch fix 'long long' typo
Comment on attachment 171477 [details] Patch Attachment 171477 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14620911
Created attachment 171554 [details] Patch unbreak chromium and android
ok, this one should be good to go - jsbell@ - anything else to add before I r?
Comment on attachment 171554 [details] Patch still lgtm
abarth@ - ok, this is a different patch (unrelated to the DOMRequest stuff) - the key here on the chromium side is that we're just adding parallel methods to what already exists, substituting 'const String&' for 'int64_t' or 'long long' where appropriate, as well as dropping a few unused parameters while we're there. tony@ - do you mind reviewing the IDB side of things? it's just more refactoring/cleanup
Comment on attachment 171554 [details] Patch API change LGTM
Comment on attachment 171554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171554&action=review LGTM, just some style nits. > Source/WebCore/Modules/indexeddb/IDBMetadata.h:42 > + IDBIndexMetadata() { } Nit: Do you want this constructor to initialized the int64_ts and bools? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:203 > + for (size_t i = 0; i < indexIds.size(); i++) Nit: ++i > Source/WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:118 > + ASSERT(webIndexIds.size() == webIndexKeys.size()); > + Vector<int64_t> indexIds(webIndexIds.size()); > + Vector<IDBObjectStoreBackendInterface::IndexKeys> indexKeys(webIndexKeys.size()); > + > + for (size_t i = 0; i < webIndexIds.size(); ++i) { > + indexIds[i] = webIndexIds[i]; Can we make a helper function to share this code with put()?
(In reply to comment #18) > (From update of attachment 171554 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171554&action=review > > LGTM, just some style nits. > > > Source/WebCore/Modules/indexeddb/IDBMetadata.h:42 > > + IDBIndexMetadata() { } > > Nit: Do you want this constructor to initialized the int64_ts and bools? > Nah - the only reason for the constructor is because HashMap requires it - and if anything I've once or twice used the 'uninitializedness' of the values to detect bugs during development. > > Can we make a helper function to share this code with put()? Sure, I'll do that in the "cleanup" half of this overall refactoring - the one that removes the 'const String&' methods.
Created attachment 171690 [details] Patch for landing
Comment on attachment 171690 [details] Patch for landing Clearing flags on attachment: 171690 Committed r133107: <http://trac.webkit.org/changeset/133107>
All reviewed patches have been landed. Closing bug.
Reverted r133107 for reason: Broke compile on Chromium Win Committed r133112: <http://trac.webkit.org/changeset/133112>
Created attachment 171891 [details] Patch for landing
Comment on attachment 171891 [details] Patch for landing Clearing flags on attachment: 171891 Committed r133195: <http://trac.webkit.org/changeset/133195>