Closed
Bug 915937
Opened 11 years ago
Closed 11 years ago
Cleanup #defines in nsNSSComponent.cpp and change them to static consts
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
7.28 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Not entirely sure if this was what you wanted, but I moved defines that are only used in one place to the most restrictive scope, and changed all the defines to |static const ...| .
Attachment #805728 -
Flags: review?(brian)
Comment 2•11 years ago
|
||
Comment on attachment 805728 [details] [diff] [review] bug915937_v1.patch Review of attachment 805728 [details] [diff] [review]: ----------------------------------------------------------------- Note that I am likely going to bitrot this patch tomorrow pretty badly. So, if you can't redo the patch right away you might want to wait a couple of days to update it. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +896,5 @@ > + static const bool OCSP_REQUIRED_DEFAULT = false; > + static const bool FRESH_REVOCATION_REQUIRED_DEFAULT = false; > + static const bool MISSING_CERT_DOWNLOAD_DEFAULT = false; > + static const char* const FIRST_REVO_METHOD_DEFAULT = "ocsp"; > + static const bool OCSP_STAPLING_ENABLED_DEFAULT = true; Great. But, if these are only used in one place, then you can just avoid the name completely. For example, you can do: bool crlDownloading = Preferences::GetBool("security.CRL_download.enabled", false); We only need a named constant for things that are used in more than one spot.
Attachment #805728 -
Flags: review?(brian) → review-
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #2) > Note that I am likely going to bitrot this patch tomorrow pretty badly. So, > if you can't redo the patch right away you might want to wait a couple of > days to update it. Ok, thanks for telling me. I'm in no hurry, so I'll just wait for a while. > We only need a named constant for things that are used in more than one spot. Sure.
Summary: Change pre-existing #defines in nsNSSComponent to static consts → Move pre-existing #defines in nsNSSComponent to most restrictive scope, and change them to static consts
Assignee | ||
Comment 4•11 years ago
|
||
+ Avoid name for constants that are only used in one place
Assignee: nobody → cykesiopka.bmo
Attachment #805728 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8341435 -
Flags: review?(brian)
Assignee | ||
Comment 5•11 years ago
|
||
Changing summary again as AFAICT any #defines/constants left are already in their most restrictive scope.
Summary: Move pre-existing #defines in nsNSSComponent to most restrictive scope, and change them to static consts → Cleanup #defines in nsNSSComponent.cpp and change them to static consts
Comment 6•11 years ago
|
||
Comment on attachment 8341435 [details] [diff] [review] bug915937_v2.patch Review of attachment 8341435 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but please fix the formatting issues below. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1012,5 @@ > int32_t ocspEnabled = Preferences::GetInt("security.OCSP.enabled", > OCSP_ENABLED_DEFAULT); > > bool ocspRequired = Preferences::GetBool("security.OCSP.require", > + false); Please fix the wrapping so that these wrap at 80 chars but do not wrap unnecessarily. @@ +1618,5 @@ > +static const char* const PROFILE_CHANGE_NET_TEARDOWN_TOPIC = "profile-change-net-teardown"; > +static const char* const PROFILE_CHANGE_NET_RESTORE_TOPIC = "profile-change-net-restore"; > +static const char* const PROFILE_CHANGE_TEARDOWN_TOPIC = "profile-change-teardown"; > +static const char* const PROFILE_BEFORE_CHANGE_TOPIC = "profile-before-change"; > +static const char* const PROFILE_DO_CHANGE_TOPIC = "profile-do-change"; Please wrap these at column 80, e.g.: static const char* const PROFILE_CHANGE_NET_TEARDOWN_TOPIC = "profile-change-net-teardown";
Attachment #8341435 -
Flags: review?(brian) → review-
Assignee | ||
Comment 7•11 years ago
|
||
+ Fix formatting
Attachment #8341435 -
Attachment is obsolete: true
Attachment #8341447 -
Flags: review?(brian)
Comment 8•11 years ago
|
||
Comment on attachment 8341447 [details] [diff] [review] bug915937_v3.patch Review of attachment 8341447 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch.
Attachment #8341447 -
Flags: review?(brian) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/65a57bce96ef
Keywords: checkin-needed
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65a57bce96ef
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•