Summary: | IndexedDB: Implement IDBIndex multientry feature | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||||||
Component: | WebKit Misc. | Assignee: | Joshua Bell <jsbell> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, hans, ojan, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Joshua Bell
2011-11-28 11:13:41 PST
Created attachment 116787 [details]
Patch
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.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. Comment on attachment 116787 [details]
Patch
WebKit API changes LGTM
Very nice! LGTM Created attachment 117082 [details]
Patch
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 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 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 Created attachment 117204 [details]
Patch
Comment on attachment 117204 [details]
Patch
Addressed Hans' comments. Also, hopefully the flaky SVG test will go away.
Comment on attachment 117204 [details]
Patch
tony@ or anyone else, want to take a look?
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. (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. Created attachment 117249 [details]
Patch
Comment on attachment 117249 [details]
Patch
Just the comment nits tony@ pointed out.
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. (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 on attachment 117249 [details] Patch Clearing flags on attachment: 117249 Committed r101602: <http://trac.webkit.org/changeset/101602> All reviewed patches have been landed. Closing bug. |