Closed
Bug 961454
Opened 7 years ago
Closed 7 years ago
PSM peers discuss whitespace too much during code reviews
Categories
(Core :: Security: PSM, enhancement)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: briansmith, Assigned: briansmith)
Details
Attachments
(1 file, 9 obsolete files)
278.23 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
These patches contain: 1. Removed trailing whitespace 2. Replaced "Foo * foo" and "Foo *foo" with "Foo* foo", and same for "&". 3. Return type on its own line. 4. C-style comments replaced with C++-style comments. 5. Unnecessary comments removed. 6. Some reformatting of lines that I changed with the above changes, or near them. 7. Removal of commented-out code.
Attachment #8362178 -
Flags: review?(dkeeler)
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8362179 -
Flags: review?(dkeeler)
Assignee | ||
Comment 3•7 years ago
|
||
Note that it is extremely unlikely that we're going to ever support that (non-standard?) scope-of-use extension, which was intended to restrict the set of websites that a certificate can be used with, so I just deleted all that commented-out/#ifdefed-out code.
Attachment #8362180 -
Flags: review?(dkeeler)
Assignee | ||
Comment 4•7 years ago
|
||
Forgot to amend commit after fixing type.
Attachment #8362180 -
Attachment is obsolete: true
Attachment #8362180 -
Flags: review?(dkeeler)
Attachment #8362181 -
Flags: review?(dkeeler)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8362182 -
Flags: review?(cviecco)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8362183 -
Flags: review?(cviecco)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8362183 -
Attachment is obsolete: true
Attachment #8362183 -
Flags: review?(cviecco)
Attachment #8362185 -
Flags: review?(cviecco)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8362186 -
Flags: review?(cviecco)
Assignee | ||
Comment 9•7 years ago
|
||
The patches I submitted cover the files we modify most often, and they are also the files that will be modified by the insanity::pkix integration. I suggest that for the rest of the files, we clean them up in a lazy fashion: If you modify other PSM files, then feel free to do the whitespace cleanup for *in a separate patch* in the same bug as the patch that makes the semantic changes.
Comment 10•7 years ago
|
||
Comment on attachment 8362182 [details] [diff] [review] ws-4-SSLServerCertVerification.patch Review of attachment 8362182 [details] [diff] [review]: ----------------------------------------------------------------- Great bug name.
Attachment #8362182 -
Flags: review?(cviecco) → review+
Updated•7 years ago
|
Attachment #8362185 -
Flags: review?(cviecco) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8362186 [details] [diff] [review] ws-6-nsNSSCertificate.patch Review of attachment 8362186 [details] [diff] [review]: ----------------------------------------------------------------- r+ with explanations in the bug ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +520,5 @@ > nsMemory::Free(key.data); // SECItem is a 'c' type without a destrutor > return (*aDbKey) ? NS_OK : NS_ERROR_FAILURE; > } > > +NS_IMETHODIMP This is not abut this patch, but should we leave the comment until we constify the input? @@ -830,1 @@ > NS_IMETHODIMP Why remove the comment?
Attachment #8362186 -
Flags: review?(cviecco) → review+
Comment on attachment 8362178 [details] [diff] [review] ws-1-CertVerifier.patch Review of attachment 8362178 [details] [diff] [review]: ----------------------------------------------------------------- Awesome.
Attachment #8362178 -
Flags: review?(dkeeler) → review+
Attachment #8362179 -
Flags: review?(dkeeler) → review+
Comment on attachment 8362181 [details] [diff] [review] ws-3-nsNSSIOLayer.patch Review of attachment 8362181 [details] [diff] [review]: ----------------------------------------------------------------- r- for now because there are still some of the same style issues remaining after this patch (e.g. there appear to be some trailing tabs, some */& still aren't in the right place, and there are still some /* */-style comments). Thanks for doing this, Brian. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +69,2 @@ > getSiteKey(const nsACString & hostName, uint16_t port, > /*out*/ nsCSubstring & key) " & " -> "& " @@ +344,5 @@ > } > > NS_IMETHODIMP > nsNSSSocketInfo::JoinConnection(const nsACString & npnProtocol, > const nsACString & hostname, " & " -> "& " @@ +468,5 @@ > if (isAlreadyShutDown()) > return NS_ERROR_NOT_AVAILABLE; > > if (SECSuccess != SSL_OptionSet(mFd, SSL_SECURITY, true)) > return NS_ERROR_FAILURE; Still some unnecessary whitespace at the end of this line. @@ +1797,5 @@ > ClientAuthDataRunnable(CERTDistNames* caNames, > CERTCertificate** pRetCert, > SECKEYPrivateKey** pRetKey, > nsNSSSocketInfo * info, > + CERTCertificate * serverCert) This line and the one above still do ' * ', not '* '. @@ +1929,4 @@ > goto loser; > } > > /* find valid user cert and key pair */ Still some whitespace at the end of this line. Also, C-style comments here and below. @@ +2326,2 @@ > const char *proxyHost, const char *host, int32_t port, > nsNSSSocketInfo *infoObject) ' *' -> '* '
Attachment #8362181 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 14•7 years ago
|
||
I fixed some more whitespace issues in this patch.
Attachment #8363291 -
Flags: review?(dkeeler)
Comment on attachment 8363291 [details] [diff] [review] ws-3.1-nsNSSIOLayer.patch Review of attachment 8363291 [details] [diff] [review]: ----------------------------------------------------------------- I still found '&' spacing problems in getSiteKey, GetNegotiatedNPN, JoinConnection, SetNPNList, getSocketInfoIfRunning, rememberTolerantAtVersion, rememberIntolerantAtVersion, adjustForTLSIntolerance, CloseSocketAndDestroy, setRenegoUnrestrictedSites, isRenegoUnrestrictedSite, ClientAuthDataRunnable (the constructor and declaration of member variables), ClientAuthDataRunnable::RunOnTargetThread (many local variables), nsSSLIOLayerImportFD, nsSSLIOLayerSetOptions, and nsSSLIOLayerAddToSocket (sslSock). There's trailing tabs in ActivateSSL, as well (and some leading tabs in nsGetUserCertChoice). Also, something strange happened with indentation in ClientAuthDataRunnable::RunOnTargetThread. Let me know if you would rather be doing something else with your time and I can finish this up (or do a follow-up in a separate bug). ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1953,5 @@ > if (privKey) { > if (hasExplicitKeyUsageNonRepudiation(node->cert)) { > privKey = nullptr; > // Not a prefered cert > if (!low_prio_nonrep_cert) // did not yet find a low prio cert braces for this 'if' too? @@ +1963,5 @@ > } > } > keyError = PR_GetError(); > if (keyError == SEC_ERROR_BAD_PASSWORD) { > + // problem with password: bail inconsistent indentation - I suggest the next line be un-indented two spaces. @@ +1979,5 @@ > if (!cert) { > goto noCert; > } > + } else { // Not Auto => ask > + // Get the SSL Certificate nit: don't need two spaces before "Get" @@ +2006,1 @@ > canceled = true; looks like this line needs an indent @@ +2008,1 @@ > nsCOMPtr<nsIX509CertDB> certdb; I think this whole function needs re-indenting from here on... @@ +2120,5 @@ > int32_t CertsToUse; > for (CertsToUse = 0, node = CERT_LIST_HEAD(certList); > !CERT_LIST_END(node, certList) && CertsToUse < nicknames->numnicknames; > node = CERT_LIST_NEXT(node) > + ) { maybe the closing parenthesis and brace can be on the previous line?
Attachment #8363291 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 16•7 years ago
|
||
David, the previous patch (ws-3.1-nsNSSIOlayer.patch) was a "diff -w". That's why the whitespace-only changes didn't show up. The "diff -w" shows that the big indention changes I did were whitespace-only. Please review the nsNSSIOLayer.cpp changes in this folded patch. After we land the insanity::pkix-related changes, we can do other reformatting, especially related to braces, if that helps people. The main intent of these patches is to make the review of the insanity::pkix-related changes easier.
Attachment #8362178 -
Attachment is obsolete: true
Attachment #8362179 -
Attachment is obsolete: true
Attachment #8362181 -
Attachment is obsolete: true
Attachment #8362182 -
Attachment is obsolete: true
Attachment #8362185 -
Attachment is obsolete: true
Attachment #8362186 -
Attachment is obsolete: true
Attachment #8363291 -
Attachment is obsolete: true
Attachment #8363369 -
Flags: review?(dkeeler)
Comment on attachment 8363369 [details] [diff] [review] All patches rolled into one. Review of attachment 8363369 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1745,5 @@ > + ret = pref->GetCharPref("security.default_personal_cert", &mode); > + if (NS_FAILED(ret)) { > + goto loser; > + } > + Splinter says there's still some unnecessary whitespace on otherwise empty lines in this function. Your call if you want to address it now.
Attachment #8363369 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/914e2811cfca
Severity: normal → enhancement
Comment 19•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/914e2811cfca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•