Bug 195247 - [SOUP] Cleanups in SoupNetworkSession
Summary: [SOUP] Cleanups in SoupNetworkSession
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-02 10:32 PST by Michael Catanzaro
Modified: 2019-03-03 09:51 PST (History)
7 users (show)

See Also:

Attachments
Patch (2.78 KB, patch)
2019-03-02 10:38 PST, Michael Catanzaro 		dbates: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>