[webkit-reviews] review denied: [Bug 191646] [Curl] Add Server Trust Evaluation Support. : [Attachment 366052] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Takashi Komori
<Takashi.Komori at sony.com>'s request for review:
Bug 191646: [Curl] Add Server Trust Evaluation Support.
https://bugs.webkit.org/show_bug.cgi?id=191646

Attachment 366052: Patch

https://bugs.webkit.org/attachment.cgi?id=366052&action=review




--- Comment #3 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 366052
  --> https://bugs.webkit.org/attachment.cgi?id=366052
Patch

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

> Source/WebCore/platform/network/curl/AuthenticationChallengeCurl.cpp:80
> +    return ProtectionSpace(url.host().toString(), static_cast<int>(port ?
*port : 0), serverType, String(), authenticationScheme, certificateInfo);

"(port ? *port : 0)" can be "port.valueOr(0)". protectionSpaceFromHandle has
the same code.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:423
> +void NetworkDataTaskCurl::restartWithCredential(const
AuthenticationChallenge& challenge, const Credential& credential)

It seems that you don't need change the first argument.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:433
> +    if
(!challenge.protectionSpace().certificateInfo().certificateChain().isEmpty())

Is this never be empty in case of ServerTrustEvaluation? 
if (challenge.protectionSpace().authenticationScheme() ==
ProtectionSpaceAuthenticationSchemeServerTrustEvaluationRequested)
I feel this is somewhat better.

> Tools/MiniBrowser/win/Common.cpp:288
> +std::wstring replaceWString(std::wstring src, const std::wstring& oldValue,
const std::wstring& newValue)

You can name this 'replaceString'. If you will need a std::string version, you
can overload replaceString(std::string, const std::string&, const
std::string&).

> Tools/MiniBrowser/win/Common.cpp:294
> +    while ((pos = src.find(oldValue, pos)) != std::string::npos) {

Nit: std::string::npos should be std::wstring::npos or src.npos.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:323
> +    if (it != m_acceptedServerTrustCert.end() && it->second == pem)

You check only the first matching. m_acceptedServerTrustCert can be
std::set<std::pair<std::wstring,std::wstring>>.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:334
> +std::wstring WebKitBrowserWindow::createPEMString(WKProtectionSpaceRef
protectionSpace)

This shouldn't be a WebKitBrowserWindow's method.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:345
> +	   pems += std::wstring(reinterpret_cast<const char*>(data),
reinterpret_cast<const char*>(data + size));

std::wstring(reinterpret_cast<const char*>(data), size)

> Tools/MiniBrowser/win/WebKitBrowserWindow.h:77
> +    std::map<std::wstring, std::wstring> m_acceptedServerTrustCert;

should be plural if this is just a single cert.
m_acceptedServerTrustCerts