Bug 156760 - Modern IDB: Lots of IDB bindings cleanup (including making IDBVersionChangeEvent constructible)
Summary: Modern IDB: Lots of IDB bindings cleanup (including making IDBVersionChangeEv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117
  Show dependency treegraph
 
Reported: 2016-04-19 14:59 PDT by Brady Eidson
Modified: 2016-04-20 09:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (27.39 KB, patch)
2016-04-19 15:59 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Patch for landing (32.16 KB, patch)
2016-04-19 16:19 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-04-19 14:59:47 PDT
Modern IDB: Lots of IDB bindings cleanup (including making IDBVersionChangeEvent constructible)
Comment 1 Brady Eidson 2016-04-19 15:59:05 PDT
Created attachment 276770 [details]
Patch v1
Comment 2 Darin Adler 2016-04-19 16:08:17 PDT
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.
Comment 3 Chris Dumez 2016-04-19 16:09:28 PDT
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.
Comment 4 Chris Dumez 2016-04-19 16:10:40 PDT
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 });
Comment 5 Alex Christensen 2016-04-19 16:14:38 PDT
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.
Comment 6 Brady Eidson 2016-04-19 16:15:39 PDT
(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!
Comment 7 Brady Eidson 2016-04-19 16:15:50 PDT
(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.
Comment 8 Chris Dumez 2016-04-19 16:16:26 PDT
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.
Comment 9 Chris Dumez 2016-04-19 16:17:11 PDT
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
Comment 10 Brady Eidson 2016-04-19 16:19:21 PDT
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
Comment 11 Brady Eidson 2016-04-19 17:28:16 PDT
iOS sim failures seem unrelated to this patch.

cq+
Comment 12 WebKit Commit Bot 2016-04-19 18:19:43 PDT
Comment on attachment 276771 [details]
Patch for landing

Clearing flags on attachment: 276771

Committed r199750: <http://trac.webkit.org/changeset/199750>
Comment 13 WebKit Commit Bot 2016-04-19 18:19:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2016-04-19 19:08:13 PDT
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.
Comment 15 Chris Dumez 2016-04-19 19:08:52 PDT
(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.
Comment 16 Brady Eidson 2016-04-19 19:09:45 PDT
(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.
Comment 17 Chris Dumez 2016-04-19 19:09:59 PDT
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)
Comment 18 Brady Eidson 2016-04-19 19:12:57 PDT
(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
Comment 19 Chris Dumez 2016-04-19 19:14:16 PDT
(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 :)
Comment 20 Brady Eidson 2016-04-20 09:31:05 PDT
http://trac.webkit.org/changeset/199774
Comment 21 Brady Eidson 2016-04-20 09:31:15 PDT
(In reply to comment #20)
> http://trac.webkit.org/changeset/199774

(That was the followup)