Bug 100425 - IndexedDB: add methods to support id-based backend APIs
Summary: IndexedDB: add methods to support id-based backend APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords:
Depends on:
Blocks: 100426
  Show dependency treegraph
 
Reported: 2012-10-25 15:17 PDT by Alec Flett
Modified: 2012-11-01 11:07 PDT (History)
11 users (show)

See Also:


Attachments
Patch (95.88 KB, patch)
2012-10-25 16:05 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (102.40 KB, patch)
2012-10-26 14:34 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (111.08 KB, patch)
2012-10-29 17:09 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (111.08 KB, patch)
2012-10-29 17:32 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (111.08 KB, patch)
2012-10-30 10:16 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (111.02 KB, patch)
2012-10-30 17:09 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (110.98 KB, patch)
2012-10-31 11:17 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (111.08 KB, patch)
2012-11-01 10:39 PDT, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-10-25 15:17:03 PDT
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.
Comment 1 Alec Flett 2012-10-25 15:19:03 PDT
the bug for the followup work is bug 100426
Comment 2 Alec Flett 2012-10-25 16:05:56 PDT
Created attachment 170759 [details]
Patch
Comment 3 WebKit Review Bot 2012-10-25 16:08:25 PDT
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 4 Joshua Bell 2012-10-25 17:25:56 PDT
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 5 Joshua Bell 2012-10-26 10:16:20 PDT
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.
Comment 6 Alec Flett 2012-10-26 14:34:39 PDT
Created attachment 171013 [details]
Patch

Address review comments
Comment 7 Alec Flett 2012-10-26 14:35:04 PDT
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
Comment 8 Alec Flett 2012-10-29 17:09:42 PDT
Created attachment 171339 [details]
Patch
Comment 9 Alec Flett 2012-10-29 17:32:09 PDT
Created attachment 171346 [details]
Patch
Comment 10 Peter Beverloo (cr-android ews) 2012-10-29 18:43:47 PDT
Comment on attachment 171346 [details]
Patch

Attachment 171346 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14620637
Comment 11 Alec Flett 2012-10-30 10:16:57 PDT
Created attachment 171477 [details]
Patch

fix 'long long' typo
Comment 12 WebKit Review Bot 2012-10-30 11:15:11 PDT
Comment on attachment 171477 [details]
Patch

Attachment 171477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14620911
Comment 13 Alec Flett 2012-10-30 17:09:41 PDT
Created attachment 171554 [details]
Patch

unbreak chromium and android
Comment 14 Alec Flett 2012-10-30 17:10:43 PDT
ok, this one should be good to go - jsbell@ - anything else to add before I r?
Comment 15 Joshua Bell 2012-10-30 17:15:51 PDT
Comment on attachment 171554 [details]
Patch

still lgtm
Comment 16 Alec Flett 2012-10-31 10:06:09 PDT
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 17 Adam Barth 2012-10-31 10:37:12 PDT
Comment on attachment 171554 [details]
Patch

API change LGTM
Comment 18 Tony Chang 2012-10-31 10:44:26 PDT
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()?
Comment 19 Alec Flett 2012-10-31 10:54:51 PDT
(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.
Comment 20 Alec Flett 2012-10-31 11:17:14 PDT
Created attachment 171690 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-10-31 18:22:06 PDT
Comment on attachment 171690 [details]
Patch for landing

Clearing flags on attachment: 171690

Committed r133107: <http://trac.webkit.org/changeset/133107>
Comment 22 WebKit Review Bot 2012-10-31 18:22:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Stephen White 2012-10-31 19:08:52 PDT
Reverted r133107 for reason:

Broke compile on Chromium Win

Committed r133112: <http://trac.webkit.org/changeset/133112>
Comment 24 Alec Flett 2012-11-01 10:39:49 PDT
Created attachment 171891 [details]
Patch for landing
Comment 25 WebKit Review Bot 2012-11-01 11:07:54 PDT
Comment on attachment 171891 [details]
Patch for landing

Clearing flags on attachment: 171891

Committed r133195: <http://trac.webkit.org/changeset/133195>
Comment 26 WebKit Review Bot 2012-11-01 11:07:59 PDT
All reviewed patches have been landed.  Closing bug.