[webkit-reviews] review canceled: [Bug 211043] IndexedDB: Support IDBFactory databases method : [Attachment 400552] Patch

[bugzilla-daemon at webkit.org]

Darryl Pogue <dvpdiner2 at gmail.com> has canceled Darryl Pogue
<dvpdiner2 at gmail.com>'s request for review:
Bug 211043: IndexedDB: Support IDBFactory databases method
https://bugs.webkit.org/show_bug.cgi?id=211043

Attachment 400552: Patch

https://bugs.webkit.org/attachment.cgi?id=400552&action=review




--- Comment #14 from Darryl Pogue <dvpdiner2 at gmail.com> ---
Comment on attachment 400552
  --> https://bugs.webkit.org/attachment.cgi?id=400552
Patch

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

Thanks for the review! I'll address a bunch of those suggestions/cleanups.

>> Source/WebCore/ChangeLog:10
>> +	    `IDBFactory.prototype.databases()`.
> 
> Let's add a reference to
https://w3c.github.io/IndexedDB/#dom-idbfactory-databases.
> I see that this is not supported yet in Firefox, do you know what their plan
is?

Firefox bug is https://bugzilla.mozilla.org/show_bug.cgi?id=934640
It looks like there's been some interest, but no substantial progress.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:72
>> +	void performCallbackOnOriginThread(T& object, void
(T::*method)(Parameters...), Arguments&&... arguments)
> 
> Ditto for one liner.
> You could move it out of of the class declaration but keep it in the header
file as well.
> 
> These two methods seem similar to IDBActiveDOMObject. Should we make
IDBDatabaseNameAndVersionRequest derive from it instead of ActiveDOMObject?

That was my original plan, but IDBActiveDOMObject has an assertion that the
ScriptExecutionContext is not null, and when calling getDatabaseNames for the
Web Inspector we don't have a ScriptExecutionContext. :(

>> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:164
>> +	m_connectionProxy->getAllDatabaseNamesAndVersions(*request);
> 
> Ideally, we would directly write it as something like:
> 
> m_connectionProxy->getAllDatabaseNamesAndVersions(context, [promise =
WTFMove(promise)](auto&& result) {
>     promise.resolve(...);
> });
> 
> And it would be up to m_connectionProxy to isolate the complexity.
> It might indeed have to create an IDBDatabaseNameAndVersionRequest.

getAllDatabaseNames (used by the Web Inspector) complicates this because it is
called without context, taking in the origins as arguments.

>>
LayoutTests/imported/w3c/web-platform-tests/IndexedDB/get-databases.any-expecte
d.txt:6
>> +FAIL Ensure that databases() doesn't pick up changes that haven't commited.
assert_equals: The result of databases() should be only those databases which
have been created at the time of calling, regardless of versionchange
transactions currently running. expected 1 but got 2
> 
> Nice to see some PASS.
> Do you know why the last test fails?

This test fails because we get our name/version pairs by looping over the
databases that exist on the filesystem, and I'm not aware of a way to check
whether versionchange has fired for a given database. Happy to hear any
suggestions.