Bug 195247 – [SOUP] Cleanups in SoupNetworkSession

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

clientCertificates actually contains server certificates, not client certificates. Client certificate support is still on my TODO.

[Michael Catanzaro]

Created attachment 363424 [details]
Patch

[Daniel Bates]

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.

[Michael Catanzaro]

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

[Michael Catanzaro]

Committed r242328: <https://trac.webkit.org/changeset/242328>