Bug 156760 – Modern IDB: Lots of IDB bindings cleanup (including making IDBVersionChangeEvent constructible)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: Lots of IDB bindings cleanup (including making IDBVersionChangeEvent constructible)

[Brady Eidson]

Created attachment 276770 [details]
Patch v1

[Darin Adler]

Comment on attachment 276770 [details]
Patch v1

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

> Source/WebCore/bindings/js/JSDictionary.h:108
> +        if (value.isUndefinedOrNull()) {

Undefined makes sense (missing property). Not sure null does. Needs test coverage.

[Chris Dumez]

Comment on attachment 276770 [details]
Patch v1

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

> LayoutTests/storage/indexeddb/modern/idbversionchangeevent-constructor-expected.txt:15
> +PASS event.newVersion is 0

This should probably test a null newVersion.

[Chris Dumez]

Comment on attachment 276770 [details]
Patch v1

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

>> LayoutTests/storage/indexeddb/modern/idbversionchangeevent-constructor-expected.txt:15
>> +PASS event.newVersion is 0
> 
> This should probably test a null newVersion.

i.e. event = new IDBVersionChangeEvent('bar', { oldVersion: 43876528735628, newVersion: null });

[Alex Christensen]

Comment on attachment 276770 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/WorkerGlobalScopeIndexedDatabase.idl:31
> +    [EnabledAtRuntime=IndexedDB, ImplementedAs=indexedDB] readonly attribute IDBFactory webkitIndexedDB;

Let's get rid of this, since we've never shipped a prefixed indexeddb in workers.

[Brady Eidson]

(In reply to comment #5)
> Comment on attachment 276770 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276770&action=review
> 
> > Source/WebCore/Modules/indexeddb/WorkerGlobalScopeIndexedDatabase.idl:31
> > +    [EnabledAtRuntime=IndexedDB, ImplementedAs=indexedDB] readonly attribute IDBFactory webkitIndexedDB;
> 
> Let's get rid of this, since we've never shipped a prefixed indexeddb in
> workers.

(In reply to comment #4)
> Comment on attachment 276770 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276770&action=review
> 
> >> LayoutTests/storage/indexeddb/modern/idbversionchangeevent-constructor-expected.txt:15
> >> +PASS event.newVersion is 0
> > 
> > This should probably test a null newVersion.
> 
> i.e. event = new IDBVersionChangeEvent('bar', { oldVersion: 43876528735628,
> newVersion: null });

Tested an undefined one, but you're totally right, it should!

[Brady Eidson]

(In reply to comment #5)
> Comment on attachment 276770 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276770&action=review
> 
> > Source/WebCore/Modules/indexeddb/WorkerGlobalScopeIndexedDatabase.idl:31
> > +    [EnabledAtRuntime=IndexedDB, ImplementedAs=indexedDB] readonly attribute IDBFactory webkitIndexedDB;
> 
> Let's get rid of this, since we've never shipped a prefixed indexeddb in
> workers.

Yup. Gone.

[Chris Dumez]

Comment on attachment 276770 [details]
Patch v1

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

> LayoutTests/storage/indexeddb/modern/idbversionchangeevent-constructor.html:30
> +finishJSTest();

This should not be needed to non-async tests?

> Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.h:38
> +    Optional<uint64_t> newVersion { Nullopt };

I don't think we need the "{ Nullopt }", do we?

>> Source/WebCore/bindings/js/JSDictionary.h:108
>> +        if (value.isUndefinedOrNull()) {
> 
> Undefined makes sense (missing property). Not sure null does. Needs test coverage.

Looks good to me, for nullable attributes, we treat set the attribute to null if the JS passes null or undefined, as per Web IDL.

[Chris Dumez]

Comment on attachment 276770 [details]
Patch v1

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

>>>> LayoutTests/storage/indexeddb/modern/idbversionchangeevent-constructor-expected.txt:15
>>>> +PASS event.newVersion is 0
>>> 
>>> This should probably test a null newVersion.
>> 
>> i.e. event = new IDBVersionChangeEvent('bar', { oldVersion: 43876528735628, newVersion: null });
> 
> Tested an undefined one, but you're totally right, it should!

Also one with the explicit undefined:
event = new IDBVersionChangeEvent('bar', { oldVersion: 43876528735628, newVersion: undefined });
-> should set to null

[Brady Eidson]

Created attachment 276771 [details]
Patch for landing

I found one other test whose results were affected, and incorporated review feedback.

I'll let EWS churn before cq+'ing

[Brady Eidson]

iOS sim failures seem unrelated to this patch.

cq+

[WebKit Commit Bot]

Comment on attachment 276771 [details]
Patch for landing

Clearing flags on attachment: 276771

Committed r199750: <http://trac.webkit.org/changeset/199750>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 276771 [details]
Patch for landing

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

> Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.h:38
> +    Optional<uint64_t> newVersion { Nullopt };

Oops, missed this. No need to state this explicitly, because Optional is set to Nullopt by default.

[Chris Dumez]

(In reply to comment #14)
> Comment on attachment 276771 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276771&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.h:38
> > +    Optional<uint64_t> newVersion { Nullopt };
> 
> Oops, missed this. No need to state this explicitly, because Optional is set
> to Nullopt by default.

This was also part of my review comments.

[Brady Eidson]

(In reply to comment #15)
> (In reply to comment #14)
> > Comment on attachment 276771 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=276771&action=review
> > 
> > > Source/WebCore/Modules/indexeddb/IDBVersionChangeEvent.h:38
> > > +    Optional<uint64_t> newVersion { Nullopt };
> > 
> > Oops, missed this. No need to state this explicitly, because Optional is set
> > to Nullopt by default.
> 
> This was also part of my review comments.

Yup. Missed that, sorry :(

Also didn't do the "explicit undefined" test.

Will followup on both once I have a clean tree.

[Chris Dumez]

Comment on attachment 276771 [details]
Patch for landing

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

> LayoutTests/storage/indexeddb/modern/idbversionchangeevent-constructor-expected.txt:10
> +event = new IDBVersionChangeEvent('bar', { oldVersion: 6, newVersion: null });

We are missing a test for newVersion: undefined (as mentioned in my earlier review comments)

[Brady Eidson]

(In reply to comment #17)
> We are missing a test for newVersion: undefined (as mentioned in my earlier
> review comments)

But in the comment before...

(In reply to comment #16)
> Yup. Missed that, sorry :(
> 
> Also didn't do the "explicit undefined" test.
> 
> Will followup on both once I have a clean tree.

=D

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #17)
> > We are missing a test for newVersion: undefined (as mentioned in my earlier
> > review comments)
> 
> But in the comment before...
> 
> (In reply to comment #16)
> > Yup. Missed that, sorry :(
> > 
> > Also didn't do the "explicit undefined" test.
> > 
> > Will followup on both once I have a clean tree.
> 
> =D

Yes, you beat me by 14 seconds :)

[Brady Eidson]

http://trac.webkit.org/changeset/199774

[Brady Eidson]

(In reply to comment #20)
> http://trac.webkit.org/changeset/199774

(That was the followup)