915937 - Cleanup #defines in nsNSSComponent.cpp and change them to static consts

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[:Cykesiopka]

Attachment: bug915937_v1.patch

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 ...| .

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 3

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, was :[e-mail]) 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.

Comment 4

[:Cykesiopka]

Attachment: bug915937_v2.patch

+ Avoid name for constants that are only used in one place

Comment 5

[:Cykesiopka]

Changing summary again as AFAICT any #defines/constants left are already in their most restrictive scope.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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";

Comment 7

[:Cykesiopka]

Attachment: bug915937_v3.patch

+ Fix formatting

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8341447 [details] [diff] [review]
bug915937_v3.patch

Review of attachment 8341447 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65a57bce96ef

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65a57bce96ef