Bug 234366 – Move WebIDBServers from NetworkProcess to NetworkSession class

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Move WebIDBServers from NetworkProcess to NetworkSession class, since they are per session.

[Chris Dumez]

Created attachment 447284 [details]
Patch

[Chris Dumez]

Created attachment 447286 [details]
Patch

[Chris Dumez]

Created attachment 447288 [details]
Patch

[Chris Dumez]

Created attachment 447292 [details]
Patch

[Darin Adler]

Comment on attachment 447292 [details]
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.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 447292 [details]
> 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.

Will fix.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1847
> > +            auto* session = this->networkSession(sessionID);
> 
> Not sure we need the "this->" here since there is no name collision.

I had to for this one because there is a networkSession variable outside the lambda and the compiler was complaining that it couldn't be called as a function and wasn't captured in the lambda.

> > 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.

Ok.

[Chris Dumez]

Created attachment 447354 [details]
Patch

[Chris Dumez]

Created attachment 447376 [details]
Patch

[Chris Dumez]

Created attachment 447377 [details]
Patch

[Chris Dumez]

Created attachment 447379 [details]
Patch

[Chris Dumez]

Created attachment 447381 [details]
Patch

[Chris Dumez]

Created attachment 447384 [details]
Patch

[EWS]

Committed r287159 (245336@main): <https://commits.webkit.org/245336@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447384 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86598786>