Closed Bug 1092398 Opened 7 years ago Closed 6 years ago

remove unused CertVerifier enums (missing_cert_download_config and crl_download_config)


(Core :: Security: PSM, defect)

Not set



Tracking Status
firefox38 --- fixed


(Reporter: keeler, Assigned: KK)


(Whiteboard: good first bug)


(1 file, 3 obsolete files)

missing_cert_download_config and crl_download_config appear to be unused and unnecessary in CertVerifier.
I think it would also be nice to rename ocsp_download_config and related enums to use InterCaps instead of underscores.
Hi, I intersting in solving this bug.

I've found the path where CertVerifier.cpp/.h is and the enums you mentioned.
So my job here is:
1. remove the enums that don't use anymore
2. rename some enum name
Am I right?
Attached patch rename.patch (obsolete) — Splinter Review
remove useless enums and rename other enums from underscore to interCap
Attachment #8536505 - Flags: review?(honzab.moz)
Comment on attachment 8536505 [details] [diff] [review]

Review of attachment 8536505 [details] [diff] [review]:

::: security/certverifier/CertVerifier.cpp
@@ +189,5 @@
>      : !mOCSPStrict              ? NSSCertDBTrustDomain::FetchOCSPForDVSoftFail
>                                  : NSSCertDBTrustDomain::FetchOCSPForDVHardFail;
> +  ocspGetConfig ocspGETConfig = mOCSPGETEnabled ? ocspGetEnabled
> +                                                  : ocspGetDisabled;

fix indention (? and : must be on the same column)

::: security/certverifier/CertVerifier.h
@@ +55,5 @@
>    };
> +  enum ocspDownloadConfig { ocspOff = 0, ocspOn };
> +  enum ocspStrictConfig { ocspRelaxed = 0, ocspStrict };
> +  enum ocspGetConfig { ocspGetDisabled = 0, ocspGetEnabled = 1 };

Start with a capital later.
Attachment #8536505 - Flags: review?(honzab.moz) → review-
Attached patch fixIndention.patch (obsolete) — Splinter Review
Hi, I've fix the indentation and letter
Attachment #8548237 - Flags: review?(honzab.moz)
Comment on attachment 8548237 [details] [diff] [review]

Review of attachment 8548237 [details] [diff] [review]:

::: security/certverifier/CertVerifier.h
@@ +12,5 @@
>  #include "ScopedNSSTypes.h"
>  namespace mozilla { namespace psm {
> +  struct ChainValidationCallbackState;

sorry, it's not clear but we don't indent inside namespaces
please remove this indention
Attachment #8548237 - Flags: review?(honzab.moz)
Attached patch remove possible defects (obsolete) — Splinter Review
Sorry for this late, I spent some time learn about Mercurail and read some articles about contribution.
Attachment #8536505 - Attachment is obsolete: true
Attachment #8548237 - Attachment is obsolete: true
Attachment #8553252 - Flags: review?(honzab.moz)
I forgot to rebuild with my last patch so I  rebuild it and modified some other code that use these enums.

Also I've run some tests with "./mach mochitest security/manager/ssl/tests/mochitest", the suggestion from IRC. I think there's no trouble while doing  tests (it end normally and print result looks fine) but I don't really understand the test result. Hope I didn't miss something important.
Attachment #8553252 - Attachment is obsolete: true
Attachment #8553252 - Flags: review?(honzab.moz)
Attachment #8553725 - Flags: review?(honzab.moz)
Comment on attachment 8553725 [details] [diff] [review]

Review of attachment 8553725 [details] [diff] [review]:

Attachment #8553725 - Flags: review?(honzab.moz) → review+
Keywords: checkin-needed
Can we please run this through Try for a quick sanity check? :)
Assignee: nobody → thumbd03803
Keywords: checkin-needed
Hi, before I push this patch to try server, I need one voucher to get  permission.
Could you help me?

here's the link:
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.