Closed Bug 970614 Opened 10 years ago Closed 10 years ago

remove code wrapped in #if 0 ... #endif blocks in PSM

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: keeler, Unassigned)

References

Details

(Whiteboard: [good first bug][mentor=keeler])

Attachments

(1 file, 2 obsolete files)

$ grep -rn '#if 0' security/manager/
security/manager/ssl/src/nsCMSSecureMessage.cpp:170:#if 0
security/manager/ssl/src/nsPKCS12Blob.cpp:240:#if 0
security/manager/ssl/src/nsPKCS12Blob.cpp:347:#if 0
security/manager/ssl/src/nsPKCS12Blob.cpp:805:#if 0
security/manager/ssl/src/nsUsageArrayHelper.cpp:223:#if 0
security/manager/ssl/src/nsUsageArrayHelper.cpp:232:#if 0
security/manager/ssl/src/nsUsageArrayHelper.cpp:238:#if 0
security/manager/ssl/src/nsPKCS12Blob.h:42:#if 0
security/manager/ssl/src/nsNSSCertificateDB.cpp:96:#if 0
security/manager/ssl/src/nsNSSCertificateDB.cpp:191:#if 0
Assignee: nobody → mozbugs.retornam
Attached patch Patch File (obsolete) — Splinter Review
Attached a patch file
Attachment #8376801 - Flags: review+
Attachment #8376801 - Flags: checkin+
Comment on attachment 8376801 [details] [diff] [review]
Patch File

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

Hi Karthik - thanks for the patch.
Couple of items, in case you didn't know:
* If a bug is assigned to someone, it's probably a good idea to check with them before working on a patch, because you might be duplicating work.
* When you want someone to review a patch, set the review flag to "?" and indicate who you want to review the patch in the text box that appears. Usually it will be a module owner or peer (see https://wiki.mozilla.org/Modules/All ). When the bug has "mentor=someone" in the whiteboard, that's a good indication that that person should review the patch.
* The checkin flag is not used until after a patch has been reviewed.
* The commit message for your patch should be something like "bug 970614 - remove code wrapped in #if 0 ... #endif blocks in PSM"
* If a patch gets "r-", that means there are some things you should change or address. Once done, you can attach a new patch and request another review.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +184,1 @@
>    certList = PK11_ListCerts(pk11type, nullptr);

We can do away with the pk11type variable entirely since it's only used once (just pass PK11CertListUnique to PK11_ListCerts here).

::: security/manager/ssl/src/nsPKCS12Blob.cpp
@@ -357,2 @@
>    for (i=0; i<numCerts; i++) {
>  //    nsNSSCertificate *cert = reinterpret_cast<nsNSSCertificate *>(certs[i]);

Let's get rid of this commented-out line as well.
Attachment #8376801 - Flags: review+ → review-
Attached patch bug-8376801.patch (obsolete) — Splinter Review
Attachment #8377905 - Flags: review?(dkeeler)
Comment on attachment 8377905 [details] [diff] [review]
bug-8376801.patch

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

Thanks for working on this, Raymond. Please coordinate with Karthik so we don't do more work than necessary here.

::: security/manager/ssl/src/nsPKCS12Blob.cpp
@@ +236,5 @@
>    SECITEM_ZfreeItem(&unicodePw, false);
>    return NS_OK;
>  }
>  
> +

nit: this will result in three empty lines here - we only need one between the end of the previous function and the start of the next.

@@ +310,2 @@
>    for (i=0; i<numCerts; i++) {
>  //    nsNSSCertificate *cert = reinterpret_cast<nsNSSCertificate *>(certs[i]);

Like I said with the other patch, let's get rid of this commented-out line as well.

@@ +754,5 @@
>        /* pop a dialog saying the cert is already in the database */
>        /* ask to keep going?  what happens if one collision but others ok? */
>        // The following errors cannot be "handled", notify the user (via an alert)
>        // that the operation failed.
> +    break;

This patch should only consist of non-functional changes. It may have been incorrect, but as it was this branch of the switch statement didn't have a break in it, so we should leave it out for now. If you feel like it, file a follow-up bug to investigate what the intent here was.
Attachment #8377905 - Flags: review?(dkeeler) → review-
Attached patch bug-970614.patchSplinter Review
Attachment #8377905 - Attachment is obsolete: true
Attachment #8377936 - Flags: review?(dkeeler)
Comment on attachment 8377936 [details] [diff] [review]
bug-970614.patch

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

Awesome. r=me.
Attachment #8377936 - Flags: review?(dkeeler) → review+
Attachment #8376801 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/27a5c0d1a55a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee: mozbugs.retornam → nobody
Blocks: 152941
You need to log in before you can comment on or make changes to this bug.