Bug 73232 – IndexedDB: Implement IDBIndex multientry feature

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Joshua Bell]

Search fodder: Older revisions of the spec call this: multirow

TL;DR: On insert, when updating an index, if the index's multientry flag is true and evaluating the keyPath against the value to be inserted yields an Array as the index key, then the index is updated with each value in the Array as a separate indexKey.

[Joshua Bell]

Created attachment 116787 [details]
Patch

[Joshua Bell]

Comment on attachment 116787 [details]
Patch

Not ready to land yet, as it doesn't actually implement the feature or tests, just the new flag.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116787 [details]
Patch

WebKit API changes LGTM

[Hans Wennborg]

Very nice!

LGTM

[Joshua Bell]

Created attachment 117082 [details]
Patch

[Joshua Bell]

Okay, this is ready for a serious look now. 

This would land first, followed by the Chromium side is http://codereview.chromium.org/8743001/ - followed by the trivial removal of the old createIndex() WebKit API method that doesn't have the new flag.

[Hans Wennborg]

Comment on attachment 117082 [details]
Patch

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

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:247
> +            && !index->addingKeyAllowed(indexKey.get(), key.get())) {

nit: no need for linebreak

> LayoutTests/storage/indexeddb/index-multientry.html:114
> +    debug("This should fail the uniqueness constraint on the index, and fail:");

Could you add a record to make sure that it's possible to update a record, e.g. store.put({x: [1,2,7], y: 'a') ?

[WebKit Review Bot]

Comment on attachment 117082 [details]
Patch

Attachment 117082 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10697153

New failing tests:
svg/transforms/text-with-pattern-with-svg-transform.svg

[Joshua Bell]

Created attachment 117204 [details]
Patch

[Joshua Bell]

Comment on attachment 117204 [details]
Patch

Addressed Hans' comments. Also, hopefully the flaky SVG test will go away.

[Joshua Bell]

Comment on attachment 117204 [details]
Patch

tony@ or anyone else, want to take a look?

[Tony Chang]

Comment on attachment 117204 [details]
Patch

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

Do you have commit access?  If so, you can use 'webkit-patch land-safely' after making the changes locally.  If not, I'll see about nominating you.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:715
> +        // FIXME: Add encode/decode functions for bools

Nit: Add a period to the end of the sentence.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:728
> +            // FIXME: Add encode/decode functions for bools

Nit: Add a period to the end of the sentence.

[Joshua Bell]

(In reply to comment #13)
> 
> Do you have commit access?  If so, you can use 'webkit-patch land-safely' after making the changes locally.  If not, I'll see about nominating you.
> 

Nope, I don't have commit access yet.

[Joshua Bell]

Created attachment 117249 [details]
Patch

[Joshua Bell]

Comment on attachment 117249 [details]
Patch

Just the comment nits tony@ pointed out.

[Darin Fisher (:fishd, Google)]

Comment on attachment 117249 [details]
Patch

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

> Source/WebKit/chromium/public/WebIDBIndex.h:64
> +    virtual bool multientry() const

nit: is "multientry" really one word?  i would have thought that multi-entry was the correct spelling, and so we would therefore name this field multiEntry.

[Joshua Bell]

(In reply to comment #17)
> (From update of attachment 117249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117249&action=review
> 
> > Source/WebKit/chromium/public/WebIDBIndex.h:64
> > +    virtual bool multientry() const
> 
> nit: is "multientry" really one word?  i would have thought that multi-entry was the correct spelling, and so we would therefore name this field multiEntry.

That's how it's written in the spec - http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html - earlier iterations called it "multirow". We may be the first to implement it with the new name, though. I'll ask on the list.

[WebKit Review Bot]

Comment on attachment 117249 [details]
Patch

Clearing flags on attachment: 117249

Committed r101602: <http://trac.webkit.org/changeset/101602>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.