[webkit-reviews] review granted: [Bug 237335] REGRESSION (r289474): cacheStoragePath is empty in NetworkStorageManager::localOriginStorageManager : [Attachment 453564] 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 237335: REGRESSION (r289474): cacheStoragePath is empty in
NetworkStorageManager::localOriginStorageManager
https://bugs.webkit.org/show_bug.cgi?id=237335

Attachment 453564: Patch

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




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

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

r=me with comments.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:419
> +    RetainPtr<CFStringRef> cfIdentifier = makeString(identifierBase,
".PrivateBrowsing.", createVersion4UUIDString()).createCFString();

Could use auto.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:442
> +    if (m_networkSessions.contains(sessionID))

This could be dropped and we could use HashMap::ensure() below to avoid double
hash map  lookup.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2177
> +    if (auto* session = networkSession(sessionID))

BUG: You're now failing to call the completionHandler in the else case.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2282
> +const String& NetworkProcess::uiProcessBundleIdentifier()

Given that this is a one liner, I would keep inline in the header. Outside the
class but inline in the header:
```
#if !PLATFORM(COCOA)
inline const String& NetworkProcess::uiProcessBundleIdentifier() const
{
    return m_uiProcessBundleIdentifier;
}
#endif
```

> Source/WebKit/NetworkProcess/NetworkProcess.h:303
> +    const String& uiProcessBundleIdentifier();

I think this should remain const.

> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:480
> +	   , WTFMove(*shouldUseCustomStoragePaths)

Please don't use WTFMove() for booleans and integers.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:258
> +const String& NetworkProcess::uiProcessBundleIdentifier()

I think we should keep the getter as const and mark m_uiProcessBundleIdentifier
as mutable. It is a typical pattern for lazy initialization.

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:260
> +    if (m_uiProcessBundleIdentifier.isEmpty())

Shouldn't this is a isNull() check?

> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:261
> +	   m_uiProcessBundleIdentifier = String([[NSBundle mainBundle]
bundleIdentifier]);

Is String() really needed?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1856
> +	   FileSystem::makeAllDirectories(directory);

Not new in this patch but we really should NOT be blocking the main thread of
the UIProcess (or any process for that matter but the UIProcess is probably
this worse) on file system operations.

If you don't want to fix in this patch, at the very least add a FIXME because
we likely need to revisit.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1857
> +	   FileSystem::excludeFromBackup(directory);

ditto.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:257
> +    const String& resolvedIndexedDBDirectory() const { return
m_resolvedConfiguration->indexedDBDatabaseDirectory(); }

The lack of consistency in the naming between "resolvedIndexedDBDirectory" and
`m_resolvedConfiguration->indexedDBDatabaseDirectory()` is bothering me.