Bug 229925 – Add basic support for Storage API

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 437375 [details]
Patch

[Sihui Liu]

Created attachment 437378 [details]
Patch

[Sihui Liu]

Created attachment 437379 [details]
Patch

[Sihui Liu]

Created attachment 437382 [details]
Patch

[Sihui Liu]

Created attachment 437383 [details]
Patch

[Sihui Liu]

Created attachment 437384 [details]
Patch

[Sihui Liu]

Created attachment 437393 [details]
Patch

[Sihui Liu]

Created attachment 437424 [details]
Patch

[Sihui Liu]

Created attachment 437436 [details]
Patch

[Sihui Liu]

Created attachment 437438 [details]
Patch

[Sihui Liu]

Created attachment 437459 [details]
Patch

[Sihui Liu]

Created attachment 437540 [details]
Patch

[Darin Adler]

Comment on attachment 437540 [details]
Patch

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

> Source/WebCore/Modules/storage/StorageConnection.h:37
> +    virtual ~StorageConnection() { }

Sometimes "{ }" but other times "= default". Is there a rationale for the different choices?

> Source/WebCore/Modules/storage/StorageManager.cpp:56
> +    return connection->persisted(context->clientOrigin(), [promise = WTFMove(promise), connection](bool persisted) mutable {
> +        promise.resolve(persisted);
> +    });

I find the captured "connection" here to be oblique. Is it necessary to capture connection even though we don’t use it? What would go wrong if we didn’t capture it? We are copying it. Does that create a performance problem?

> Source/WebCore/Modules/storage/StorageManager.cpp:68
> +    return connection->persist(context->clientOrigin(), [promise = WTFMove(promise), connection](bool persisted) mutable {
> +        promise.resolve(persisted);
> +    });

Ditto.

> Source/WebCore/Modules/storage/StorageProvider.h:38
> +class StorageProvider {
> +    WTF_MAKE_FAST_ALLOCATED;
> +public:
> +    StorageProvider() = default;
> +    virtual ~StorageProvider() = default;
> +    virtual StorageConnection& storageConnection() = 0;
> +};

Seem excessive that this is an object instead of just a function.

> Source/WebCore/Modules/storage/StorageProvider.h:40
> +class DummyStorageProvider final : public StorageProvider {

Normally we Ould not put this in StorageProvider.h.

[Sihui Liu]

Comment on attachment 437540 [details]
Patch

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

>> Source/WebCore/Modules/storage/StorageConnection.h:37
>> +    virtual ~StorageConnection() { }
> 
> Sometimes "{ }" but other times "= default". Is there a rationale for the different choices?

Nope, will change this to =default.

>> Source/WebCore/Modules/storage/StorageManager.cpp:56
>> +    });
> 
> I find the captured "connection" here to be oblique. Is it necessary to capture connection even though we don’t use it? What would go wrong if we didn’t capture it? We are copying it. Does that create a performance problem?

I tried to protect the connection, but it seems not necessary as WebStorageProvider holds a reference to connection. I will remove it.

>> Source/WebCore/Modules/storage/StorageManager.cpp:68
>> +    });
> 
> Ditto.

Will remove.

>> Source/WebCore/Modules/storage/StorageProvider.h:38
>> +};
> 
> Seem excessive that this is an object instead of just a function.

It seems we usually have a Provider class (as many parameters in PageConfiguration) to get or create connections

>> Source/WebCore/Modules/storage/StorageProvider.h:40
>> +class DummyStorageProvider final : public StorageProvider {
> 
> Normally we Ould not put this in StorageProvider.h.

Will move it to its own file.

[Sihui Liu]

Created attachment 437569 [details]
Patch

[Sihui Liu]

Created attachment 437570 [details]
Patch

[Sihui Liu]

Created attachment 437571 [details]
Patch

[Darin Adler]

Comment on attachment 437570 [details]
Patch

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

> Source/WebCore/Modules/storage/StorageManager.cpp:54
> +    auto connection = context->storageConnection();
> +    return connection->persisted(context->clientOrigin(), [promise = WTFMove(promise)](bool persisted) mutable {

Don’t really need a local variable here any more, although the line would get a little longer.

> Source/WebCore/Modules/storage/StorageManager.cpp:66
> +    auto connection = context->storageConnection();
> +    return connection->persist(context->clientOrigin(), [promise = WTFMove(promise)](bool persisted) mutable {

Ditto.

> Source/WebCore/Modules/storage/StorageManager.h:34
> +template<typename IDLType> class DOMPromiseDeferred;

I think the typename IDLType here is OK, but I’ll just mention for future references that a name is not needed for a forward declaration.

> Source/WebCore/dom/ScriptExecutionContext.h:147
> +    virtual ClientOrigin clientOrigin() const
> +    {
> +        auto* origin = securityOrigin();
> +        return { topOrigin().data(), origin ? origin->data() : SecurityOriginData { } };
> +    }

I’d prefer that a function body like this be below the class definition rather than including it. Helps us keep interface (list of members functions) separate from implementation (the bodies of those functions).

Also not sure this needs to be in the header file since it’s unlikely to be inlined since it’s a virtual function.

But why is it a virtual function? I don’t see it overridden anywhere. Does this really need to be a member of ScriptExecutionContext at all? Can we just put this code in StorageManager.cpp? It can still be a function, a non-member function.

> Source/WebKit/CMakeLists.txt:29
>      "${WEBKIT_DIR}/NetworkProcess/ServiceWorker"
> +    "${WEBKIT_DIR}/NetworkProcess/storage"
>      "${WEBKIT_DIR}/NetworkProcess/WebStorage"

We normally use case-sensitive sorting, as in the sort tool, rather than case-folded sorting.

> Source/WebKit/CMakeLists.txt:175
>      NetworkProcess/ServiceWorker/WebSWServerToContextConnection
>  
> +    NetworkProcess/storage/NetworkStorageManager
> +
>      NetworkProcess/WebStorage/StorageManagerSet

We normally use case-sensitive sorting, as in the sort tool, rather than case-folded sorting.

> Source/WebKit/DerivedSources.make:40
>      $(WebKit2)/NetworkProcess/ServiceWorker \
> +	$(WebKit2)/NetworkProcess/storage \
>      $(WebKit2)/NetworkProcess/WebStorage \

We normally use case-sensitive sorting, as in the sort tool, rather than case-folded sorting.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:41
> +        m_defaultBucket = makeUnique<StorageBucket>(m_path, "default");

Should be "default"_s to get the ASCIILiteral optimization. Could go even further if we wanted to avoid allocating a string every time.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:33
> +    explicit OriginStorageManager(const String& path);

Could save a little bit of reference count churn by taking a String&& instead.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:54
> +    class StorageBucket {
> +        WTF_MAKE_FAST_ALLOCATED;
> +    public:
> +        StorageBucket(const String& rootPath, const String& identifier)
> +            : m_rootPath(rootPath)
> +            , m_identifier(identifier)
> +        {
> +        }
> +        StorageBucketMode mode() const { return m_mode; }
> +        void setMode(StorageBucketMode mode) { m_mode = mode; }
> +    private:
> +        String m_rootPath;
> +        String m_identifier;
> +        StorageBucketMode m_mode { StorageBucketMode::BestEffort };
> +    };

This class doesn’t need to be in the header file. Not sure why we chose to put it on the heap and in a unique_ptr, but because we did, then we can just make the destructor not inlined, and put the entire class in the .cpp file. It can still be a member of the OriginStorageManager class.

[Sihui Liu]

Comment on attachment 437570 [details]
Patch

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

>> Source/WebCore/Modules/storage/StorageManager.cpp:54
>> +    return connection->persisted(context->clientOrigin(), [promise = WTFMove(promise)](bool persisted) mutable {
> 
> Don’t really need a local variable here any more, although the line would get a little longer.

Will remove the local variable.

>> Source/WebCore/Modules/storage/StorageManager.h:34
>> +template<typename IDLType> class DOMPromiseDeferred;
> 
> I think the typename IDLType here is OK, but I’ll just mention for future references that a name is not needed for a forward declaration.

Got it.

>> Source/WebCore/dom/ScriptExecutionContext.h:147
>> +    }
> 
> I’d prefer that a function body like this be below the class definition rather than including it. Helps us keep interface (list of members functions) separate from implementation (the bodies of those functions).
> 
> Also not sure this needs to be in the header file since it’s unlikely to be inlined since it’s a virtual function.
> 
> But why is it a virtual function? I don’t see it overridden anywhere. Does this really need to be a member of ScriptExecutionContext at all? Can we just put this code in StorageManager.cpp? It can still be a function, a non-member function.

I remembered there are other cases where I wanted to get ClientOrigin from ScripExecutionContext, but it's not necessarily related to this patch, so I will move the function to StorageManager.cpp.

>> Source/WebKit/CMakeLists.txt:29
>>      "${WEBKIT_DIR}/NetworkProcess/WebStorage"
> 
> We normally use case-sensitive sorting, as in the sort tool, rather than case-folded sorting.

Will update.

>> Source/WebKit/CMakeLists.txt:175
>>      NetworkProcess/WebStorage/StorageManagerSet
> 
> We normally use case-sensitive sorting, as in the sort tool, rather than case-folded sorting.

Will update.

>> Source/WebKit/DerivedSources.make:40
>>      $(WebKit2)/NetworkProcess/WebStorage \
> 
> We normally use case-sensitive sorting, as in the sort tool, rather than case-folded sorting.

Will update. (This file seems to not use sorting webrtc/ is before IndexedDB/ above...)

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:41
>> +        m_defaultBucket = makeUnique<StorageBucket>(m_path, "default");
> 
> Should be "default"_s to get the ASCIILiteral optimization. Could go even further if we wanted to avoid allocating a string every time.

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:33
>> +    explicit OriginStorageManager(const String& path);
> 
> Could save a little bit of reference count churn by taking a String&& instead.

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:54
>> +    };
> 
> This class doesn’t need to be in the header file. Not sure why we chose to put it on the heap and in a unique_ptr, but because we did, then we can just make the destructor not inlined, and put the entire class in the .cpp file. It can still be a member of the OriginStorageManager class.

The bucket can be cleared according to spec, so we put it on a heap.
Will move this class to .cpp.

[Sihui Liu]

Created attachment 437575 [details]
Patch

[Darin Adler]

Comment on attachment 437575 [details]
Patch

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

> Source/WebCore/Modules/storage/StorageManager.cpp:63
> +    return context->storageConnection()->persisted(clientOrigin(*context), [promise = WTFMove(promise)](bool persisted) mutable {
> +        promise.resolve(persisted);
> +    });

Just realized: what guarantees storeConnection() won’t return nullptr?

> Source/WebCore/Modules/storage/StorageManager.cpp:74
> +    return context->storageConnection()->persist(clientOrigin(*context), [promise = WTFMove(promise)](bool persisted) mutable {
> +        promise.resolve(persisted);
> +    });

Ditto.

> Source/WebCore/Modules/storage/StorageProvider.h:28
> +#include "StorageConnection.h"

Seems this file only needs a forward declaration of StorageConnection, not to include the header.

> Source/WebCore/Modules/storage/StorageProvider.h:33
> +    WTF_MAKE_FAST_ALLOCATED;

Not sure you need this for an abstract base class. Since you can’t allocate one at all. Pretty sure we can leave it out.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:406
> +    auto iter = m_storageManagers.find(sessionID);
> +    if (iter != m_storageManagers.end())
> +        iter->value->startReceivingMessageFromConnection(connection.connection());

No need to use an iterator for this:

    if (auto manager = m_storageManagers.get(sessionID))
        manager->startReceivingMessageFromConnection(connection.connection());

If we did need an iterator I wouldn’t want it named "iter" given WebKit coding style traditions.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:461
> +    auto isNewEntry = m_storageManagers.ensure(sessionID, [sessionID, &generalStoragePath] {
> +        return NetworkStorageManager::create(sessionID, generalStoragePath);
> +    }).isNewEntry;
> +    if (isNewEntry)
> +        SandboxExtension::consumePermanently(generalStoragePathHandle);

Can achieve this by putting the call inside the lambda, no need to do this after the fact:

    m_storageManagers.ensure(sessionID, [&] {
        SandboxExtension::consumePermanently(generalStoragePathHandle);;
        return NetworkStorageManager::create(sessionID, generalStoragePath);
    });

Or if the ordering matters:

    m_storageManagers.ensure(sessionID, [&] {
        auto manager = NetworkStorageManager::create(sessionID, generalStoragePath);
        SandboxExtension::consumePermanently(generalStoragePathHandle);
        return manager;
    });

But also is it correct that the function does nothing at all if there is already a storage manager? Do we have tests that cover that state?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2699
> +    auto iter = m_storageManagers.find(sessionID);
> +    if (iter != m_storageManagers.end())
> +        iter->value->stopReceivingMessageFromConnection(connection);

No need to use an iterator for this:

    if (auto manager = m_storageManagers.get(sessionID))
        manager->stopReceivingMessageFromConnection(connection);

If we did need an iterator I wouldn’t want it named "iter" given WebKit coding style traditions.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:73
> +    auto utf8String = string.utf8();

Slightly inelegant for us to make a String that we know will be all ASCII out of a security origin (although maybe this is not true for IDNA, so that invalidates my whole comment) which will already be stored as 8-bit characters, then "convert it to UTF-8" just to convert it back to 8-bit characters, allocating the C string on the heap and null-terminating it for no reason, just so we can feed the bytes into the SHA algorithm, but I guess I don’t have a better idea.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:64
> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<OriginStorageManager>> m_localOriginStorageManagers;
> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<OriginStorageManager>> m_sessionOriginStorageManagers;

Given that each OriginStorageManager is a small object, not much larger than a pointer, not sure we need you use unique_ptr here for the map entries. How about just putting OriginStorageManager objects in there?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:31
> +class OriginStorageManager::StorageBucket {

This seems more like a structure than a class. Do we really need a class for this?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:40
> +    enum class StorageBucketMode : uint8_t { BestEffort, Persistent };

Seems like this doesn’t need to be in the header; not used in the class definition. Also could use bool instead of uint8_t.

> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:274
> +WTF::String WebsiteDataStore::defaultGeneralStorageDirectory()

No idea why this file uses WTF::String instead of String everywhere.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2093
> +    auto generalStorageDirectory = this->generalStorageDirectory();
> +    if (!generalStorageDirectory.isEmpty()) {

In latest C++ we can scope the variable like this:

    if (auto generalStorageDirectory = this->generalStorageDirectory(); !generalStorageDirectory.isEmpty()) {

Neat, isn’t it? Also, since it’s scoped we can use a shorter name like we do for handle, so I would write:

    if (auto directory = generalStorageDirectory(); !directory.isEmpty()) {
        parameters.generalStorageDirectory = directory;
        if (auto handle = SandboxExtension::createHandleForReadWriteDirectory(directory))
            parameters.generalStorageDirectoryHandle = WTFMove(*handle);
    }

[Sihui Liu]

Comment on attachment 437575 [details]
Patch

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

>> Source/WebCore/Modules/storage/StorageManager.cpp:63
>> +    });
> 
> Just realized: what guarantees storeConnection() won’t return nullptr?

ah nice catch! Will add a null check.

>> Source/WebCore/Modules/storage/StorageProvider.h:28
>> +#include "StorageConnection.h"
> 
> Seems this file only needs a forward declaration of StorageConnection, not to include the header.

Will remove.

>> Source/WebCore/Modules/storage/StorageProvider.h:33
>> +    WTF_MAKE_FAST_ALLOCATED;
> 
> Not sure you need this for an abstract base class. Since you can’t allocate one at all. Pretty sure we can leave it out.

Will remove.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:406
>> +        iter->value->startReceivingMessageFromConnection(connection.connection());
> 
> No need to use an iterator for this:
> 
>     if (auto manager = m_storageManagers.get(sessionID))
>         manager->startReceivingMessageFromConnection(connection.connection());
> 
> If we did need an iterator I wouldn’t want it named "iter" given WebKit coding style traditions.

Got it. Will use get() instead.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:461
>> +        SandboxExtension::consumePermanently(generalStoragePathHandle);
> 
> Can achieve this by putting the call inside the lambda, no need to do this after the fact:
> 
>     m_storageManagers.ensure(sessionID, [&] {
>         SandboxExtension::consumePermanently(generalStoragePathHandle);;
>         return NetworkStorageManager::create(sessionID, generalStoragePath);
>     });
> 
> Or if the ordering matters:
> 
>     m_storageManagers.ensure(sessionID, [&] {
>         auto manager = NetworkStorageManager::create(sessionID, generalStoragePath);
>         SandboxExtension::consumePermanently(generalStoragePathHandle);
>         return manager;
>     });
> 
> But also is it correct that the function does nothing at all if there is already a storage manager? Do we have tests that cover that state?

It is correct that we don't need to consume the handle if there is an existing manager. This is align with other managers (added in NetworkProcess::addWebsiteDataStore).
I am not sure if there is a test for this (session is added again when there is an existing one); seems like a rare case. It looks like NetworkProcessProxy::addSession already checks if the session exists before sending NetworkProcess::AddWebsiteDataStore message.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2699
>> +        iter->value->stopReceivingMessageFromConnection(connection);
> 
> No need to use an iterator for this:
> 
>     if (auto manager = m_storageManagers.get(sessionID))
>         manager->stopReceivingMessageFromConnection(connection);
> 
> If we did need an iterator I wouldn’t want it named "iter" given WebKit coding style traditions.

Will update.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:73
>> +    auto utf8String = string.utf8();
> 
> Slightly inelegant for us to make a String that we know will be all ASCII out of a security origin (although maybe this is not true for IDNA, so that invalidates my whole comment) which will already be stored as 8-bit characters, then "convert it to UTF-8" just to convert it back to 8-bit characters, allocating the C string on the heap and null-terminating it for no reason, just so we can feed the bytes into the SHA algorithm, but I guess I don’t have a better idea.

I only thought about converting string to byte here. IDNA is a good point that we may want to keep current implementation.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:64
>> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<OriginStorageManager>> m_sessionOriginStorageManagers;
> 
> Given that each OriginStorageManager is a small object, not much larger than a pointer, not sure we need you use unique_ptr here for the map entries. How about just putting OriginStorageManager objects in there?

Each OriginStorageManager is small for now, as it does not actually maintain any data. It will likely own some storage backing map as the implementation goes on.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:31
>> +class OriginStorageManager::StorageBucket {
> 
> This seems more like a structure than a class. Do we really need a class for this?

Yes, will extend it to own some StorageManagers (for different storage types) later.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:40
>> +    enum class StorageBucketMode : uint8_t { BestEffort, Persistent };
> 
> Seems like this doesn’t need to be in the header; not used in the class definition. Also could use bool instead of uint8_t.

Sure, will move.

>> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:274
>> +WTF::String WebsiteDataStore::defaultGeneralStorageDirectory()
> 
> No idea why this file uses WTF::String instead of String everywhere.

ha right. I will remove the WTF:: here.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2093
>> +    if (!generalStorageDirectory.isEmpty()) {
> 
> In latest C++ we can scope the variable like this:
> 
>     if (auto generalStorageDirectory = this->generalStorageDirectory(); !generalStorageDirectory.isEmpty()) {
> 
> Neat, isn’t it? Also, since it’s scoped we can use a shorter name like we do for handle, so I would write:
> 
>     if (auto directory = generalStorageDirectory(); !directory.isEmpty()) {
>         parameters.generalStorageDirectory = directory;
>         if (auto handle = SandboxExtension::createHandleForReadWriteDirectory(directory))
>             parameters.generalStorageDirectoryHandle = WTFMove(*handle);
>     }

Cool! Seems like we can clean up some code.

[Sihui Liu]

Created attachment 437594 [details]
Patch for landing

[EWS]

Committed r282130 (241427@main): <https://commits.webkit.org/241427@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/82858170>

[youenn fablet]

Comment on attachment 437594 [details]
Patch for landing

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

> Source/WebCore/Modules/storage/StorageManager.cpp:52
> +    return { context.topOrigin().data(), origin ? origin->data() : SecurityOriginData { } };

Having origin null here is probably not great.
Probably it is not null, but if it can be null, we should probably stop processing.

> Source/WebCore/Modules/storage/StorageManager.cpp:67
> +    return promise.reject(Exception { InvalidStateError, "The connection is invalid"_s });

We tend to write error cases first.
Something like:
auto* connection = ...
if (!connection) {
   promise.reject(Exception { InvalidStateError, "The connection is invalid"_s });
   return;
}

> Source/WebCore/Modules/storage/StorageManager.h:46
> +    NavigatorBase& m_navigator;

This is unsafe, StorageManager is RefCounted so could outlive its m_navigator.
One solution is to no longer StorageManager be RefCounted and add StorageManager::ref and StorageManager::deref implementations calling m_navigator.ref()/m_navigator.deref().
See ServiceWorkerContainer for this pattern.

Another option would be to use a WeakPtr<NavigatorBase>

> Source/WebCore/dom/ScriptExecutionContext.h:121
> +    virtual RefPtr<StorageConnection> storageConnection() { return nullptr; }

Can we return a StorageConnection* to reduce count churn?
It does not seem like storageConnection() callers will make use of a RefPtr<>.

> Source/WebCore/page/NavigatorBase.cpp:143
> +ExceptionOr<StorageManager&> NavigatorBase::storage()

Why not returning StorageManager& directly?

> Source/WebCore/page/NavigatorStorage.idl:32
> +    [SameObject] readonly attribute StorageManager storage;

It does not seem like we can throw an exception for that getter.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:94
> +    return *m_localOriginStorageManagers.ensure(origin, [path = m_path, origin, salt = m_salt] {

We could remove some count churn by using &:
[this, &origin] { // or just [&]
     return makeUnique<OriginStorageManager>(originPath(m_path, origin, m_salt));
}

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:95
> +        return makeUnique<OriginStorageManager>(originPath(path, origin, salt));

makeUniqueRef?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:64
> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<OriginStorageManager>> m_sessionOriginStorageManagers;

UniqueRef<OriginStorageManager>?

> Source/WebKit/Shared/WebsiteDataStoreParameters.cpp:145
> +    parameters.generalStorageDirectoryHandle = WTFMove(*generalStorageDirectoryHandle);

I am not totally clear what the plan is for this.
It would be great if you are planning to migrate all storage API data (IDB, SW, Cache API) to generalStorageDirectory, each storage API having a subfolder for it.
If so, I would rename generalStorageDirectoryHandle to storageAPIsDirectory. I would beef-up a bit the change log as well to describe its purpose.

Ditto for generalStorageDirectoryHandle and derived names below.

[Sihui Liu]

Comment on attachment 437594 [details]
Patch for landing

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

Thanks for the comments. I will make a follow-up on https://bugs.webkit.org/show_bug.cgi?id=230059.

>> Source/WebCore/Modules/storage/StorageManager.cpp:52
>> +    return { context.topOrigin().data(), origin ? origin->data() : SecurityOriginData { } };
> 
> Having origin null here is probably not great.
> Probably it is not null, but if it can be null, we should probably stop processing.

Okay, will add a null check for origin.

>> Source/WebCore/Modules/storage/StorageManager.cpp:67
>> +    return promise.reject(Exception { InvalidStateError, "The connection is invalid"_s });
> 
> We tend to write error cases first.
> Something like:
> auto* connection = ...
> if (!connection) {
>    promise.reject(Exception { InvalidStateError, "The connection is invalid"_s });
>    return;
> }

Sure, will change the order.

>> Source/WebCore/Modules/storage/StorageManager.h:46
>> +    NavigatorBase& m_navigator;
> 
> This is unsafe, StorageManager is RefCounted so could outlive its m_navigator.
> One solution is to no longer StorageManager be RefCounted and add StorageManager::ref and StorageManager::deref implementations calling m_navigator.ref()/m_navigator.deref().
> See ServiceWorkerContainer for this pattern.
> 
> Another option would be to use a WeakPtr<NavigatorBase>

WeakPtr may be enough. If navigator is gone, it seems Okay to not make StorageManage numb.

>> Source/WebCore/dom/ScriptExecutionContext.h:121
>> +    virtual RefPtr<StorageConnection> storageConnection() { return nullptr; }
> 
> Can we return a StorageConnection* to reduce count churn?
> It does not seem like storageConnection() callers will make use of a RefPtr<>.

Will change to StorageConnection*.

>> Source/WebCore/page/NavigatorBase.cpp:143
>> +ExceptionOr<StorageManager&> NavigatorBase::storage()
> 
> Why not returning StorageManager& directly?

Will change to StorageManager&

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:94
>> +    return *m_localOriginStorageManagers.ensure(origin, [path = m_path, origin, salt = m_salt] {
> 
> We could remove some count churn by using &:
> [this, &origin] { // or just [&]
>      return makeUnique<OriginStorageManager>(originPath(m_path, origin, m_salt));
> }

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:95
>> +        return makeUnique<OriginStorageManager>(originPath(path, origin, salt));
> 
> makeUniqueRef?

Compile error:
/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashTraits.h:77:45: error: calling a private constructor of class 'WTF::UniqueRef<WebKit::OriginStorageManager>'
        new (NotNull, std::addressof(slot)) T(Traits::emptyValue());
                                            ^
/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/HashTraits.h:364:31: note: in instantiation of function template specialization 'WTF::GenericHashTraits<WTF::UniqueRef<WebKit::OriginStorageManager>>::constructEmptyValue<WTF::HashTraits<WTF::UniqueRef<WebKit::OriginStorageManager>>>' requested here
        ValueTraits::template constructEmptyValue<ValueTraits>(slot.value);

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:64
>> +    HashMap<WebCore::ClientOrigin, std::unique_ptr<OriginStorageManager>> m_sessionOriginStorageManagers;
> 
> UniqueRef<OriginStorageManager>?

Compile error above.

>> Source/WebKit/Shared/WebsiteDataStoreParameters.cpp:145
>> +    parameters.generalStorageDirectoryHandle = WTFMove(*generalStorageDirectoryHandle);
> 
> I am not totally clear what the plan is for this.
> It would be great if you are planning to migrate all storage API data (IDB, SW, Cache API) to generalStorageDirectory, each storage API having a subfolder for it.
> If so, I would rename generalStorageDirectoryHandle to storageAPIsDirectory. I would beef-up a bit the change log as well to describe its purpose.
> 
> Ditto for generalStorageDirectoryHandle and derived names below.

Yes, I think we should move them to the same directory, so it's more align with the structure of current storage standard. 

Maybe not only Storage APIs, as https://storage.spec.whatwg.org/#infrastructure mentions APIs like notifications and maybe permissions. "general" here is more like we don't have a specific directory for this so let's put it here.

Maybe we can rename it to just StorageDirectory or APIStorageDirectory later.