Bug 211043 – IndexedDB: Support IDBFactory databases method

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darryl Pogue]

Link to spec: https://w3c.github.io/IndexedDB/#dom-idbfactory-databases

[Radar WebKit Bug Importer]

<rdar://problem/62402059>

[Darryl Pogue]

I am working on a patch for this :)

[Darryl Pogue]

Created attachment 398299 [details]
Patch

[Darryl Pogue]

There remains some work to do in IDBFactory for web workers to resolve the promise on the correct thread. I'd love suggestions on how best to do that, and any suggested improvements to the rest of the changes.

[Darryl Pogue]

Created attachment 398366 [details]
Patch

[Darin Adler]

Comment on attachment 398366 [details]
Patch

Next step would be to investigate and fix the failing tests seen on EWS and post a new patch.

[Sihui Liu]

(In reply to Darryl Pogue from comment #4)
> There remains some work to do in IDBFactory for web workers to resolve the
> promise on the correct thread. I'd love suggestions on how best to do that,
> and any suggested improvements to the rest of the changes.

Probably remember the ScriptExecutionContext with the callback and invoke the the callback on the context's thread. You can subclass IDBActiveDomObject, which has the needed functions. Then replace the class in m_databaseInfoCallbacks. Also remember to do proper cleanup of the map for situations like connection is lost, see IDBConnectionProxy::forgetActivityForCurrentThread.

Again, for the rest of the change, I suggest to merge your IDBConnectionProxy::getAllDatabaseNamesAndVersions with the existing IDBConnectionProxy::getAllDatabaseNames (You can rename it to getAllDatabaseNamesAndVersions). Current getAllDatabaseNamesAndVersions is basically the same as IDBConnectionProxy::getAllDatabaseNames on the server side but including the version number in results. You can throw away the version info in result or use a parameter to indicate whether version is needed in the function.

[Darryl Pogue]

Created attachment 400412 [details]
Patch

[Darryl Pogue]

Created attachment 400418 [details]
Patch

[Darryl Pogue]

Created attachment 400552 [details]
Patch

[Darryl Pogue]

This latest patch should address the feedback and sends both getAllDatabaseNames and getAllDatabaseNamesAndVersions through the same code path, as well as handling the threading properly for web workers.

Note that this patch exposes new web-facing API (IDBFactory.prototype.databases) and I think there's an additional review step involved?

[Maciej Stachowiak]

The new web-facing API is a minor addition and clearly per-spec according to the test cases, so seems fine. Unfortunately, I don't know enough about IndexedDB internals to review this.

[youenn fablet]

Comment on attachment 400552 [details]
Patch

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

> 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?

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.cpp:62
> +    m_callback = WTFMove(callback);

We could probably add an ASSERT(!callback);

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:42
> +

two lines

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:56
> +

Two lines

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:57
> +    virtual ~IDBDatabaseNameAndVersionRequest();

No need for virtual

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:63
> +    void contextDestroyed() final

We usually try to only have one liners here.
You can probably put it in the source file.

> 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?

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:156
> +    RefPtr<IDBDatabaseNameAndVersionRequest> request = IDBDatabaseNameAndVersionRequest::create(&context, m_connectionProxy, origin);

auto

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:158
> +    request->setCallback([promise = WTFMove(promise)](const Vector<IDBDatabaseNameAndVersion>& result) mutable {

auto&

> 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.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:504
> +        ASSERT(!m_databaseInfoCallbacks.contains(request.resourceIdentifier()));

The id value could probably be generated by IDBConnectionProxy.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:511
> +void IDBConnectionProxy::didGetAllDatabaseNamesAndVersions(const IDBResourceIdentifier& requestIdentifier, const Vector<IDBDatabaseNameAndVersion>& databases)

We can pass requestIdentifier by value. Ditto for other methods.

We could probably have a Vector<IDBDatabaseNameAndVersion>&& here.
That might allow to reduce count churning and optimise isolated copy to not have to reallocate strings when hopping to another thread.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:519
> +    request->performCallbackOnOriginThread(*request, &IDBDatabaseNameAndVersionRequest::fireCallback, databases);

We should probably check that request is not null here.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h:175
> +    HashMap<IDBResourceIdentifier, RefPtr<IDBDatabaseNameAndVersionRequest>> m_databaseInfoCallbacks;

s/RefPtr/Ref

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:31
> +#include "IDBDatabaseNameAndVersion.h"

We can probably forward declare IDBDatabaseNameAndVersion.

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClientDelegate.h:30
> +#include "IDBDatabaseNameAndVersion.h"

Ditto

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:522
> +            databases.append(databaseTuple);

WTFMove

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:530
> +            databases.append(databaseTuple);

WTFMove.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:915
> +IDBDatabaseNameAndVersion SQLiteIDBBackingStore::databaseNameAndVersionFromFile(const String& databasePath)

It might be better to return an Optional<IDBDatabaseNameAndVersion>.
That will make it clearer whether there is a database or not.

> LayoutTests/imported/w3c/web-platform-tests/IndexedDB/get-databases.any-expected.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?

[Darryl Pogue]

Comment on attachment 400552 [details]
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-expected.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.

[youenn fablet]

> 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. :(

Web Inspector is probably calling this method for a particular Document which is a ScriptExecutionContext. Not sure about service worker.

[Darryl Pogue]

Comment on attachment 400552 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:504
>> +        ASSERT(!m_databaseInfoCallbacks.contains(request.resourceIdentifier()));
> 
> The id value could probably be generated by IDBConnectionProxy.

You're suggesting using a uint64_t identifier instead of an IDBResourceIdentifier?

>> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h:175
>> +    HashMap<IDBResourceIdentifier, RefPtr<IDBDatabaseNameAndVersionRequest>> m_databaseInfoCallbacks;
> 
> s/RefPtr/Ref

Can you explain the rationale behind changing this to Ref? All the other HashMaps here are using RefPtr, and my attempts to change it to Ref are resulting in a bunch of compiler errors about assignments and `=`

[Darin Adler]

Any place where a value is always non-null should use Ref rather than a RefPtr to clearly communicate a null value is not allowed, just as we use & rather than * throughout WebKit for the same reason. Typically there are a few relatively straightforward changes needed, but occasionally we run into things that just can't easily be made to work without nullptr. From the phrase alone, it's not clear if "a bunch of compiler errors" is what's normally expected or if you are running into a more serious problem.

[youenn fablet]

RefPtr is copiable while Ref is not. WTFMove() or copyRef() might help removing some of these compilation issues.

[Darin Adler]

(In reply to youenn fablet from comment #18)
> RefPtr is copiable while Ref is not. WTFMove() or copyRef() might help
> removing some of these compilation issues.

That was true before r261467, but it is no longer the case.

[Sihui Liu]

Comment on attachment 400552 [details]
Patch

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

Thanks for updates on the patch.

>>> 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. :(

WebInspector should have a ScriptExecutionContext or at least have access to one (otherwise it will not be able to inspect database data I think). 
InspectorIndexedDBAgent seems to confirm that. In InspectorIndexedDBAgent it uses document as context, so maybe you can use that document too.

>> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:158
>> +    request->setCallback([promise = WTFMove(promise)](const Vector<IDBDatabaseNameAndVersion>& result) mutable {
> 
> auto&

Maybe change const Vector<IDBDatabaseNameAndVersion>& result to Optional<Vector<IDBDatabaseNameAndVersion>> so that you can reject the promise when result is null in cases like connection is lost or database errors on server side. 
As in spec:
If this cannot be determined for any reason, then reject p with an appropriate error (e.g. an "UnknownError" DOMException) and terminate these steps.

>>> 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.

I think it's possible to make getAllDatabaseNames be called with context. WebInspector does access IDBDatabases with document and document is ScriptExecutionContext. Please check InspectorIndexedDBAgent::requestDatabaseNames.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:171
> +    RefPtr<IDBDatabaseNameAndVersionRequest> request = IDBDatabaseNameAndVersionRequest::create(nullptr, m_connectionProxy, origin);

As comment above, maybe try creating IDBDatabaseNameAndVersionRequest with ScriptExecutionContext, which should make thing less complicated.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:617
> +    {
> +        Locker<Lock> lock(m_databaseInfoMapLock);
> +        removeItemsMatchingCurrentThread(m_databaseInfoCallbacks);
> +    }

You probably also need to complete the callbacks in IDBConnectionProxy::connectionToServerLost, which means the request won't be answered.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.h:32
> +#include "IDBDatabaseNameAndVersion.h"

Maybe forward declaration.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:924
> +        database.close();

This is not needed. ~SQLiteDatabase() does the close.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:937
> +        database.close();

close() is not needed.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:941
> +    database.close();

close() is not needed.

[Darryl Pogue]

Created attachment 401650 [details]
Patch

[youenn fablet]

Comment on attachment 401650 [details]
Patch

Good progress.
Some additional comments below.

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.cpp:66
> +void IDBDatabaseNameAndVersionRequest::fireCallback(Optional<Vector<IDBDatabaseNameAndVersion>> databases)

&&

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.cpp:71
> +        callback(databases);

WTFMove()

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:30
> +#include "ClientOrigin.h"

Is this one needed?

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:60
> +    using InfoCallback = Function<void(Optional<Vector<IDBDatabaseNameAndVersion>>)>;

Optional<>&&

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:62
> +    void fireCallback(Optional<Vector<IDBDatabaseNameAndVersion>>);

Optional<>&&.
fireCallback name could be improved. How about complete?.
Also, it seems this callback will always be called, so we could migrate it to a CompletionHandler.

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:67
> +    InfoCallback m_callback;

We usually put the members at the end and the private methods before.

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:75
> +    const char* activeDOMObjectName() const final;

This is fine like this given the current model, but it seems a bit weird to have these methods given this is not exposed to JS.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:139
> +void IDBFactory::databases(ScriptExecutionContext& context, Ref<DeferredPromise>&& deferred)

You can write it as IDBFactory::databases(..., IDBDatabasesResponsePromise&& promise)

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:152
> +    m_connectionProxy->getAllDatabaseNamesAndVersions(context, [promise = WTFMove(promise)](Optional<Vector<IDBDatabaseNameAndVersion>> result) mutable {

s/Optional<>/auto&&

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:154
> +            promise.reject(Exception { UnknownError });

We usually early return:
if (!result) {
    promise.reject(Exception { UnknownError });
    return;
}

This is fine as is but an improvement would be to use something like ExceptionOr<Vector> to allow passing back an exception code.
Ditto below for IDBFactory::getAllDatabaseNames.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:156
> +            promise.resolve(WTF::map(*result, [](const auto& info) {

s/const auto&/auto&&/
and you can move info.name

> Source/WebCore/Modules/indexeddb/IDBFactory.h:70
> +    WEBCORE_EXPORT void getAllDatabaseNames(ScriptExecutionContext&, WTF::Function<void(const Vector<String>&)>&&);

No need for WTF:: anymore I believe.
Given your implementation, you could probably have a Function<void(Vector<String>&&)>, but this could be left as is for now.

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:32
> +#include "IDBDatabaseNameAndVersionRequest.h"

Is it needed?

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:484
> +            request = m_databaseInfoCallbacks.get(requestIdentifier);

This is pre-existing model but it seems to me we should take the request here.
Otherwise we will keep it in the map and call again the request next time the connection is lost.

Ideally we would reuse didGetAllDatabaseNamesAndVersions(requestIdentifier, { }) but this is not straightforward.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:528
> +        request->setCallback(WTFMove(callback));

Given we create the request and have the callback, it seems best to pass it in constructor instead of setting it.
Then we can remove the setter probably.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:529
> +        m_databaseInfoCallbacks.set(request->resourceIdentifier(), request.get());

add is a bit more efficient than set.
If you want, you can add ASSERT(!m_databaseInfoCallbacks.set.contains(request->resourceIdentifier())).

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h:31
> +#include "IDBDatabaseNameAndVersion.h"

Can we forward declare IDBDatabaseNameAndVersion?
Ditto below.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h:120
> +    void getAllDatabaseNamesAndVersions(ScriptExecutionContext&, Function<void(Optional<Vector<IDBDatabaseNameAndVersion>>)>&&);

Function<void(Optional<Vector<IDBDatabaseNameAndVersion>>&&)>

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:503
> +        m_delegate->getAllDatabaseNamesAndVersions(requestIdentifier, origin);

We usually early return.

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:31
> +#include "IDBDatabaseNameAndVersion.h"

Can we forward declare it?

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClientDelegate.h:30
> +#include "IDBDatabaseNameAndVersion.h"

Can we forward declare it?

> Source/WebCore/Modules/indexeddb/shared/IDBDatabaseNameAndVersion.h:39
> +    template<class Decoder> static WARN_UNUSED_RETURN bool decode(Decoder&, IDBDatabaseNameAndVersion&);

The modern way is Optional<IDBDatabaseNameAndVersion> decode(Decoder&)

> Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:210
> +void WebIDBConnectionToServer::getAllDatabaseNamesAndVersions(const IDBResourceIdentifier& requestIdentifier, const WebCore::ClientOrigin& origin)

WebCore:: probably not needed.

> Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:325
> +void WebIDBConnectionToServer::didGetAllDatabaseNamesAndVersions(const IDBResourceIdentifier& requestIdentifier, const Vector<WebCore::IDBDatabaseNameAndVersion>& databases)

You can replace it by a Vector<WebCore::IDBDatabaseNameAndVersion>&&
WebCore:: probably not needed.

> Source/WebKit/WebProcess/Databases/IndexedDB/WebIDBConnectionToServer.cpp:327
> +    m_connectionToServer->didGetAllDatabaseNamesAndVersions(requestIdentifier, databases);

And WTFMove(databases) here.

[Darryl Pogue]

Created attachment 401829 [details]
Patch

[youenn fablet]

Comment on attachment 401829 [details]
Patch

LGTM

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:50
> +    using InfoCallback = CompletionHandler<void(Optional<Vector<IDBDatabaseNameAndVersion>>&&)>;

I take this suggestion back.
There is no guarantee right now that complete will be called, so it is best to change it back to a Function.

> Source/WebCore/Modules/indexeddb/IDBDatabaseNameAndVersionRequest.h:64
> +    bool m_hasPendingActivity { true };

Should be moved with other members.
Given it stays true though, we could probably remove it.

> Source/WebCore/Modules/indexeddb/IDBFactory.h:48
> +class DeferredPromise;

Not needed?

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:482
> +        Optional<Vector<IDBDatabaseNameAndVersion>> result = WTF::nullopt;

By default optional is null, so no need for assignment here.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:483
> +        didGetAllDatabaseNamesAndVersions(requestIdentifier, WTFMove(result));

Can we write it as didGetAllDatabaseNamesAndVersions(requestIdentifier, { });?

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:519
> +        auto req = IDBDatabaseNameAndVersionRequest::create(context, *this, WTFMove(callback));

s/req/newRequest/

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:535
> +        if (!req)

s/req/takenRequest/

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:538
> +        request = (*req).ptr();

request = WTFMove(*req)

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h:54
> +struct IDBDatabaseNameAndVersion;

Might not be needed if IDBDatabaseNameAndVersionRequest.h declares it

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBConnectionToClient.cpp:196
> +void WebIDBConnectionToClient::didGetAllDatabaseNamesAndVersions(const WebCore::IDBResourceIdentifier& requestIdentifier, Vector<WebCore::IDBDatabaseNameAndVersion>&& databases)

We can keep a const & here since that is what Messages::WebIDBConnectionToServer::DidGetAllDatabaseNamesAndVersions is taking

> Source/WebKitLegacy/Storage/InProcessIDBServer.cpp:489
> +    dispatchTaskReply([this, protectedThis = makeRef(*this), requestIdentifier, databases = WTFMove(databases)]() mutable {

It is safer to keep isolatedCopying databases.

[Darryl Pogue]

Comment on attachment 401829 [details]
Patch

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

Thanks so much for working through this with me. I really appreciate the review & feedback!

>> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:483
>> +        didGetAllDatabaseNamesAndVersions(requestIdentifier, WTFMove(result));
> 
> Can we write it as didGetAllDatabaseNamesAndVersions(requestIdentifier, { });?

We can.
Will { } be considered nullopt here or an empty Vector? I think we want the null optional so that the promise will reject.

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBConnectionToClient.cpp:196
>> +void WebIDBConnectionToClient::didGetAllDatabaseNamesAndVersions(const WebCore::IDBResourceIdentifier& requestIdentifier, Vector<WebCore::IDBDatabaseNameAndVersion>&& databases)
> 
> We can keep a const & here since that is what Messages::WebIDBConnectionToServer::DidGetAllDatabaseNamesAndVersions is taking

If I make this const, then WTFMove(databases) fails due to the static_assert: T is const qualified.

[youenn fablet]

> > Can we write it as didGetAllDatabaseNamesAndVersions(requestIdentifier, { });?
> 
> We can.
> Will { } be considered nullopt here or an empty Vector? I think we want the
> null optional so that the promise will reject.

It will be nullopt, so this promise should reject.
You can use WTF::nullopt as well.

Maybe we should have some tests in that area if we do not have already.
As a follow-up, we could write a with testRunner.terminateNetworkProcess, though that might not be easy to make it non racy.
Or we could add a testRunner/internals API to trigger the same code path.

> >> Source/WebKit/NetworkProcess/IndexedDB/WebIDBConnectionToClient.cpp:196
> >> +void WebIDBConnectionToClient::didGetAllDatabaseNamesAndVersions(const WebCore::IDBResourceIdentifier& requestIdentifier, Vector<WebCore::IDBDatabaseNameAndVersion>&& databases)
> > 
> > We can keep a const & here since that is what Messages::WebIDBConnectionToServer::DidGetAllDatabaseNamesAndVersions is taking
> 
> If I make this const, then WTFMove(databases) fails due to the
> static_assert: T is const qualified.

You can remove WTFMove.

[Darryl Pogue]

Created attachment 401899 [details]
Patch

[EWS]

Committed r263079: <https://trac.webkit.org/changeset/263079>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401899 [details].

[Truitt Savell]

Reverted r263079 for reason:

Broke Internal builds.

Committed r263093: <https://trac.webkit.org/changeset/263093>

[youenn fablet]

I'll have a look at the build error tomorrow.

[youenn fablet]

Created attachment 402094 [details]
Patch + build fix

[youenn fablet]

Created attachment 402111 [details]
Patch + build fix

[EWS]

Committed r263157: <https://trac.webkit.org/changeset/263157>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402111 [details].

[Chris Dumez]

Comment on attachment 402111 [details]
Patch + build fix

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

> Source/WebCore/rendering/RenderThemeMac.mm:2813
> +#endif

Why this change? This seems to have broken the build for me..

[youenn fablet]

(In reply to Chris Dumez from comment #34)
> Comment on attachment 402111 [details]
> Patch + build fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402111&action=review
> 
> > Source/WebCore/rendering/RenderThemeMac.mm:2813
> > +#endif
> 
> Why this change? This seems to have broken the build for me..

My mistake, we should revert that change.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 402126 [details]
Build fix

[Chris Dumez]

(In reply to youenn fablet from comment #35)
> (In reply to Chris Dumez from comment #34)
> > Comment on attachment 402111 [details]
> > Patch + build fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=402111&action=review
> > 
> > > Source/WebCore/rendering/RenderThemeMac.mm:2813
> > > +#endif
> > 
> > Why this change? This seems to have broken the build for me..
> 
> My mistake, we should revert that change.

<https://trac.webkit.org/changeset/263163>

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 402126 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Darryl Pogue]

This has been landed, and has shipped in iOS 14 and Safari 14 for macOS.

[Darin Adler]

Does anyone have the revision number for when we landed this?

[Darryl Pogue]

(In reply to Darin Adler from comment #41)
> Does anyone have the revision number for when we landed this?

Looks like r263157: https://trac.webkit.org/changeset/263157/webkit