Bug 195247

Summary: [SOUP] Cleanups in SoupNetworkSession
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, cgarcia, dbates, ews-watchlist, gustavo, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=195257
Attachments:
Description Flags
Patch dbates: review+

Description Michael Catanzaro 2019-03-02 10:32:38 PST 
clientCertificates actually contains server certificates, not client certificates. Client certificate support is still on my TODO.
Comment 1 Michael Catanzaro 2019-03-02 10:38:28 PST 
Created attachment 363424 [details]
Patch
Comment 2 Daniel Bates 2019-03-02 15:12:46 PST 
Comment on attachment 363424 [details]
Patch

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

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:101
> +static HashMap<String, HostTLSCertificateSet, ASCIICaseInsensitiveHash>& allowedCertificates()

Ok as-is. Just a thought, define a type alias for the HashMap to avoid repetition below.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:287
> +    auto it = allowedCertificates().find(requestURL.host().toString());

Ok as-is, but can be made a bit more efficient by using toStringWithoutCopying(). Even better, as I type this I could swear we solved this issue with template magic and made it so you could pass StringView to HashMap::find(). Can you check? I think we call this magic a HashTranslator or adapter and there is a template overload of HashMap::find(). And if we don’t have such magic because we are missing the HashTranslator for StringView then please either add it OR file a bug so that we can add it.
Comment 3 Michael Catanzaro 2019-03-03 09:50:36 PST 
(In reply to Daniel Bates from comment #2)
> Ok as-is. Just a thought, define a type alias for the HashMap to avoid
> repetition below.

Yeah, OK.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:287
> > +    auto it = allowedCertificates().find(requestURL.host().toString());
> 
> Ok as-is, but can be made a bit more efficient by using
> toStringWithoutCopying().

OK.

> Even better, as I type this I could swear we
> solved this issue with template magic and made it so you could pass
> StringView to HashMap::find(). Can you check? I think we call this magic a
> HashTranslator or adapter and there is a template overload of
> HashMap::find(). And if we don’t have such magic because we are missing the
> HashTranslator for StringView then please either add it OR file a bug so
> that we can add it.

Doesn't work, reported bug #195257.
Comment 4 Michael Catanzaro 2019-03-03 09:51:44 PST 
Committed r242328: <https://trac.webkit.org/changeset/242328>