[webkit-reviews] review granted: [Bug 234366] Move WebIDBServers from NetworkProcess to NetworkSession class : [Attachment 447292] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 234366: Move WebIDBServers from NetworkProcess to NetworkSession class
https://bugs.webkit.org/show_bug.cgi?id=234366

Attachment 447292: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 447292
  --> https://bugs.webkit.org/attachment.cgi?id=447292
Patch

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

Looks like a good, relatively straightforward refactoring. I didn’t spot
anything obviously missing.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:393
> +    if (auto* networkSession = this->networkSession(sessionID))
> +	  
networkSession->ensureWebIDBServer().addConnection(connection.connection(),
identifier);

Tempting to just use "session" to avoid the need for "this->" and possibly make
things even more readable. Other cases of that below.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1847
> +	       auto* session = this->networkSession(sessionID);

Not sure we need the "this->" here since there is no name collision.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:677
> +    // *********
> +    // IMPORTANT: Do not change the directory structure for indexed
databases on disk without first consulting a reviewer from Apple
(<rdar://problem/17454712>)
> +    // *********

This is clearly a valuable comment. But is this function the absolute best
place for it?

> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:120
> +	   forEachNetworkSession([](auto& networkSession) {

Formatting tweak: can’t remember if we use spaces here.
Cod style tweak: Always love the single words when the scope is small enough,
so just "session" would be nice.