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)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: keeler, Unassigned)
References
Details
(Whiteboard: [good first bug][mentor=keeler])
Attachments
(1 file, 2 obsolete files)
7.72 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
$ 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
Updated•10 years ago
|
Assignee: nobody → mozbugs.retornam
Comment 1•10 years ago
|
||
Attached a patch file
Attachment #8376801 -
Flags: review+
Attachment #8376801 -
Flags: checkin+
Reporter | ||
Comment 2•10 years ago
|
||
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-
Reporter | ||
Updated•10 years ago
|
Attachment #8376801 -
Flags: checkin+
Comment 3•10 years ago
|
||
Attachment #8377905 -
Flags: review?(dkeeler)
Reporter | ||
Comment 4•10 years ago
|
||
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-
Comment 5•10 years ago
|
||
Attachment #8377905 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8377936 -
Flags: review?(dkeeler)
Reporter | ||
Comment 6•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Attachment #8376801 -
Attachment is obsolete: true
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a5c0d1a55a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/27a5c0d1a55a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•10 years ago
|
Assignee: mozbugs.retornam → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•