gtk: Certificate viewer should show both A-label and U-label for CN and SubjectAltName IDNs

gtk: Certificate viewer should show both A-label and U-label for CN and SubjectAltName IDNs

Remove extraneous '\n' from ProcessGeneralNames.

BUG=43966
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49831

[wtc, 2010-06-11 23:39:22]

mattm: thanks for the CL.  I will review it next Monday.

I cc'ed davidben, a summer intern who will be working on
areas related to client certificates.

[wtc, 2010-06-15 01:36:58]

LGTM.  Sorry about the delay in reviewing this CL.

Can you provide some test URLs for test engineers to verify
the fix manually?

Why do certificate_manager.cc and certificate_viewer.cc
contain no mention of subjectAltName?

http://codereview.chromium.org/2733014/diff/1/5
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (left):

http://codereview.chromium.org/2733014/diff/1/5#oldcode571
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:571: rv += text
+ '\n';
Why did you remove this '\n' character?  It doesn't seem
related to A-label/U-label.

http://codereview.chromium.org/2733014/diff/1/5
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/2733014/diff/1/5#newcode45
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:45: #include
<unicode/uidna.h>
Is this an ICU header?

http://codereview.chromium.org/2733014/diff/1/5#newcode171
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:171: // Convert
the ASCII input to a wide string for ICU.
Nit: should we change "wide string" to "string16"?

To me, a wide string means std::wstring.

http://codereview.chromium.org/2733014/diff/1/5#newcode186
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:186: else if
(status != U_BUFFER_OVERFLOW_ERROR) {
Put '}' and 'else' on the same line.  See also line 189 below.

The code in this file doesn't use the
  }
  else {
formatting style.

http://codereview.chromium.org/2733014/diff/1/5#newcode196
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:196:
DCHECK_EQ(static_cast<size_t>(output_chars), output16.length());
Should we still do
  output16.resize(output_chars);
to be safe?

http://codereview.chromium.org/2733014/diff/1/6
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h (right):

http://codereview.chromium.org/2733014/diff/1/6#newcode84
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h:84: // For host
values, if they contain IDN encoded labels, this will return a
Please clarify in this comment what you mean by "encoded" and
"decoded".  I guess you mean punycode?

Consider working "A-label" and "U-label" into this comment.

[mattm, 2010-06-15 02:42:26]

> Can you provide some test URLs for test engineers to verify 
> the fix manually?

Hrm, I don't know of any test URLs.  I generated and imported a test cert by
hand, and viewed it using the certificate manager (which isn't hooked up in
default builds..)  I could include instructions for generating the test cert,
but since the cert manager isn't hooked up that wouldn't actually help.

> Why do certificate_manager.cc and certificate_viewer.cc
> contain no mention of subjectAltName?

Sorry, updated CL description (it is for CommonName and SubjectAltName)

http://codereview.chromium.org/2733014/diff/1/5
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (left):

http://codereview.chromium.org/2733014/diff/1/5#oldcode571
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:571: rv += text
+ '\n';
On 2010/06/15 01:36:58, wtc wrote:
> Why did you remove this '\n' character?  It doesn't seem
> related to A-label/U-label.

I noticed while testing that there was an extraneous \n here (ProcessGeneralName
already includes one.)  Added note to CL desc.

http://codereview.chromium.org/2733014/diff/1/5
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

http://codereview.chromium.org/2733014/diff/1/5#newcode45
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:45: #include
<unicode/uidna.h>
On 2010/06/15 01:36:58, wtc wrote:
> Is this an ICU header?

yes

http://codereview.chromium.org/2733014/diff/1/5#newcode171
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:171: // Convert
the ASCII input to a wide string for ICU.
On 2010/06/15 01:36:58, wtc wrote:
> Nit: should we change "wide string" to "string16"?
> 
> To me, a wide string means std::wstring.

Done.

http://codereview.chromium.org/2733014/diff/1/5#newcode186
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:186: else if
(status != U_BUFFER_OVERFLOW_ERROR) {
On 2010/06/15 01:36:58, wtc wrote:
> Put '}' and 'else' on the same line.  See also line 189 below.
> 
> The code in this file doesn't use the
>   }
>   else {
> formatting style.

Done.

http://codereview.chromium.org/2733014/diff/1/5#newcode196
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:196:
DCHECK_EQ(static_cast<size_t>(output_chars), output16.length());
On 2010/06/15 01:36:58, wtc wrote:
> Should we still do
>   output16.resize(output_chars);
> to be safe?

Done.

http://codereview.chromium.org/2733014/diff/1/6
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h (right):

http://codereview.chromium.org/2733014/diff/1/6#newcode84
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h:84: // For host
values, if they contain IDN encoded labels, this will return a
On 2010/06/15 01:36:58, wtc wrote:
> Please clarify in this comment what you mean by "encoded" and
> "decoded".  I guess you mean punycode?
> 
> Consider working "A-label" and "U-label" into this comment.

Done.

[wtc, 2010-06-15 18:34:26]

LGTM!

http://codereview.chromium.org/2733014/diff/7001/8005
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h (right):

http://codereview.chromium.org/2733014/diff/7001/8005#newcode84
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h:84: // For host
values, if they contain IDN A-labels, this will return a
It's fine to say "encoded" and "decoded" in this comment.
What I need clarification on is what the encoding format is.
I believe it's Punycode, right?

Saying only "A-label" and "U-label" is precise but makes it
hard for a different set of people.

My suggestion is "Punycode-encoded A-labels" and "decoded U-label".
Do these terms make sense to you?

[mattm, 2010-06-15 20:36:14]

http://codereview.chromium.org/2733014/diff/7001/8005
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h (right):

http://codereview.chromium.org/2733014/diff/7001/8005#newcode84
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.h:84: // For host
values, if they contain IDN A-labels, this will return a
On 2010/06/15 18:34:26, wtc wrote:
> It's fine to say "encoded" and "decoded" in this comment.
> What I need clarification on is what the encoding format is.
> I believe it's Punycode, right?
> 
> Saying only "A-label" and "U-label" is precise but makes it
> hard for a different set of people.
> 
> My suggestion is "Punycode-encoded A-labels" and "decoded U-label".
> Do these terms make sense to you?

Yeah, that makes sense.  Doing that.