Summary: | IndexedDB: Migrate backend ObjectStore/Index calls to use transaction id | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alec Flett <alecflett> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Alec Flett <alecflett> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dgrogan, fishd, jamesr, jsbell, peter+ews, tkent+wkapi, tony, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 102732, 102742, 104592 | ||||||||||||||
Bug Blocks: | 99774 | ||||||||||||||
Attachments: |
|
Description
Alec Flett
2012-11-19 16:44:33 PST
Created attachment 181251 [details]
Patch
*** Bug 102736 has been marked as a duplicate of this bug. *** 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. Created attachment 181316 [details]
Patch
ok, this is fully ready for idb review now. 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 on attachment 181316 [details] Patch Attachment 181316 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15708219 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! Created attachment 181326 [details]
Patch
oops, bad merge - trying again
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 on attachment 181326 [details] Patch Attachment 181326 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15709222 Created attachment 181353 [details]
Patch
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. 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 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. Created attachment 181378 [details]
Patch for landing
Comment on attachment 181378 [details] Patch for landing Clearing flags on attachment: 181378 Committed r138900: <http://trac.webkit.org/changeset/138900> All reviewed patches have been landed. Closing bug. |