Bug 90923 – IndexedDB: Introduce putWithIndexKeys and calculate them in the renderer

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

[Alec Flett]

IndexedDB: Introduce putWithIndexKeys and calculate them in the renderer

[Alec Flett]

Created attachment 151562 [details]
Patch

[Alec Flett]

jsbell@ - care to take  a look?

After this comes a chromium patch to hook back up to putWithIndexKeys()

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

[Joshua Bell]

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.

[Alec Flett]

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.

[Alec Flett]

Created attachment 151824 [details]
Patch

[Alec Flett]

jsbell@ - latest patch addresses outstanding review comments, sans a few minor issues described above. Another r?

[Joshua Bell]

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"

[Alec Flett]

Created attachment 152020 [details]
Patch

[Alec Flett]

tony@ r? for idb stuff

dglazkov@ r? for chromium stuff? (just stubbing out new methods)

[Tony Chang]

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

[Alec Flett]

thanks tony, will do.

fishd@ - dglazkov is on vacation. mind a quick r? for chromium API stuff?

[Darin Fisher (:fishd, Google)]

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.

[Alec Flett]

Created attachment 152626 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152626 [details]
Patch for landing

Clearing flags on attachment: 152626

Committed r122779: <http://trac.webkit.org/changeset/122779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.