Open Bug 922269 Opened 8 years ago Updated 8 years ago

Fix function comments


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: wtc, Assigned: wtc)



(2 files, 1 obsolete file)

I am using this bug report to fix function comments that don't warrant their
own bug reports.

The first patch updates the "called from ssl3_xxx" comments in ssl3con.c.
ssl3_HandleServerHelloDone now does the bulk of its work in the new
ssl3_SendClientSecondRound function, so the functions that were called by
ssl3_HandleServerHelloDone directly are now called by ssl3_SendClientSecondRound.
Brian, feel free to reject this patch. (The change to add ssl3_HandleFinished
to the list for ssl3_SendNextProto is worth keeping though.)
Attachment #812207 - Flags: review?(brian)
Attachment #812214 - Flags: review?(ryan.sleevi)
Comment on attachment 812214 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList

Review of attachment 812214 [details] [diff] [review]:

r- on styleistic nits, but r+ for adding documentation.

::: lib/certdb/cert.h
@@ +1226,5 @@
>  void
>  CERT_RemoveCertListNode(CERTCertListNode *node);
> +/*
> + * The new cert list node takes ownership of |cert|. |cert| is freed

The use of |cert| is a Chromium-ism.

In NSS, "cert" is used (see lines 106-209, 116-117, 123-124, etc)

@@ +1233,5 @@
>  SECStatus
>  CERT_AddCertToListTail(CERTCertList *certs, CERTCertificate *cert);
> +/*
> + * The new cert list node takes ownership of |cert|. |cert| is freed

Same here

@@ +1240,5 @@
>  SECStatus
>  CERT_AddCertToListHead(CERTCertList *certs, CERTCertificate *cert);
>  SECStatus
>  CERT_AddCertToListTailWithData(CERTCertList *certs, CERTCertificate *cert,

Seems like you should document this
Attachment #812214 - Flags: review?(ryan.sleevi) → review-
Attachment #812207 - Flags: review?(brian) → review+
Comment on attachment 812207 [details] [diff] [review]
Fix the "called from ssl3_xxx" comments in ssl3con.c
Attachment #812207 - Flags: checked-in+
Ryan: please review the new patch. Thanks.

I remember seeing the use of |cert| in Mozilla code and Bugzilla comments
before the Chromium project existed. It has crept into the NSS source tree
because many NSS developers are familiar with this notation from either
Mozilla or Chromium. I think it is a good notation.
Attachment #812214 - Attachment is obsolete: true
Attachment #815120 - Flags: review?(ryan.sleevi)
Attachment #815120 - Flags: review?(ryan.sleevi) → review+
Comment on attachment 815120 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList, v2
Attachment #815120 - Flags: checked-in+
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
You need to log in before you can comment on or make changes to this bug.