Bug 106377 – IndexedDB: Allow createIndex/createObjectStore to be asynchronous

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alec Flett]

IndexedDB: Allow createIndex/createObjectStore to be asynchronous

[Alec Flett]

Created attachment 181784 [details]
Patch

[Alec Flett]

Comment on attachment 181784 [details]
Patch

oops, forgot one file

[Alec Flett]

Created attachment 181791 [details]
Patch

[Alec Flett]

jsbell@ / dgrogan@ - one more big review? Lots of code removal in this one :)

[Joshua Bell]

Comment on attachment 181791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181791&action=review

Overall LGTM...

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:640
> +            return false;

This changes the semantics of this method from "plow ahead in case of error" to "fail the load completely". I don't disagree with this change, but it's orthogonal to the rest of the patch, isn't it? (Also, I'd rather that we just implemented the FIXME if we're going to change the behavior.)

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1017
> +            return false;

Ditto.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:1037
>      return indexes;

Should be return true ?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:56
> +    const RefPtr<IDBBackingStore> m_backingStore;

Not for this patch (because it's consistent with other Operations), but given that we can get to the backing store via the transaction which is passed into perform(), we can probably eliminate this member from this and other Operations.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:1347
> +    m_metadata.objectStores.clear();

Is this ever called with non-empty m_metadata.objectStores?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:108
>      class VersionChangeOperation;

Remove this from IDBDatabaseBackendImpl class while the others are moving out too?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:111
>      class VersionChangeAbortOperation;

Ditto?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h:46
> +namespace IDBObjectStoreBackendImpl {

May want to add a FIXME indicating that this is temporary, and things in this namespace will move to ... ?

> Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:-50
> -TEST(IDBDatabaseBackendTest, BackingStoreRetention)

Any way to change this test rather than removing it? E.g. creating an IDBTransactionBackendImpl which holds a reference?

[EFL EWS Bot]

Comment on attachment 181791 [details]
Patch

Attachment 181791 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15763293

[WebKit Review Bot]

Comment on attachment 181791 [details]
Patch

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

[Alec Flett]

Created attachment 182026 [details]
Patch

[Alec Flett]

showed the updated IDBDatabaseBackendTest to jsbell in person he sez "lgtm"

All other issues have been addressed.

tony@ - r?

[WebKit Review Bot]

Comment on attachment 182026 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 182026 [details]
Patch

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

[Alec Flett]

Created attachment 182037 [details]
Patch

[Alec Flett]

Created attachment 182040 [details]
Patch

[Tony Chang]

Comment on attachment 182040 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182040&action=review

> Source/WebCore/ChangeLog:35
> +        * Modules/indexeddb/IDBDatabaseBackendImpl.cpp: Add all create/delete operations.

Nit: I would say where these are copied from.

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:608
> +// FIXME: This should do some error handling rather than plowing ahead when bad data is encountered.

Are there security implications of plowing ahead when bad data is encountered?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:69
> +    virtual PassRefPtr<IDBObjectStoreBackendInterface> createObjectStore(int64_t id, const String& name, const IDBKeyPath&, bool autoIncrement, IDBTransactionBackendInterface*, ExceptionCode&)  { ASSERT_NOT_REACHED(); return 0; }
> +    virtual void createObjectStore(int64_t transactionId, int64_t objectStoreId, const String& name, const IDBKeyPath&, bool autoIncrement);
> +    virtual void deleteObjectStore(int64_t, IDBTransactionBackendInterface*, ExceptionCode&) { ASSERT_NOT_REACHED(); }
> +    virtual void deleteObjectStore(int64_t transactionId, int64_t objectStoreId);

Should these have OVERRIDE?

[EFL EWS Bot]

Comment on attachment 182040 [details]
Patch

Attachment 182040 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15758795

[Alec Flett]

Created attachment 182061 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 182061 [details]
Patch for landing

Rejecting attachment 182061 [details] from commit-queue.

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Full output: http://queues.webkit.org/results/15774678

[Alec Flett]

Comment on attachment 182061 [details]
Patch for landing

sigh. Those are not my failures. trying again.

[WebKit Review Bot]

Comment on attachment 182061 [details]
Patch for landing

Clearing flags on attachment: 182061

Committed r139289: <http://trac.webkit.org/changeset/139289>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.