[webkit-reviews] review canceled: [Bug 90923] IndexedDB: Introduce putWithIndexKeys and calculate them in the renderer : [Attachment 151562] Patch

[bugzilla-daemon at webkit.org]

Alec Flett <alecflett at chromium.org> has canceled Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 90923: IndexedDB: Introduce putWithIndexKeys and calculate them in the
renderer
https://bugs.webkit.org/show_bug.cgi?id=90923

Attachment 151562: Patch
https://bugs.webkit.org/attachment.cgi?id=151562&action=review

------- Additional Comments from Alec Flett <alecflett at chromium.org>
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.