IndexedDB: Introduce putWithIndexKeys and calculate them in the renderer
Created attachment 151562 [details] Patch
jsbell@ - care to take a look? After this comes a chromium patch to hook back up to putWithIndexKeys()
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 151562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151562&action=review I'm still not sure how this approach will work with indexes created after the put() call. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:-176 > - // FIXME: Pass through keyPathKey as key to simplify back end implementation. Why is this comment being removed? > Source/WebCore/Modules/indexeddb/IDBObjectStore.h:106 > + typedef HashMap<String, IndexKeys> IndexKeyMap; Is this second typedef used? > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:156 > + OwnPtr<Vector<String> > nullIndexNames; Unless they will diverge in the future, rather than having put() duplicate the (admittedly minimal) logic of putWithIndexKeys() I'd just have it turn around and call putWithIndexKeys() with the two empty vectors. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:157 > + OwnPtr<Vector<IndexKeys> > nullIndexKeys; Maybe add a comment that null pointers signify to putInternal that it should compute the keys, since not everything in the new pipeline is functional yet. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:231 > + bool verifyIndexKeys(IDBBackingStore& backingStore, Mark method as const. Make the param a const reference, while you're there? (Will require touching IDBBackingStore and IDBLevelDBBackingStore since keyExistsInObjectStore is not marked const) > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:247 > + bool writeIndexKeys(const IDBBackingStore::ObjectStoreRecordIdentifier* recordIdentifier, IDBBackingStore& backingStore, int64_t databaseId, int64_t objectStoreId, int64_t indexId) Mark method as const. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:262 > + bool addingKeyAllowed(IDBBackingStore& backingStore, Mark method as const. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:308 > + // FIXME: remove this when get-side key injection is the norm. Capitalize. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:351 > + if (indexKeys) Add FIXME that keys will eventually be passed in and this can change to an ASSERT and the else block can go away.
Comment on attachment 151562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151562&action=review >> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:-176 >> - // FIXME: Pass through keyPathKey as key to simplify back end implementation. > > Why is this comment being removed? oops, this belongs in the next patch, which does this. >> Source/WebCore/Modules/indexeddb/IDBObjectStore.h:106 >> + typedef HashMap<String, IndexKeys> IndexKeyMap; > > Is this second typedef used? yes, in IDBObjectStore::put() >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:156 >> + OwnPtr<Vector<String> > nullIndexNames; > > Unless they will diverge in the future, rather than having put() duplicate the (admittedly minimal) logic of putWithIndexKeys() I'd just have it turn around and call putWithIndexKeys() with the two empty vectors. good point. [Later] Ooops, I now remember why I did it this way: 1) there's a slight difference between a null pointer and an empty array (as you noticed in your next comment) 2) due to ownership/callback issues I had to make putInternal take a pointer (const Vector<> *), whereas putWithIndexKeys takes a reference (const Vector<>&) - and I cant pass null to a reference. This is only a band-aid solution that will be cleaned up in my next patch (Which I've already written and tested) which removes the excess methods. >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:231 >> + bool verifyIndexKeys(IDBBackingStore& backingStore, > > Mark method as const. > > Make the param a const reference, while you're there? (Will require touching IDBBackingStore and IDBLevelDBBackingStore since keyExistsInObjectStore is not marked const) Tried this already - keyExistsInObjectStore deletes stale entries from the database, so it can't be const, and thus neither can addingKeyAllowed, nor this method. >> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:262 >> + bool addingKeyAllowed(IDBBackingStore& backingStore, > > Mark method as const. (as mentioned above, tried this and can't due to the fact that keyExistsInIndex tweaks stale entries (via findKeyInIndex) - I'm happy to try to clean up that a bit in a separate pass though - one const path and one non-const path.
Created attachment 151824 [details] Patch
jsbell@ - latest patch addresses outstanding review comments, sans a few minor issues described above. Another r?
Comment on attachment 151824 [details] Patch lgtm, just nits. View in context: https://bugs.webkit.org/attachment.cgi?id=151824&action=review > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:-458 > - Eliminate this diff so this file is not touched just for a whitespace change. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:158 > + // null pointers here signifiy that index keys should be generated as a part of this put. Capitalize null, typo: "signify"
Created attachment 152020 [details] Patch
tony@ r? for idb stuff dglazkov@ r? for chromium stuff? (just stubbing out new methods)
Comment on attachment 152020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152020&action=review LGTM. Passing off to dglazkov for API review. > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:343 > + for (size_t i = 0; i < indexNames->size(); i++) Nit: ++i
thanks tony, will do. fishd@ - dglazkov is on vacation. mind a quick r? for chromium API stuff?
Comment on attachment 152020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152020&action=review > Source/WebKit/chromium/public/WebIDBObjectStore.h:57 > + virtual void putWithIndexKeys(const WebSerializedScriptValue&, const WebIDBKey&, PutMode, WebIDBCallbacks*, const WebIDBTransaction&, const WebVector<WebString>&, const WebVector<WebIndexKeys>&, WebExceptionCode&) { WEBKIT_ASSERT_NOT_REACHED(); } nit: give the WebVector<WebString> parameter a name? using indexNames would help a lot.
Created attachment 152626 [details] Patch for landing
Comment on attachment 152626 [details] Patch for landing Clearing flags on attachment: 152626 Committed r122779: <http://trac.webkit.org/changeset/122779>
All reviewed patches have been landed. Closing bug.