Bug 176910

Summary: [Curl] Create a class dedicated to handle SSL related task
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: WebCore Misc.Assignee: Basuke Suzuki <Basuke.Suzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, Basuke.Suzuki, buildbot, commit-queue, galpeter, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
fix
none
fix
achristensen: review-
fixed
achristensen: review+
fixed none

Description Basuke Suzuki 2017-09-14 08:54:05 PDT
Create SSLSettings/SSLVerifier classes to gather SSL related processes in one place.
And fixed file name from SSLHandle to CurlSSLHandle for classification.
Comment 1 Basuke Suzuki 2017-09-14 08:59:52 PDT
Created attachment 320772 [details]
patch
Comment 2 Build Bot 2017-09-14 09:01:52 PDT
Attachment 320772 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Basuke Suzuki 2017-09-14 14:54:48 PDT
Created attachment 320827 [details]
fix
Comment 4 Basuke Suzuki 2017-09-14 14:59:07 PDT
Created attachment 320829 [details]
fix
Comment 5 Alex Christensen 2017-09-14 16:57:57 PDT
Comment on attachment 320829 [details]
fix

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.cppSource/WebCore/platform/network/curl/SSLHandle.cpp:146
> +// CurlSSLVeritifer -------------------------------------------------

I don't think comments like this are helpful.  This one's spelled wrong.

> Source/WebCore/platform/network/curl/CurlSSLHandle.h:45
> +class CurlSSLSettings {

We should have one class per file.  This file contains no class called CURLSSLHandle.
Comment 6 Basuke Suzuki 2017-09-15 12:59:45 PDT
Created attachment 320952 [details]
fixed
Comment 7 Basuke Suzuki 2017-09-15 13:01:17 PDT
Fixed. Separate files, use proper name for classes and remove unnecessary comments. Also reduce usage of singleton(), but put them into CurlContext.
Comment 8 Alex Christensen 2017-09-15 13:44:59 PDT
Comment on attachment 320952 [details]
fixed

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:103
> +        auto valueIter = (it->value).begin();
> +        for (; valueIter != (it->value).end(); ++valueIter, ++certsIter) {

it->value should at least be given a name.  This loop can be made to be much more clear.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.h:91
> +

This is a lot of whitespace.  Let's not.
Comment 9 Basuke Suzuki 2017-09-15 15:06:33 PDT
Created attachment 320967 [details]
fixed
Comment 10 Basuke Suzuki 2017-09-15 15:07:18 PDT
(In reply to Alex Christensen from comment #8)
> Comment on attachment 320952 [details]
> fixed
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320952&action=review
> 
> > Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:103
> > +        auto valueIter = (it->value).begin();
> > +        for (; valueIter != (it->value).end(); ++valueIter, ++certsIter) {
> 
> it->value should at least be given a name.  This loop can be made to be much
> more clear.

Replaced with std::equal()

> > Source/WebCore/platform/network/curl/CurlSSLVerifier.h:91
> > +
> 
> This is a lot of whitespace.  Let's not.

removed.
Comment 11 WebKit Commit Bot 2017-09-18 08:41:18 PDT
Comment on attachment 320967 [details]
fixed

Clearing flags on attachment: 320967

Committed r222147: <http://trac.webkit.org/changeset/222147>
Comment 12 WebKit Commit Bot 2017-09-18 08:41:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-09-27 12:27:14 PDT
<rdar://problem/34693307>