Bug 234087 – Merge StorageManager with NetworkStorageManager to manage WebStorage by origin

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 446563 [details]
Patch

[Sihui Liu]

Created attachment 446567 [details]
Patch

[Sihui Liu]

Created attachment 446578 [details]
Patch

[Sihui Liu]

Created attachment 446752 [details]
Patch

[Sihui Liu]

Created attachment 446756 [details]
Patch

[Sihui Liu]

Created attachment 446762 [details]
Patch

[Chris Dumez]

Comment on attachment 446762 [details]
Patch

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

> Source/WebCore/platform/sql/SQLiteFileSystem.cpp:86
> +        String path = filePath + suffix;

I don't know if + is as efficient as makeString().

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:458
> +        std::optional<String> optionalLocalStoragePath;

We never use std::optional<String> because String is just a pointer and thus already has a "null" state.

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:50
> +    auto originIdentifier = fileName.substring(0, fileNameLength - suffixLength);

Probably worth a FIXME comment since we shouldn't be using origins in file names.

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:133
> +void LocalStorageManager::ensureStorageArea(const WebCore::ClientOrigin& origin, const String& path, std::optional<Ref<WorkQueue>> workQueue)

`std::optional<Ref<WorkQueue>>` makes little sense. Just use a RefPtr<WorkQueue>.

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:147
> +    ensureStorageArea(origin, m_path, std::make_optional(WTFMove(workQueue)));

No std::make_optional()

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.h:43
> +    static Vector<WebCore::SecurityOriginData> getOriginsOfLocalStorageData(const String& path);

We don't usually use "get" prefixes.

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.h:47
> +    bool isActive();

can it be const?

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.h:48
> +    bool hasDataInMemory();

ditto.

> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.cpp:37
> +    m_map = makeUnique<WebCore::StorageMap>(WebCore::StorageMap::noQuota);

Should be part of the initializer list, not the constructor body.

Also, does it really need to be a unique_ptr ? It seems like it can never be null and you rely on the StorageMap assignment operator for copying.

> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.cpp:80
> +    if (!m_map->length())

Do we really want to return an error when calling clear() and the StorageArea is already empty? Seems odd.

> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.cpp:99
> +

That's quite a few extra lines here.

> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.h:37
> +    WTF_MAKE_FAST_ALLOCATED;

Not needed, StorageAreaBase already has it.

> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.h:39
> +    MemoryStorageArea(const WebCore::ClientOrigin&, StorageAreaBase::StorageType = StorageAreaBase::StorageType::Session);

explicit

> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.h:49
> +    HashMap<String, String> allItems() final;

can this be const ?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:103
> +Ref<NetworkStorageManager> NetworkStorageManager::create(PAL::SessionID sessionID, const String& path, std::optional<String> customLocalStoragePath)

No std::optional<String> please and also don't pass by value.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:108
> +NetworkStorageManager::NetworkStorageManager(PAL::SessionID sessionID, const String& path, std::optional<String> customLocalStoragePath)

ditto.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:132
> +bool NetworkStorageManager::canHandleTypes(OptionSet<WebsiteDataType> types)

can this be const?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:214
> +        std::optional<String> localStoragePath;

No std::optional<String>, just String

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:429
> +            if (openingOrigin == ".DS_Store")

Should probably be in a #if PLATFORM(COCOA). Alternatively, maybe we could skip all files starting with a '.', in which case it would be more cross-platform.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:441
> +    for (auto origin : m_localOriginStorageManagers.keys())

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:444
> +    forEachOriginDirectory([&](auto directory) mutable {

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:451
> +        for (auto origin : origins)

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:463
> +    auto allOrigins = getAllOrigins();

We don't need this local variable, you can just use getAllOrigins() if your for loop.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:464
> +    for (auto origin : allOrigins) {

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:498
> +    auto allOrigins = getAllOrigins();

We don't need this local variable.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:499
> +    for (auto origin : allOrigins) {

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:586
> +        std::optional<String> newLocalStoragePath;

no std::optional<String>, just String

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:656
> +

extra line.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:692
> +        return completionHandler(HashMap<String, String> { });

completionHandler({ }) probably works?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:701
> +    auto storageArea = m_storageAreaRegistry->getStorageArea(identifier);

We can reduce the scope by moving it to the if condition.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:702
> +    bool quotaError = false;

needs a prefix since this is a Boolean variable. e.g. hasQuotaError or didExceedQuota.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:703
> +    if (storageArea) {

if (auto storageArea = m_storageAreaRegistry->getStorageArea(identifier))

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:716
> +    auto storageArea = m_storageAreaRegistry->getStorageArea(identifier);

Can be moved inside the if condition.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:727
> +    auto storageArea = m_storageAreaRegistry->getStorageArea(identifier);

ditto.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:76
> +    NetworkStorageManager(PAL::SessionID, const String& path, std::optional<String> customLocalStoragePath);

No std::optional<String>, just const String& or String&&.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:84
> +    HashSet<WebCore::ClientOrigin> getAllOrigins();

can this be const?

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:92
> +    // Message handlers for FileSystem

Period at the end.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:110
> +    // Message handlers for WebStorage

ditto.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:111
> +    void connectToLocalStorageArea(IPC::Connection&, StorageNamespaceIdentifier, const WebCore::SecurityOriginData&, CompletionHandler<void(StorageAreaIdentifier)>&&);

Assuming those are IPC handlers, you can do WebCore::SecurityOriginData&&.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:112
> +    void connectToTransientLocalStorageArea(IPC::Connection&, StorageNamespaceIdentifier, const WebCore::SecurityOriginData&, const WebCore::SecurityOriginData&, CompletionHandler<void(StorageAreaIdentifier)>&&);

ditto.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:113
> +    void connectToSessionStorageArea(IPC::Connection&, StorageNamespaceIdentifier, const WebCore::SecurityOriginData&, CompletionHandler<void(StorageAreaIdentifier)>&&);

ditto.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:117
> +    void setItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, const String& key, const String& value, const String& urlString);

String&& for all the strings.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:118
> +    void removeItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, const String& key, const String& urlString);

ditto.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:119
> +    void clear(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, const String& urlString);

String&&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:130
> +    std::optional<String> m_customLocalStoragePath;

Never std::optional<String>, just String.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:46
> +    StorageBucket(const String& rootPath, const String& identifier, std::optional<String> localStoragePath)

const String& or String&&, never std::optional<String>

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:70
> +        if (storageIdentifier == "FileSystem")

What about LocalStorage and Session Storage?

Seems logical to mirror...

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:81
> +        case StorageType::LocalStorage:

... this function.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:112
> +            m_localStorageManager = makeUnique<LocalStorageManager>(m_localStoragePath ? *m_localStoragePath : String { }, registry);

No ternary.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:135
>      bool isActive()

can this be const?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:140
> +    bool isEmpty()

can this be const?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:155
> +    OptionSet<WebsiteDataType> fetchData(OptionSet<WebsiteDataType> types)

I don't really understand these names. It says it is fetching data but it doesn't seem to return any data, just data types. Should it be fetchDataTypes()? It is also taking the types in parameter so even fetchDataTypes() would be unclear IMO.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:175
> +    void moveData(const String& path, std::optional<String> localStoragePath)

No std::optional<String>

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:188
> +    OptionSet<WebsiteDataType> fetchDataInMemory(OptionSet<WebsiteDataType> types)

Weird name again.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:190
> +        OptionSet<WebsiteDataType> result { };

No need for { }

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:204
> +    OptionSet<WebsiteDataType> fetchDataFromDisk(OptionSet<WebsiteDataType> types)

Weird name again.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:206
> +        OptionSet<WebsiteDataType> result { };

No need for { }

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:228
> +    void deleteLocalStorageData(WallTime time)

deleteLocalStorageData -> deleteLocalStorageDataSince? given that it is taking a time in parameter.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:231
> +            m_localStorageManager->clearData();

This seems to ignore the time?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:258
> +    std::optional<String> m_localStoragePath;

std::optional<String> -> String

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:262
> +OriginStorageManager::OriginStorageManager(String&& path, std::optional<String> localStoragePath)

ditto.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:315
> +bool OriginStorageManager::isEmpty()

can this be const?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:29
> +#include <WebCore/ClientOrigin.h>

Why the include instead of the forward declare?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:44
> +    OriginStorageManager(String&& path, std::optional<String> localStoragePath);

std::optional<String> -> String&& or const String&

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:55
>      bool isActive();

can this be const?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:56
> +    bool isEmpty();

ditto.

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:57
> +    OptionSet<WebsiteDataType> fetchData(OptionSet<WebsiteDataType>);

Weird name, really unclear what this does.

Can it be const too?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:59
> +    void moveData(const String& newPath, std::optional<String> localStoragePath);

no std::optional<>

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:73
> +    std::optional<String> m_localStoragePath;

no std::optional<>

> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:68
> +{

We should add plenty of ASSERT(!isMainRunLoop()) in this class since it does database operations and we wouldn't want those to happen on the main thread, ever.

> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:176
> +    m_queue->dispatchAfter(transactionDuration, [weakThis = WeakPtr { *this }] {

I assume SQLiteStorageArea only gets constructed and destroyed on the queue? Otherwise, the usage of weakThis here would be unsafe.

> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:192
> +        auto result = m_database->prepareHeapStatement(statementString(type));

Can be moved inside the if condition

> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.h:35
> +    WTF_MAKE_FAST_ALLOCATED;

No needed, StorageAreaBase already has this.

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:39
> +bool SessionStorageManager::isActive()

can this be const?

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:46
> +bool SessionStorageManager::hasDataInMemory()

can this be const?

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:70
> +    m_storageAreasByNamespace.set(namespaceIdentifier, identifier);

Why are we using set() instead of add()? Do we expect to overwrite sometimes?

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:71
> +    m_storageAreas.set(identifier, WTFMove(storageArea));

ditto.

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:108
> +

To many blank lines here.

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:39
> +    SessionStorageManager(StorageAreaRegistry&);

explicit

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:40
> +    bool isActive();

const ?

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:41
> +    bool hasDataInMemory();

const ?

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:2
> + * Copyright (C) 2021 Apple Inc. All rights reserved.

It is not clear to me which code is new and which code was moved. However, I believe that some code in this patch was just moved. When you move code (even if it gets modified), you should maintain the copyright header.
Now it all looks like new code from 2021 when I am pretty sure that some of the code is older and was merely moved.

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:79
> +

Too many blank lines

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:50
> +    StorageAreaBase(unsigned quota, const WebCore::ClientOrigin&);

The constructor should be protected, not public since this is a virtual interface.

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:53
> +    enum class Type : uint8_t { SQLite, Memory };

bool would suffice, not uint8_t.

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:55
> +    enum class StorageType : bool { Session, Local };

did it right here :)

> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:85
> +

Too many blank lines.

> Source/WebKit/NetworkProcess/storage/StorageAreaRegistry.cpp:51
> +    if (auto storageArea = m_storageAreas.get(identifier))

seems this could all just be on one line?
```
return m_storageAreas.get(identifier).get();
```

> Source/WebKit/NetworkProcess/storage/StorageAreaRegistry.h:28
> +#include "Connection.h"

What is this for?

[Sihui Liu]

Comment on attachment 446762 [details]
Patch

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

>> Source/WebCore/platform/sql/SQLiteFileSystem.cpp:86
>> +        String path = filePath + suffix;
> 
> I don't know if + is as efficient as makeString().

It seems operator uses StringAppend, and StringAppend uses tryMakeString, and makeString uses tryMakeString.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:458
>> +        std::optional<String> optionalLocalStoragePath;
> 
> We never use std::optional<String> because String is just a pointer and thus already has a "null" state.

I guess I can change it to String. Try to make a more obvious difference between empty string for ephemeral session v.s. not using custom local storage path for persistent session.

>> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:50
>> +    auto originIdentifier = fileName.substring(0, fileNameLength - suffixLength);
> 
> Probably worth a FIXME comment since we shouldn't be using origins in file names.

Sure. This function should only be used for existing files.

>> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:133
>> +void LocalStorageManager::ensureStorageArea(const WebCore::ClientOrigin& origin, const String& path, std::optional<Ref<WorkQueue>> workQueue)
> 
> `std::optional<Ref<WorkQueue>>` makes little sense. Just use a RefPtr<WorkQueue>.

Sure.

>> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:147
>> +    ensureStorageArea(origin, m_path, std::make_optional(WTFMove(workQueue)));
> 
> No std::make_optional()

Will remove.

>> Source/WebKit/NetworkProcess/storage/LocalStorageManager.h:43
>> +    static Vector<WebCore::SecurityOriginData> getOriginsOfLocalStorageData(const String& path);
> 
> We don't usually use "get" prefixes.

Okay, will remove.

>> Source/WebKit/NetworkProcess/storage/LocalStorageManager.h:47
>> +    bool isActive();
> 
> can it be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/LocalStorageManager.h:48
>> +    bool hasDataInMemory();
> 
> ditto.

Yes.

>> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.cpp:37
>> +    m_map = makeUnique<WebCore::StorageMap>(WebCore::StorageMap::noQuota);
> 
> Should be part of the initializer list, not the constructor body.
> 
> Also, does it really need to be a unique_ptr ? It seems like it can never be null and you rely on the StorageMap assignment operator for copying.

I guess it does not need to be a unique_ptr; will change.

>> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.cpp:80
>> +    if (!m_map->length())
> 
> Do we really want to return an error when calling clear() and the StorageArea is already empty? Seems odd.

No, we can return void.

>> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.cpp:99
>> +
> 
> That's quite a few extra lines here.

Will remove.

>> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.h:37
>> +    WTF_MAKE_FAST_ALLOCATED;
> 
> Not needed, StorageAreaBase already has it.

Will remove.

>> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.h:39
>> +    MemoryStorageArea(const WebCore::ClientOrigin&, StorageAreaBase::StorageType = StorageAreaBase::StorageType::Session);
> 
> explicit

Will add.

>> Source/WebKit/NetworkProcess/storage/MemoryStorageArea.h:49
>> +    HashMap<String, String> allItems() final;
> 
> can this be const ?

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:103
>> +Ref<NetworkStorageManager> NetworkStorageManager::create(PAL::SessionID sessionID, const String& path, std::optional<String> customLocalStoragePath)
> 
> No std::optional<String> please and also don't pass by value.

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:108
>> +NetworkStorageManager::NetworkStorageManager(PAL::SessionID sessionID, const String& path, std::optional<String> customLocalStoragePath)
> 
> ditto.

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:132
>> +bool NetworkStorageManager::canHandleTypes(OptionSet<WebsiteDataType> types)
> 
> can this be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:214
>> +        std::optional<String> localStoragePath;
> 
> No std::optional<String>, just String

Okay.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:429
>> +            if (openingOrigin == ".DS_Store")
> 
> Should probably be in a #if PLATFORM(COCOA). Alternatively, maybe we could skip all files starting with a '.', in which case it would be more cross-platform.

good idea; will skip all '.' files.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:441
>> +    for (auto origin : m_localOriginStorageManagers.keys())
> 
> auto&

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:444
>> +    forEachOriginDirectory([&](auto directory) mutable {
> 
> auto&

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:451
>> +        for (auto origin : origins)
> 
> auto&

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:463
>> +    auto allOrigins = getAllOrigins();
> 
> We don't need this local variable, you can just use getAllOrigins() if your for loop.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:464
>> +    for (auto origin : allOrigins) {
> 
> auto&

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:498
>> +    auto allOrigins = getAllOrigins();
> 
> We don't need this local variable.

Will remove.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:499
>> +    for (auto origin : allOrigins) {
> 
> auto&

Will add.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:586
>> +        std::optional<String> newLocalStoragePath;
> 
> no std::optional<String>, just String

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:656
>> +
> 
> extra line.

Will remove.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:692
>> +        return completionHandler(HashMap<String, String> { });
> 
> completionHandler({ }) probably works?

Will try.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:701
>> +    auto storageArea = m_storageAreaRegistry->getStorageArea(identifier);
> 
> We can reduce the scope by moving it to the if condition.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:702
>> +    bool quotaError = false;
> 
> needs a prefix since this is a Boolean variable. e.g. hasQuotaError or didExceedQuota.

Okay.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:703
>> +    if (storageArea) {
> 
> if (auto storageArea = m_storageAreaRegistry->getStorageArea(identifier))

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:716
>> +    auto storageArea = m_storageAreaRegistry->getStorageArea(identifier);
> 
> Can be moved inside the if condition.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:727
>> +    auto storageArea = m_storageAreaRegistry->getStorageArea(identifier);
> 
> ditto.

Will move.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:76
>> +    NetworkStorageManager(PAL::SessionID, const String& path, std::optional<String> customLocalStoragePath);
> 
> No std::optional<String>, just const String& or String&&.

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:84
>> +    HashSet<WebCore::ClientOrigin> getAllOrigins();
> 
> can this be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:92
>> +    // Message handlers for FileSystem
> 
> Period at the end.

Will add.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:110
>> +    // Message handlers for WebStorage
> 
> ditto.

Will add.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:111
>> +    void connectToLocalStorageArea(IPC::Connection&, StorageNamespaceIdentifier, const WebCore::SecurityOriginData&, CompletionHandler<void(StorageAreaIdentifier)>&&);
> 
> Assuming those are IPC handlers, you can do WebCore::SecurityOriginData&&.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:112
>> +    void connectToTransientLocalStorageArea(IPC::Connection&, StorageNamespaceIdentifier, const WebCore::SecurityOriginData&, const WebCore::SecurityOriginData&, CompletionHandler<void(StorageAreaIdentifier)>&&);
> 
> ditto.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:113
>> +    void connectToSessionStorageArea(IPC::Connection&, StorageNamespaceIdentifier, const WebCore::SecurityOriginData&, CompletionHandler<void(StorageAreaIdentifier)>&&);
> 
> ditto.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:117
>> +    void setItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, const String& key, const String& value, const String& urlString);
> 
> String&& for all the strings.

Will change.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:118
>> +    void removeItem(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, const String& key, const String& urlString);
> 
> ditto.

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:119
>> +    void clear(IPC::Connection&, StorageAreaIdentifier, StorageAreaImplIdentifier, uint64_t seed, const String& urlString);
> 
> String&&

Sure.

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:130
>> +    std::optional<String> m_customLocalStoragePath;
> 
> Never std::optional<String>, just String.

Okay.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:46
>> +    StorageBucket(const String& rootPath, const String& identifier, std::optional<String> localStoragePath)
> 
> const String& or String&&, never std::optional<String>

Okay.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:70
>> +        if (storageIdentifier == "FileSystem")
> 
> What about LocalStorage and Session Storage?
> 
> Seems logical to mirror...

Will add.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:81
>> +        case StorageType::LocalStorage:
> 
> ... this function.

ack.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:112
>> +            m_localStorageManager = makeUnique<LocalStorageManager>(m_localStoragePath ? *m_localStoragePath : String { }, registry);
> 
> No ternary.

Will remove.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:135
>>      bool isActive()
> 
> can this be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:140
>> +    bool isEmpty()
> 
> can this be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:155
>> +    OptionSet<WebsiteDataType> fetchData(OptionSet<WebsiteDataType> types)
> 
> I don't really understand these names. It says it is fetching data but it doesn't seem to return any data, just data types. Should it be fetchDataTypes()? It is also taking the types in parameter so even fetchDataTypes() would be unclear IMO.

Will change it to something like fetchDataTypesInList().

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:175
>> +    void moveData(const String& path, std::optional<String> localStoragePath)
> 
> No std::optional<String>

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:188
>> +    OptionSet<WebsiteDataType> fetchDataInMemory(OptionSet<WebsiteDataType> types)
> 
> Weird name again.

Will change to fetchDataTypesInListFromMemory()

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:190
>> +        OptionSet<WebsiteDataType> result { };
> 
> No need for { }

Will remove.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:204
>> +    OptionSet<WebsiteDataType> fetchDataFromDisk(OptionSet<WebsiteDataType> types)
> 
> Weird name again.

Will change to fetchDataTypesInListFromDisk()

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:206
>> +        OptionSet<WebsiteDataType> result { };
> 
> No need for { }

Will remove.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:228
>> +    void deleteLocalStorageData(WallTime time)
> 
> deleteLocalStorageData -> deleteLocalStorageDataSince? given that it is taking a time in parameter.

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:231
>> +            m_localStorageManager->clearData();
> 
> This seems to ignore the time?

ah nice catch; should probably check modification time first and then decide whether we need to clear data here. Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:258
>> +    std::optional<String> m_localStoragePath;
> 
> std::optional<String> -> String

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:262
>> +OriginStorageManager::OriginStorageManager(String&& path, std::optional<String> localStoragePath)
> 
> ditto.

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:315
>> +bool OriginStorageManager::isEmpty()
> 
> can this be const?

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:29
>> +#include <WebCore/ClientOrigin.h>
> 
> Why the include instead of the forward declare?

oh this was added when I added ClientOrigin as parameter in the file; not needed any more. Will change back.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:44
>> +    OriginStorageManager(String&& path, std::optional<String> localStoragePath);
> 
> std::optional<String> -> String&& or const String&

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:55
>>      bool isActive();
> 
> can this be const?

Sure.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:56
>> +    bool isEmpty();
> 
> ditto.

Will add const.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:57
>> +    OptionSet<WebsiteDataType> fetchData(OptionSet<WebsiteDataType>);
> 
> Weird name, really unclear what this does.
> 
> Can it be const too?

Yes.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:59
>> +    void moveData(const String& newPath, std::optional<String> localStoragePath);
> 
> no std::optional<>

Will change.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.h:73
>> +    std::optional<String> m_localStoragePath;
> 
> no std::optional<>

Wil change.

>> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:68
>> +{
> 
> We should add plenty of ASSERT(!isMainRunLoop()) in this class since it does database operations and we wouldn't want those to happen on the main thread, ever.

Okay, will add.

>> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:176
>> +    m_queue->dispatchAfter(transactionDuration, [weakThis = WeakPtr { *this }] {
> 
> I assume SQLiteStorageArea only gets constructed and destroyed on the queue? Otherwise, the usage of weakThis here would be unsafe.

Yes, it's used only on one queue.

>> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:192
>> +        auto result = m_database->prepareHeapStatement(statementString(type));
> 
> Can be moved inside the if condition

Sure.

>> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.h:35
>> +    WTF_MAKE_FAST_ALLOCATED;
> 
> No needed, StorageAreaBase already has this.

Will remove.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:39
>> +bool SessionStorageManager::isActive()
> 
> can this be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:46
>> +bool SessionStorageManager::hasDataInMemory()
> 
> can this be const?

Yes.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:70
>> +    m_storageAreasByNamespace.set(namespaceIdentifier, identifier);
> 
> Why are we using set() instead of add()? Do we expect to overwrite sometimes?

I think we don't expect overwrite (unless there's some edge case I'm not aware of)

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:71
>> +    m_storageAreas.set(identifier, WTFMove(storageArea));
> 
> ditto.

Will change to add.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.cpp:108
>> +
> 
> To many blank lines here.

Will remove.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:39
>> +    SessionStorageManager(StorageAreaRegistry&);
> 
> explicit

Will add.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:40
>> +    bool isActive();
> 
> const ?

Will add.

>> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:41
>> +    bool hasDataInMemory();
> 
> const ?

Will add.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:2
>> + * Copyright (C) 2021 Apple Inc. All rights reserved.
> 
> It is not clear to me which code is new and which code was moved. However, I believe that some code in this patch was just moved. When you move code (even if it gets modified), you should maintain the copyright header.
> Now it all looks like new code from 2021 when I am pretty sure that some of the code is older and was merely moved.

Sure, I will update the date info here.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.cpp:79
>> +
> 
> Too many blank lines

Will remove.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:50
>> +    StorageAreaBase(unsigned quota, const WebCore::ClientOrigin&);
> 
> The constructor should be protected, not public since this is a virtual interface.

Will move.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:53
>> +    enum class Type : uint8_t { SQLite, Memory };
> 
> bool would suffice, not uint8_t.

I was considering about possibility of adding other implementation (like noSQL database); but seems Okay to use bool now.

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:55
>> +    enum class StorageType : bool { Session, Local };
> 
> did it right here :)

And storage type can only be either Session or Local (Persistent) :)

>> Source/WebKit/NetworkProcess/storage/StorageAreaBase.h:85
>> +
> 
> Too many blank lines.

Will remove.

>> Source/WebKit/NetworkProcess/storage/StorageAreaRegistry.cpp:51
>> +    if (auto storageArea = m_storageAreas.get(identifier))
> 
> seems this could all just be on one line?
> ```
> return m_storageAreas.get(identifier).get();
> ```

Will change.

>> Source/WebKit/NetworkProcess/storage/StorageAreaRegistry.h:28
>> +#include "Connection.h"
> 
> What is this for?

I forgot why I added this. Will remove.

[Sihui Liu]

Created attachment 446838 [details]
Patch

[Chris Dumez]

Comment on attachment 446838 [details]
Patch

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

r=me assuming the bots are happy.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:321
> +    for (auto& manager : m_storageManagers.values())

I think eventually we'll want to store the NetworkStorageManager on the NetworkSession object and get rid of the NetworkProcess::m_storageManagers HashMap.
Probably shouldn't be done in this already very large patch though.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1933
> +            iterator->value->deleteDataForRegistrableDomains(websiteDataTypes, domainsToDeleteAllNonCookieWebsiteDataFor, [callbackAggregator](auto deletedDomains) mutable {

auto&&

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1934
> +                for (auto domain : deletedDomains)

auto&

> Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:144
> +        m_storageArea = makeUnique<SQLiteStorageArea>(localStorageQuotaInBytes, origin, path, Ref { *workQueue });

Unnecessary ref counting churn, you want `workQueue.releaseNonNull()`.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:426
> +            if (openingOrigin.startsWith("."))

openingOrigin.startsWith('.') is more efficient here.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:457
> +    for (auto origin : getAllOrigins()) {

auto&

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:491
> +    for (auto origin : getAllOrigins()) {

auto&

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:148
> +            return file != ".DS_Store"_s && file != originFileName;

Probably want a #if PLATFORM(COCOA) for the .DS_Store check.

> Source/WebKit/NetworkProcess/storage/SessionStorageManager.h:59
> +

extra lines

[Sihui Liu]

Created attachment 446928 [details]
Patch

[Sihui Liu]

Created attachment 446937 [details]
Patch

[Sihui Liu]

Created attachment 446949 [details]
Patch

[Sihui Liu]

Created attachment 446961 [details]
Patch

[Sihui Liu]

Created attachment 446963 [details]
Patch

[EWS]

Committed r286936 (245162@main): <https://commits.webkit.org/245162@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/86399162>