Bug 230565

Summary: Pending preconnect key should include User-Agent
Product: WebKit Reporter: Ben Nham <nham>
Component: New BugsAssignee: Ben Nham <nham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, nham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ben Nham 2021-09-21 10:35:26 PDT 
Pending preconnect key should include User-Agent
Comment 1 Ben Nham 2021-09-21 11:06:16 PDT 
Created attachment 438836 [details]
Patch
Comment 2 Ben Nham 2021-09-21 11:08:39 PDT 
<rdar://83259078>
Comment 3 Ben Nham 2021-09-21 22:33:24 PDT 
Created attachment 438922 [details]
Patch
Comment 4 Chris Dumez 2021-09-24 13:14:09 PDT 
Comment on attachment 438922 [details]
Patch

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

r=me with changes.

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.cpp:156
> +static std::tuple<String, String> mainResourceLoadKey(String protocolHostAndPort, String userAgent)

`const String&` for both parameters to avoid ref-counting churn.

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.h:54
> +    void startedPreconnectForMainResource(const URL&, const String&);

Please do not omit the second parameter name as it is not obvious what it is.

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.h:55
> +    void finishedPreconnectForMainResource(const URL&, const String&, const WebCore::ResourceError&);

ditto.

> Source/WebKit/NetworkProcess/NetworkLoadScheduler.h:81
> +    using PendingPreconnectMap = HashMap<std::tuple<String, String>, PendingMainResourcePreconnectInfo>;

We probably want inline comments to indicate what those Strings are.
Comment 5 Ben Nham 2021-09-24 14:00:25 PDT 
Created attachment 439195 [details]
Patch
Comment 6 EWS 2021-09-25 09:12:33 PDT 
Committed r283081 (242139@main): <https://commits.webkit.org/242139@main>

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