[webkit-reviews] review granted: [Bug 189415] Move IndexedDB to Network Process : [Attachment 349782] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 189415: Move IndexedDB to Network Process
https://bugs.webkit.org/show_bug.cgi?id=189415

Attachment 349782: Patch

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




--- Comment #38 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 349782
  --> https://bugs.webkit.org/attachment.cgi?id=349782
Patch

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

r=me with comments

> Source/WebKit/ChangeLog:25
> +

I think this is 1 blank line too many?

> Source/WebKit/ChangeLog:29
> +

Why the blank line?

> Source/WebKit/ChangeLog:124
> +	   process just asks for sandbox extension fot itself.

Typo: fot.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:512
> +	   if (!m_connection->isValid())

This check should not be needed as IPC::Connection::send() already does this.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:38
> +#include <pal/SessionID.h>

Can probably be forward-declared.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:245
> +    // Messages handlers (Modern IDB)

Period at the end.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:246
> +    void establishIDBConnectionToServer(PAL::SessionID, uint64_t&
serverConnectionIdentifier);

We do not usually put methods in the middle of the data members, please move
these methods above, even if you need a new #if ENABLE(INDEXED_DATABASE) block.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:986
> +void NetworkProcess::ensurePathExists(const String& path)

Does not seem like this method is needed. Why not use
FileSystem::makeAllDirectories() directly?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1064
> +    auto addResult = m_idbDatabasePaths.ensure(sessionID, [path =
indexedDatabaseDirectory] {

I do not see any benefit to using ensure() here, add() would suffice and likely
be more efficient:
auto addResult = m_idbDatabasePaths.add(sessionID, indexedDatabaseDirectory);

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1077
> +void NetworkProcess::getSandboxExtensionsForBlobFiles(const Vector<String>&
filenames, WTF::Function<void(SandboxExtension::HandleArray&&)>&&
completionHandler)

WTF:: is not needed.

> Source/WebKit/NetworkProcess/NetworkProcess.h:169
> +    WorkQueue& queue() { return m_storageTaskQueue.get(); }

queue() is too generic a name here given that we're on the Network process.
Maybe storageQueue() ?

Also does this method need to be public?

> Source/WebKit/NetworkProcess/NetworkProcess.h:170
> +    void postStorageTask(CrossThreadTask&&);

does this method need to be public?

> Source/WebKit/NetworkProcess/NetworkProcess.h:173
> +    WebCore::IDBServer::IDBServer& idbServer(PAL::SessionID);

does this method need to be public?

> Source/WebKit/NetworkProcess/NetworkProcess.h:176
> +    void prepareForAccessToTemporaryFile(const String& path) final;

does this method need to be public?

> Source/WebKit/NetworkProcess/NetworkProcess.h:177
> +    void accessToTemporaryFileComplete(const String& path) final;

does this method need to be public?

> Source/WebKit/NetworkProcess/NetworkProcess.h:181
> +    void getSandboxExtensionsForBlobFiles(const Vector<String>& filenames,
WTF::Function<void(SandboxExtension::HandleArray&&)>&& completionHandler);

does this method need to be public?

> Source/WebKit/NetworkProcess/NetworkProcess.h:182
> +    void updateTemporaryFileSandboxExtensions(const Vector<String>& paths,
SandboxExtension::HandleArray&);

does this method need to be public?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1635
> +    resolveDirectoriesIfNecessary();

This is already called at the beginning of the method, why is this needed again
here?

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.cpp:113
> +	   auto idbConnection =
m_webIDBConnectionsByIdentifier.get(decoder.destinationID());

can be:
if (auto idbConnection =
m_webIDBConnectionsByIdentifier.get(decoder.destinationID()))

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:68
> +    WebIDBConnectionToServer*
existingIDBConnectionToServerForIdentifier(uint64_t identifier) { return
m_webIDBConnectionsByIdentifier.get(identifier); };

Can probably be const.