Bug 72567 – IndexedDB: Indexes should be secondarily sorted on primary key

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

[Joshua Bell]

IndexedDB: Indexes should be secondarily sorted on primary key

[Joshua Bell]

Created attachment 115491 [details]
Patch

[Joshua Bell]

I should probably add something to the IDBLevelDBCodingTest.cpp

[Joshua Bell]

Created attachment 115623 [details]
Patch

[Joshua Bell]

Comment on attachment 115623 [details]
Patch

Ugh, I actually need to remove some of the logic around "sequence number"; it's unnecessary for new records.

[Joshua Bell]

Created attachment 115652 [details]
Patch

[Joshua Bell]

Comment on attachment 115652 [details]
Patch

Should be good to go now, review at leisure.

[Hans Wennborg]

Comment on attachment 115652 [details]
Patch

Looks great, thanks for fixing this!

View in context: https://bugs.webkit.org/attachment.cgi?id=115652&action=review

> Source/WebCore/ChangeLog:7
> +

Maybe say a few words in the Changelog about how this is implemented, i.e. that we're appending the primary key to index data keys.

> Source/WebKit/chromium/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

I would probably add a line saying that this updates the unit test.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:1429
> +Vector<char> IndexDataKey::encode(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const Vector<char>& encodedUserKey, const Vector<char>& encodedPrimaryKey, int64_t sequenceNumber)

hmm, maybe we should change the parameter order here? So far, I think they've been in the same order that we encode them.

> LayoutTests/storage/indexeddb/cursor-primary-key-order.html:95
> +    curreq = evalAndLog("curreq = index.openCursor()");

not super happy about "curreq" as name.. cursorRequest, or just request, maybe?

[Joshua Bell]

(In reply to comment #7)
> (From update of attachment 115652 [details])
> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:1429
> > +Vector<char> IndexDataKey::encode(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const Vector<char>& encodedUserKey, const Vector<char>& encodedPrimaryKey, int64_t sequenceNumber)
> 
> hmm, maybe we should change the parameter order here? So far, I think they've been in the same order that we encode them.

Following this change, sequenceNumber is unnecessary except for testing; it has a default in the header, so I wanted it to be last.

Alternately, I could add another overload that takes sequenceNumber in the "correct" order, that's only used for testing.

[Hans Wennborg]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 115652 [details] [details])
> > 
> > > Source/WebCore/storage/IDBLevelDBCoding.cpp:1429
> > > +Vector<char> IndexDataKey::encode(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const Vector<char>& encodedUserKey, const Vector<char>& encodedPrimaryKey, int64_t sequenceNumber)
> > 
> > hmm, maybe we should change the parameter order here? So far, I think they've been in the same order that we encode them.
> 
> Following this change, sequenceNumber is unnecessary except for testing; it has a default in the header, so I wanted it to be last.

Didn't realize there was a default in the header. Never mind, then.

[Joshua Bell]

Created attachment 116386 [details]
Patch

[Joshua Bell]

tony@ can you take a look?

[WebKit Review Bot]

Comment on attachment 116386 [details]
Patch

Clearing flags on attachment: 116386

Committed r101142: <http://trac.webkit.org/changeset/101142>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.