Bug 102741 – IndexedDB: Migrate backend ObjectStore/Index calls to use transaction id

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

[Alec Flett]

Rather than calling through IDBTransactionBackendInterface, backend object store calls should just call through IDBDatabase with a transaction id

specifically: (leaving out the details of request/callback hookup/etc)
* IDBObjectStoreBackendInterface::openCursor() becomes 
  IDBDatabaseBackendInterface::openCursor(transaction_id, objectstore_id, -1, keyRange, direction, key=false);
* IDBObjectStoreBackendInterface::openKeyCursor() becomes 
  IDBDatabaseBackendInterface::openCursor(transaction_id, objectstore_id, -1, keyRange, direction, key=true);
* IDBObjectStoreBackendInterface::count() becomes 
  IDBDatabaseBackendInterface::count(transaction_id, objectstore_id, -1, keyRange);
* IDBObjectStoreBackendInterface::get() becomes 
  IDBDatabaseBackendInterface::get(transaction_id, objectstore_id, -1, keyRange, key=false);
* IDBObjectStoreBackendInterface::put() becomes
  IDBDatabaseBackendInterface::put(transaction_id, objectstore_id, -1, key);
* IDBObjectStoreBackendInterface::deleteFunctiuon() becomes
  IDBDatabaseBackendInterface::deleteFunction(transaction_id, objectstore_id, -1, keyRange);
* IDBObjectStoreBackendInterface::clear() becomes
  IDBDatabaseBackendInterface::clear(transaction_id, objectstore_id);
* IDBObjectStoreBackendInterface::setIndexKeys() becomes
  IDBDatabaseBackendInterface::setIndexKeys(transaction_id, objectstore_id, index_ids, index_keys, ...);
* IDBObjectStoreBackendInterface::setIndexesReady() becomes
  IDBDatabaseBackendInterface::setIndexesReady(transaction_id, objectstore_id, index_ids);


This bug does not cover:
* IDBObjectStoreBackendInterface::createIndex()
* IDBObjectStoreBackendInterface::deleteIndex()

(I'm leaving openKeyCursor in here for now, even though it isn't yet implemented in the IDL or in the spec)

[Alec Flett]

Created attachment 181251 [details]
Patch

[Alec Flett]

*** Bug 102736 has been marked as a duplicate of this bug. ***

[Alec Flett]

jsbell/dgrogan - this is my mega-patch as promised. this does NOT yet incorporate josh's changes to the scheduleTask() stuff, but you can take a look at it now, and I'll update it on friday (2012-01-04) while you're reviewing.

[Alec Flett]

Created attachment 181316 [details]
Patch

[Alec Flett]

ok, this is fully ready for idb review now.

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

[WebKit Review Bot]

Comment on attachment 181316 [details]
Patch

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

[Joshua Bell]

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

Overall LGTM with some merge glitches, nits, and one question.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:509
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Don't need this since the PassRefPtr is otherwise unused in this method - can just pass the argument straight through.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:532
> +                // Index Value Retrieval Operation

Mixing the Object Store and Index operations together makes this method fairly unweildy. Did you try keeping it as two separate operations, with the IDBDatabaseBackendImpl::get() deciding which to create?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:763
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Don't need this refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:806
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Don't need this refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:816
> +    IDB_TRACE("ObjectStoreCountOperation");

This now handles Indexes as well, so just CountOperation.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:840
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Don't need this refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:866
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Don't need this refptr churn.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:208
> +IDBDatabaseBackendInterface* IDBIndex::backendDB() const

This is short enough that I'd put it in the header file.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:216
> +    backendDB()->put(m_transaction->id(), id(), &valueBytes, key.release(), static_cast<IDBDatabaseBackendInterface::PutMode>(putMode), request, indexIds, indexKeys);

Subsequent patch will collapse the enumerations into just the IDBDatabaseBackendInterface versions to avoid the casting?

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:553
> +IDBDatabaseBackendInterface* IDBObjectStore::backendDB() const

Move to the header?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:144
> +    if (!transaction->scheduleTask(ObjectStoreRetrievalOperation::create(this, keyRange, callbacks)))

Looks like a merge glitch - shouldn't need to send the AbortError in this async call.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:195
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::AbortError));

Looks like a merge glitch - shouldn't need to send the AbortError in this async call.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:346
> +        RefPtr<IDBKey> autoIncKey = IDBObjectStoreBackendImpl::generateKey(m_objectStore->backingStore(), transaction, m_objectStore->databaseId(), m_objectStore->id());

The key generator logic will be flattened into the DB backend in a subsequent patch?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:418
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::AbortError));

Looks like a merge glitch - shouldn't need to send the AbortError in this async call.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:446
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::AbortError));

Looks like a merge glitch - shouldn't need to send the AbortError in this async call.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:466
> +IDBDatabaseBackendInterface* IDBTransaction::backendDB() const

Move to header?

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:-167
> -    Vector<uint8_t> valueBuffer(value->size());

Nice find.

> LayoutTests/ChangeLog:8
> +        Add additional count() tests for multi-entry indexes, not previously

Also nice!

[Alec Flett]

Created attachment 181326 [details]
Patch

oops, bad merge - trying again

[Joshua Bell]

Comment on attachment 181326 [details]
Patch

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

still LGTM with nits

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:509
> -    ASSERT_NOT_REACHED();
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Unnecessary refptr churn. (Not new, though, was in the original location too.)

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:608
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Unnecessary refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:763
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Unnecessary refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:806
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Unnecessary refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:840
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Unnecessary refptr churn.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:866
> +    RefPtr<IDBCallbacks> callbacks = prpCallbacks;

Unnecessary refptr churn.

[Peter Beverloo (cr-android ews)]

Comment on attachment 181326 [details]
Patch

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

[Alec Flett]

Created attachment 181353 [details]
Patch

[Alec Flett]

ok, new patch addresses nits, also:

1) I couldn't make backendDB() methods be part of the class definition because of all sorts of crazy circular dependencies between IDBDatabaseBackendImpl, IDBTransactionBackendImp, IDBObjectStoreBackendImpl and the like. These will hopefully be fixed by the overall refactor and I can make a pass through and clean up little stuff like that.

2) Yup, subsequent patches will move all code, including enums and static methods, out of IDBObjectStoreBackend* and IDBIndexBackend* so those casts will eventually go away.

tony@ - r? This is a biggie, I'm happy to discuss before you start reviewing, so you get the basic idea.

[Alec Flett]

actually CC tony this time.

tony@ - r? This is a biggie, I'm happy to discuss before you start reviewing, so you get the basic idea.

[Tony Chang]

Comment on attachment 181353 [details]
Patch

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

LGTM as it's mostly moving code around.

> Source/WebCore/ChangeLog:37
> +        * Modules/indexeddb/IDBCursorBackendImpl.cpp:
> +        (WebCore::IDBCursorBackendImpl::IDBCursorBackendImpl):
> +        (WebCore::IDBCursorBackendImpl::~IDBCursorBackendImpl):
> +        (WebCore::IDBCursorBackendImpl::deleteFunction):
> +        * Modules/indexeddb/IDBCursorBackendImpl.h:
> +        (WebCore::IDBCursorBackendImpl::create):
> +        (IDBCursorBackendImpl):
> +        * Modules/indexeddb/IDBDatabase.h:
> +        (WebCore::IDBDatabase::backend):
> +        (IDBDatabase):
> +        * Modules/indexeddb/IDBDatabaseBackendImpl.cpp:
> +        (GetOperation):

It would be helpful to fill some of this in. Specifically, you could point out when you're moving code vs when you're changing code.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.h:87
> +    // Order is important, the cursors have to be destructed before the transaction.
> +    RefPtr<IDBBackingStore::Cursor> m_cursor;
> +    RefPtr<IDBBackingStore::Cursor> m_savedCursor;

Nit: I would probably do end of line comments to explain dependencies like you would do for locks.  E.g.:
  RefPtr<IDBBackingStore::Cursor> m_cursor; // Must be destroyed before m_transaction.
  RefPtr<IDBBackingStore::Cursor> m_savedCursor; // Must be destroyed before m_transaction.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:167
> +
> +    void perform(IDBTransactionBackendImpl*);
> +

Nit: Maybe get rid of the blank lines around perform()? The other Operation declarations don't have these blank lines.

[Alec Flett]

Created attachment 181378 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181378 [details]
Patch for landing

Clearing flags on attachment: 181378

Committed r138900: <http://trac.webkit.org/changeset/138900>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.