Bug 73232 - IndexedDB: Implement IDBIndex multientry feature
Summary: IndexedDB: Implement IDBIndex multientry feature
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-28 11:13 PST by Joshua Bell
Modified: 2011-11-30 21:22 PST (History)
7 users (show)

See Also:


Attachments
Patch (37.34 KB, patch)
2011-11-28 11:16 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (58.02 KB, patch)
2011-11-29 17:33 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (58.77 KB, patch)
2011-11-30 09:13 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (59.67 KB, patch)
2011-11-30 12:39 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-11-28 11:13:41 PST
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.
Comment 1 Joshua Bell 2011-11-28 11:16:20 PST
Created attachment 116787 [details]
Patch
Comment 2 Joshua Bell 2011-11-28 11:16:51 PST
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.
Comment 3 WebKit Review Bot 2011-11-28 16:19:50 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Darin Fisher (:fishd, Google) 2011-11-28 16:31:47 PST
Comment on attachment 116787 [details]
Patch

WebKit API changes LGTM
Comment 5 Hans Wennborg 2011-11-29 08:06:43 PST
Very nice!

LGTM
Comment 6 Joshua Bell 2011-11-29 17:33:22 PST
Created attachment 117082 [details]
Patch
Comment 7 Joshua Bell 2011-11-29 17:39:20 PST
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.
Comment 8 Hans Wennborg 2011-11-30 04:01:34 PST
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') ?
Comment 9 WebKit Review Bot 2011-11-30 04:52:49 PST
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
Comment 10 Joshua Bell 2011-11-30 09:13:57 PST
Created attachment 117204 [details]
Patch
Comment 11 Joshua Bell 2011-11-30 09:14:44 PST
Comment on attachment 117204 [details]
Patch

Addressed Hans' comments. Also, hopefully the flaky SVG test will go away.
Comment 12 Joshua Bell 2011-11-30 10:04:41 PST
Comment on attachment 117204 [details]
Patch

tony@ or anyone else, want to take a look?
Comment 13 Tony Chang 2011-11-30 11:57:52 PST
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.
Comment 14 Joshua Bell 2011-11-30 12:02:04 PST
(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.
Comment 15 Joshua Bell 2011-11-30 12:39:35 PST
Created attachment 117249 [details]
Patch
Comment 16 Joshua Bell 2011-11-30 12:40:27 PST
Comment on attachment 117249 [details]
Patch

Just the comment nits tony@ pointed out.
Comment 17 Darin Fisher (:fishd, Google) 2011-11-30 14:52:10 PST
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.
Comment 18 Joshua Bell 2011-11-30 15:03:59 PST
(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.
Comment 19 WebKit Review Bot 2011-11-30 21:22:42 PST
Comment on attachment 117249 [details]
Patch

Clearing flags on attachment: 117249

Committed r101602: <http://trac.webkit.org/changeset/101602>
Comment 20 WebKit Review Bot 2011-11-30 21:22:48 PST
All reviewed patches have been landed.  Closing bug.