Bug 234179 - Move SWServers from NetworkProcess to NetworkSession
Summary: Move SWServers from NetworkProcess to NetworkSession
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-10 15:02 PST by Chris Dumez
Modified: 2021-12-15 14:20 PST (History)
9 users (show)

See Also:


Attachments
Patch (45.32 KB, patch)
2021-12-10 15:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (45.66 KB, patch)
2021-12-11 15:33 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (45.00 KB, patch)
2021-12-15 08:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-12-10 15:02:44 PST
Move SWServers from NetworkProcess to NetworkSession, since they are per session.
Comment 1 Chris Dumez 2021-12-10 15:07:45 PST
Created attachment 446824 [details]
Patch
Comment 2 Chris Dumez 2021-12-11 15:33:40 PST
Created attachment 446909 [details]
Patch
Comment 3 Chris Dumez 2021-12-14 07:27:02 PST
ping review?
Comment 4 Darin Adler 2021-12-14 09:05:28 PST
Comment on attachment 446909 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:504
> +        auto& server = session->ensureSWServer();
> +        if (!server.isImportCompleted()) {

Scope it using the semicolon style?

    if (auto& server = session->ensureSWServer(); !server.isImportCompleted()) {

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:509
> +                ASSERT(networkSession() && networkSession()->swServer() && networkSession()->swServer()->isImportCompleted());

I would write three separate assertions. Nicer to know which one failed.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1596
> +            // If we are deleting all of the data types that the resource load statistics store monitors
> +            // we do not need to re-grandfather old data.
> +            auto shouldGrandfather = ((monitoredTypesRaw & deletedTypesRaw) == monitoredTypesRaw) ? ShouldGrandfatherStatistics::No : ShouldGrandfatherStatistics::Yes;

The enum makes this harder to read. I hope there’s other code that makes it easier to read.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1664
> +        Vector<String> hosts;
> +        hosts.reserveInitialCapacity(originDatas.size());
> +        for (auto& origin : originDatas)
> +            hosts.uncheckedAppend(origin.host);

Should do this more directly with Vector::map?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1849
> +        Vector<String> registrableDomainsToDelete;
> +        registrableDomainsToDelete.reserveInitialCapacity(domainsToDeleteAllNonCookieWebsiteDataFor.size());
> +        for (auto& domain : domainsToDeleteAllNonCookieWebsiteDataFor)
> +            registrableDomainsToDelete.uncheckedAppend(domain.string());

Same question.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:553
> +    UNUSED_PARAM(critical);

Could we omit the argument name instead of using UNUSED_PARAM?
Comment 5 Chris Dumez 2021-12-15 08:44:10 PST
Created attachment 447236 [details]
Patch
Comment 6 EWS 2021-12-15 10:20:55 PST
Committed r287084 (245279@main): <https://commits.webkit.org/245279@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447236 [details].
Comment 7 Radar WebKit Bug Importer 2021-12-15 10:22:43 PST
<rdar://problem/86530837>