[webkit-reviews] review canceled: [Bug 234087] Merge StorageManager with NetworkStorageManager to manage WebStorage by origin : [Attachment 446762] Patch

[bugzilla-daemon at webkit.org]

Sihui Liu <sihui_liu at apple.com> has canceled Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 234087: Merge StorageManager with NetworkStorageManager to manage
WebStorage by origin
https://bugs.webkit.org/show_bug.cgi?id=234087

Attachment 446762: Patch

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




--- Comment #8 from Sihui Liu <sihui_liu at apple.com> ---
Comment on attachment 446762
  --> https://bugs.webkit.org/attachment.cgi?id=446762
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.