Bug 176910 - [Curl] Create a class dedicated to handle SSL related task
Summary: [Curl] Create a class dedicated to handle SSL related task
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-14 08:54 PDT by Basuke Suzuki
Modified: 2017-09-27 12:27 PDT (History)
6 users (show)

See Also:


Attachments
patch (38.76 KB, patch)
2017-09-14 08:59 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
fix (20.11 KB, patch)
2017-09-14 14:54 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
fix (39.83 KB, patch)
2017-09-14 14:59 PDT, Basuke Suzuki
achristensen: review-
Details | Formatted Diff | Diff
fixed (40.65 KB, patch)
2017-09-15 12:59 PDT, Basuke Suzuki
achristensen: review+
Details | Formatted Diff | Diff
fixed (40.36 KB, patch)
2017-09-15 15:06 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff

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