Bug 100425 – IndexedDB: add methods to support id-based backend APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alec Flett]

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.

[Alec Flett]

the bug for the followup work is bug 100426

[Alec Flett]

Created attachment 170759 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

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?

[Joshua Bell]

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.

[Alec Flett]

Created attachment 171013 [details]
Patch

Address review comments

[Alec Flett]

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

[Alec Flett]

Created attachment 171339 [details]
Patch

[Alec Flett]

Created attachment 171346 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 171346 [details]
Patch

Attachment 171346 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14620637

[Alec Flett]

Created attachment 171477 [details]
Patch

fix 'long long' typo

[WebKit Review Bot]

Comment on attachment 171477 [details]
Patch

Attachment 171477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14620911

[Alec Flett]

Created attachment 171554 [details]
Patch

unbreak chromium and android

[Alec Flett]

ok, this one should be good to go - jsbell@ - anything else to add before I r?

[Joshua Bell]

Comment on attachment 171554 [details]
Patch

still lgtm

[Alec Flett]

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

[Adam Barth]

Comment on attachment 171554 [details]
Patch

API change LGTM

[Tony Chang]

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()?

[Alec Flett]

(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.

[Alec Flett]

Created attachment 171690 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171690 [details]
Patch for landing

Clearing flags on attachment: 171690

Committed r133107: <http://trac.webkit.org/changeset/133107>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Reverted r133107 for reason:

Broke compile on Chromium Win

Committed r133112: <http://trac.webkit.org/changeset/133112>

[Alec Flett]

Created attachment 171891 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 171891 [details]
Patch for landing

Clearing flags on attachment: 171891

Committed r133195: <http://trac.webkit.org/changeset/133195>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.