Add a serial number column to the certificate selector dialog and fix its size.

Add a serial number column to the certificate selector dialog and fix its size.

BUG=580805
Committed: https://crrev.com/c5c6dc3337419dc90dcb10ac083f134d3ef78041
Cr-Commit-Position: refs/heads/master@{#390824}

[Ryan Sleevi, 2016-04-28 01:48:50]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-04-28 01:48:51]

It's actually probably a bug that you can call x509_certificate_model from OS X

It really is only supposed to be for UI on Linux/ChromeOS.

[Ryan Sleevi, 2016-04-28 01:51:13]

On 2016/04/28 01:48:51, Ryan Sleevi wrote:
> It's actually probably a bug that you can call x509_certificate_model from OS
X
> 
> It really is only supposed to be for UI on Linux/ChromeOS.

Oh, I missed that this is views. But either way, it's correct that, as somewhat
noted here, it's really only used by Linux -
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/net/...

(We should have guarded the entire file with the USE_NSS_CERTS/USE_OPENSSL_CERTS
ifdefs to make this clearer, neither of which are set on Windows/Mac)

[meacer, 2016-04-28 21:49:56]

On 2016/04/28 01:51:13, Ryan Sleevi wrote:
> On 2016/04/28 01:48:51, Ryan Sleevi wrote:
> > It's actually probably a bug that you can call x509_certificate_model from
OS
> X
> > 
> > It really is only supposed to be for UI on Linux/ChromeOS.
> 
> Oh, I missed that this is views. But either way, it's correct that, as
somewhat
> noted here, it's really only used by Linux -
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/net/...
> 
> (We should have guarded the entire file with the
USE_NSS_CERTS/USE_OPENSSL_CERTS
> ifdefs to make this clearer, neither of which are set on Windows/Mac)

Taking a shot at it here: https://codereview.chromium.org/1930173002/
Should it #error on unsupported platforms, or simply have no effect?

[Ryan Sleevi, 2016-04-28 22:13:00]

https://codereview.chromium.org/1918143005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/1918143005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:98: row.serial =
base::UTF8ToUTF16(BytesToHexString(cert->serial_number()));
You should actually be able to use this method on all platforms.

I'm not even sure why x509_certificate_model has alternative text for
GetSerialNumberHexified, since it must ALWAYS be present.

[meacer, 2016-04-29 00:20:03]

https://codereview.chromium.org/1918143005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/1918143005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:98: row.serial =
base::UTF8ToUTF16(BytesToHexString(cert->serial_number()));
On 2016/04/28 22:13:00, Ryan Sleevi wrote:
> You should actually be able to use this method on all platforms.
> 
> I'm not even sure why x509_certificate_model has alternative text for
> GetSerialNumberHexified, since it must ALWAYS be present.

Done.

[Ryan Sleevi, 2016-04-29 00:30:29]

LGTM with pedantry

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (left):

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:153: 
Eh, I thought this helped readability, but *shrug*

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:75: std::string result;
pedantry: std::string result(bytes.size() * 3, '0') ?

(Mostly, just so you've got enough space to avoid reallocations; should * 3
overflow, which it won't, because we cap serial size anyways, then at worst,
you'd just devolve into the normal growth)

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:95: row.serial =
base::UTF8ToUTF16(BytesToHexString(cert->serial_number()));
I leave the UI in your fully capable hands, but there's always
base::strings::HexEncode (which doesn't insert the colons). But I'm entirely
ambivalent.

[meacer, 2016-04-29 17:39:41]

meacer@chromium.org changed reviewers:
+ msw@chromium.org

[meacer, 2016-04-29 17:39:42]

msw: Can you PTAL? Thanks.

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (left):

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:153: 
On 2016/04/29 00:30:29, Ryan Sleevi wrote:
> Eh, I thought this helped readability, but *shrug*

Not intentional, fixed locally but added back when I patched to Windows and
patched back to Linux from there.

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:75: std::string result;
On 2016/04/29 00:30:29, Ryan Sleevi wrote:
> pedantry: std::string result(bytes.size() * 3, '0') ?
> 
> (Mostly, just so you've got enough space to avoid reallocations; should * 3
> overflow, which it won't, because we cap serial size anyways, then at worst,
> you'd just devolve into the normal growth)

Removed in favor of HexEncode.

https://codereview.chromium.org/1918143005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:95: row.serial =
base::UTF8ToUTF16(BytesToHexString(cert->serial_number()));
On 2016/04/29 00:30:29, Ryan Sleevi wrote:
> I leave the UI in your fully capable hands, but there's always
> base::strings::HexEncode (which doesn't insert the colons). But I'm entirely
> ambivalent.

I searched for combinations of (Binary,Hex,String) but forgot about HexEncode.
Using that.

[msw, 2016-04-29 18:04:54]

lgtm with nits (and deferring on encoding details)

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:25: 
nit: remove

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:80: row.serial =
base::UTF8ToUTF16(base::HexEncode(
nit: maybe ensure the size is less than (std::numeric_limits<size_t>::max() /
2), as mentioned by HexEncode's function comment?

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:81:
cert->serial_number().data(), cert->serial_number().size()));
I know absolutely nothing about DER encoding or hex encoding DER encoded values
and presenting that as a "serial number" string... I'll defer to Ryan there...

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector_browsertest.cc (right):

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:115:
EXPECT_EQ(base::UTF8ToUTF16(""),
nit: use base::string16()

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:124:
EXPECT_EQ(base::UTF8ToUTF16(""),
ditto

[meacer, 2016-04-29 21:50:53]

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:25: 
On 2016/04/29 18:04:54, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector.cc:80: row.serial =
base::UTF8ToUTF16(base::HexEncode(
On 2016/04/29 18:04:54, msw wrote:
> nit: maybe ensure the size is less than (std::numeric_limits<size_t>::max() /
> 2), as mentioned by HexEncode's function comment?

Ryan mentioned earlier that serial lengths are limited, but done anyways.

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector_browsertest.cc (right):

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:115:
EXPECT_EQ(base::UTF8ToUTF16(""),
On 2016/04/29 18:04:54, msw wrote:
> nit: use base::string16()

Done.

https://codereview.chromium.org/1918143005/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:124:
EXPECT_EQ(base::UTF8ToUTF16(""),
On 2016/04/29 18:04:54, msw wrote:
> ditto

Done.

[meacer, 2016-04-29 22:14:06]

The CQ bit was checked by meacer@chromium.org

[meacer, 2016-04-29 22:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1918143005/#ps100001
(title: "msw comments")

[commit-bot: I haz the power, 2016-04-29 22:14:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1918143005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1918143005/100001

[commit-bot: I haz the power, 2016-04-30 00:47:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:30:38]

Patchset 6 (id:??) landed as
https://crrev.com/c5c6dc3337419dc90dcb10ac083f134d3ef78041
Cr-Commit-Position: refs/heads/master@{#390824}

[commit-bot: I haz the power, 2016-04-30 18:03:48]

Description was changed from

==========
Add a serial number column to the certificate selector dialog and fix its size.

BUG=580805
==========

to

==========
Add a serial number column to the certificate selector dialog and fix its size.

BUG=580805

Committed: https://crrev.com/c5c6dc3337419dc90dcb10ac083f134d3ef78041
Cr-Commit-Position: refs/heads/master@{#390824}
==========