Bug 163805

Summary: IndexedDB 2.0: Support IDBIndex name assignment
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, cdumez, commit-queue, esprehn+autocc, jsbell, kondapallykalyan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 160306    
Attachments:
Description Flags
Patch
none
Patch none

Description Brady Eidson 2016-10-21 12:38:47 PDT
IndexedDB 2.0: Support IDBIndex name assignment
Comment 1 Brady Eidson 2016-10-21 12:39:08 PDT
<rdar://problem/28806932>
Comment 2 Brady Eidson 2016-10-21 15:27:35 PDT
Created attachment 292420 [details]
Patch
Comment 3 Alex Christensen 2016-10-21 19:24:44 PDT
Comment on attachment 292420 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:197
> +template<typename MP1, typename P1, typename MP2, typename P2, typename MP3, typename P3>

Could this be variadic with template<...> template<...> ?

> LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idbindex-rename-errors-expected.txt:6
> +FAIL IndexedDB index rename throws in an inactive transaction assert_throws: function "() => authorIndex.name = 'renamed_by_author'" threw object "InvalidStateError (DOM Exception 11): Failed set property..." that is not a DOMException TransactionInactiveError: property "code" is equal to 11, expected 0
> +FAIL IndexedDB index rename to the name of another index throws assert_throws: function "() => index.name = 'by_title'" threw object "InvalidStateError (DOM Exception 11): Failed set property..." that is not a DOMException ConstraintError: property "code" is equal to 11, expected 0

It seems like not all your errors are correct.
Comment 4 Alex Christensen 2016-10-21 19:26:49 PDT
Comment on attachment 292420 [details]
Patch

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

> LayoutTests/storage/indexeddb/modern/resources/index-rename-1.js:104
> +		index.name = "YetAnotherName";

Try setting it to an existing name and make sure it overwrites the other existing index.
Try setting it to its same name.
Comment 5 Brady Eidson 2016-10-24 08:43:14 PDT
(In reply to comment #3)
> Comment on attachment 292420 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292420&action=review
> 
> > Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:197
> > +template<typename MP1, typename P1, typename MP2, typename P2, typename MP3, typename P3>
> 
> Could this be variadic with template<...> template<...> ?

Possibly, but as we learned with the Function overhauls, a lot of edge cases come up.

I already scribbled "Fully templatize TransactionOperation" on my white board. :)

> > LayoutTests/imported/w3c/web-platform-tests/IndexedDB/idbindex-rename-errors-expected.txt:6
> > +FAIL IndexedDB index rename throws in an inactive transaction assert_throws: function "() => authorIndex.name = 'renamed_by_author'" threw object "InvalidStateError (DOM Exception 11): Failed set property..." that is not a DOMException TransactionInactiveError: property "code" is equal to 11, expected 0
> > +FAIL IndexedDB index rename to the name of another index throws assert_throws: function "() => index.name = 'by_title'" threw object "InvalidStateError (DOM Exception 11): Failed set property..." that is not a DOMException ConstraintError: property "code" is equal to 11, expected 0
> 
> It seems like not all your errors are correct.

This is *known* to not be correct - We're not making an effort to 100% fix these tests yet.

Doing so is clearly part of the IDB 2.0 effort.

(In reply to comment #4)
> Comment on attachment 292420 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292420&action=review
> 
> > LayoutTests/storage/indexeddb/modern/resources/index-rename-1.js:104
> > +		index.name = "YetAnotherName";
> 
> Try setting it to an existing name and make sure it overwrites the other
> existing index.

We already try that, and it does not overwrite the other one; It throws an exception.

> Try setting it to its same name.

This will just throw an exception, as well, but should be tested. Thanks!
Comment 6 Brady Eidson 2016-10-24 08:46:50 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > Try setting it to its same name.
> 
> This will just throw an exception, as well, but should be tested. Thanks!

Sorry I lied, no exception - It just stops the rename algorithm there.

Still will add the test.
Comment 7 Brady Eidson 2016-10-24 08:51:52 PDT
Created attachment 292614 [details]
Patch
Comment 8 WebKit Commit Bot 2016-10-24 09:26:58 PDT
Comment on attachment 292614 [details]
Patch

Clearing flags on attachment: 292614

Committed r207761: <http://trac.webkit.org/changeset/207761>
Comment 9 WebKit Commit Bot 2016-10-24 09:27:03 PDT
All reviewed patches have been landed.  Closing bug.