Bug 102741 - IndexedDB: Migrate backend ObjectStore/Index calls to use transaction id
Summary: IndexedDB: Migrate backend ObjectStore/Index calls to use transaction id
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:
: 102736 (view as bug list)
Depends on: 102732 102742 104592
Blocks: 99774
  Show dependency treegraph
 
Reported: 2012-11-19 16:44 PST by Alec Flett
Modified: 2013-01-05 12:44 PST (History)
10 users (show)

See Also:


Attachments
Patch (113.75 KB, patch)
2013-01-03 17:07 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (106.74 KB, patch)
2013-01-04 09:27 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (113.46 KB, patch)
2013-01-04 10:25 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (112.97 KB, patch)
2013-01-04 12:00 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (113.64 KB, patch)
2013-01-04 14:49 PST, 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-11-19 16:44:33 PST
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)
Comment 1 Alec Flett 2013-01-03 17:07:42 PST
Created attachment 181251 [details]
Patch
Comment 2 Alec Flett 2013-01-03 17:08:30 PST
*** Bug 102736 has been marked as a duplicate of this bug. ***
Comment 3 Alec Flett 2013-01-03 17:09:54 PST
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.
Comment 4 Alec Flett 2013-01-04 09:27:02 PST
Created attachment 181316 [details]
Patch
Comment 5 Alec Flett 2013-01-04 09:27:31 PST
ok, this is fully ready for idb review now.
Comment 6 WebKit Review Bot 2013-01-04 09:28:40 PST
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 7 WebKit Review Bot 2013-01-04 10:00:02 PST
Comment on attachment 181316 [details]
Patch

Attachment 181316 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15708219
Comment 8 Joshua Bell 2013-01-04 10:09:38 PST
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!
Comment 9 Alec Flett 2013-01-04 10:25:28 PST
Created attachment 181326 [details]
Patch

oops, bad merge - trying again
Comment 10 Joshua Bell 2013-01-04 11:33:53 PST
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.
Comment 11 Peter Beverloo (cr-android ews) 2013-01-04 11:40:05 PST
Comment on attachment 181326 [details]
Patch

Attachment 181326 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15709222
Comment 12 Alec Flett 2013-01-04 12:00:12 PST
Created attachment 181353 [details]
Patch
Comment 13 Alec Flett 2013-01-04 12:03:15 PST
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.
Comment 14 Alec Flett 2013-01-04 12:26:32 PST
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.
Comment 15 Tony Chang 2013-01-04 14:31:37 PST
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.
Comment 16 Alec Flett 2013-01-04 14:49:39 PST
Created attachment 181378 [details]
Patch for landing
Comment 17 WebKit Review Bot 2013-01-05 12:43:59 PST
Comment on attachment 181378 [details]
Patch for landing

Clearing flags on attachment: 181378

Committed r138900: <http://trac.webkit.org/changeset/138900>
Comment 18 WebKit Review Bot 2013-01-05 12:44:04 PST
All reviewed patches have been landed.  Closing bug.