[webkit-reviews] review granted: [Bug 229925] Add basic support for Storage API : [Attachment 437575] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 229925: Add basic support for Storage API
https://bugs.webkit.org/show_bug.cgi?id=229925

Attachment 437575: Patch

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




--- Comment #21 from Darin Adler <darin at apple.com> ---
Comment on attachment 437575
  --> https://bugs.webkit.org/attachment.cgi?id=437575
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);
    }