Condition the ShowManageSSLCertificates button on operating system.

Condition the ShowManageSSLCertificates button on operating system.

Excluding USE_OPENSSL is no longer appropriate now that Mac is on OpenSSL/BoringSSL.
(It should have been USE_OPENSSL_CERTS.) Switch them to condition on OS instead to
match the condition in browser_options.js.

BUG=417241
TEST=Manage certificates button in Settings opens Keychain on Mac

Committed: https://crrev.com/029701e55b16a7caa74ce010319c21288e4ac26a
Cr-Commit-Position: refs/heads/master@{#296527}

[davidben, 2014-09-24 15:06:37]

davidben@chromium.org changed reviewers:
+ dbeam@chromium.org

[davidben, 2014-09-24 15:06:37]

More settings page stuff, unrelated to JavaScript and back/forward list for a
change.

[Dan Beam, 2014-09-24 18:41:09]

https://codereview.chromium.org/597913004/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (left):

https://codereview.chromium.org/597913004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:704: #if
!defined(USE_NSS) && !defined(USE_OPENSSL)
why not add a USE_BORINGSSL?

[davidben, 2014-09-24 18:50:15]

https://codereview.chromium.org/597913004/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (left):

https://codereview.chromium.org/597913004/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:704: #if
!defined(USE_NSS) && !defined(USE_OPENSSL)
On 2014/09/24 18:41:08, Dan Beam wrote:
> why not add a USE_BORINGSSL?

The problem isn't OpenSSL -> BoringSSL (all references in code still say OpenSSL
right now; BoringSSL is our in-house OpenSSL). It's Mac going NSS ->
OpenSSL/BoringSSL. These flags are kinda awkward and really should be renamed.
They match the GYP flags which are similarly confused. I believe GN's flags were
newer so they get it right. There's:

USE_NSS = port uses NSS for /certificates/. This is currently CrOS and Linux.
USE_OPENSSL = port uses OpenSSL for /all crypto/. If undefined, general crypto
is NSS. This used to be only Android. Now it's Mac as well. Eventually it will
be everything and there won't be any NSS anywhere.
USE_OPENSSL_CERTS = port uses OpenSSL for /certificates/. This is just Android
and the unfinished Linux+OpenSSL port.

So USE_NSS is really USE_NSS_CERTS and what should be USE_NSS is
!defined(USE_OPENSSL).

One fix would be to replace USE_OPENSSL with USE_OPENSSL_CERTS. I can flip it
that way if you prefer. That would work as well. (This code should be built on
Mac.) I opted to flip it to the OS ones because this is the other half to the
showManageSSLCertificates message, and that one is keyed by OS.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

[davidben, 2014-09-24 18:51:37]

Updated CL description to hopefully be less confusing.

[Dan Beam, 2014-09-24 19:23:51]

lgtm, I guess

in general, i've seen way too many cases of a feature only working on 1 platform
even though it will eventually be on all of them.  this code usually starts with
#if defined(OS_FIRST_PLATFORM) and it's bound to be wrong in the near future. 
this is why i prefer USE_* flags or something that has a higher chance of being
correct.

[davidben, 2014-09-24 19:33:43]

On 2014/09/24 19:23:51, Dan Beam wrote:
> lgtm, I guess
> 
> in general, i've seen way too many cases of a feature only working on 1
platform
> even though it will eventually be on all of them.  this code usually starts
with
> #if defined(OS_FIRST_PLATFORM) and it's bound to be wrong in the near future. 
> this is why i prefer USE_* flags or something that has a higher chance of
being
> correct.

(Heh. I've actually had the inverse issue flipping Mac from NSS to OpenSSL. :-)
Some tests were disabled on USE_OPENSSL because they weren't working yet on the
OpenSSL port and there was no indication that they were now needed for feature
parity. Though that is definitely a weird cases.)

*shrug* I'm just as happy with s/USE_OPENSSL/USE_OPENSSL_CERTS/ which would be
the correct USE_* flag. I only went with the OS_* because it matches the
corresponding JS code (which I suppose doesn't have access to USE_*). Arguably
if we grow another platform where we use the platform certs, we will want to go
through this codepath and so the build error is helpful. (But we still wouldn't
get any error reminding us to update the JS side.)

[Dan Beam, 2014-09-24 20:55:41]

On 2014/09/24 19:33:43, David Benjamin wrote:
> On 2014/09/24 19:23:51, Dan Beam wrote:
> > lgtm, I guess
> > 
> > in general, i've seen way too many cases of a feature only working on 1
> platform
> > even though it will eventually be on all of them.  this code usually starts
> with
> > #if defined(OS_FIRST_PLATFORM) and it's bound to be wrong in the near
future. 
> > this is why i prefer USE_* flags or something that has a higher chance of
> being
> > correct.
> 
> (Heh. I've actually had the inverse issue flipping Mac from NSS to OpenSSL.
:-)
> Some tests were disabled on USE_OPENSSL because they weren't working yet on
the
> OpenSSL port and there was no indication that they were now needed for feature
> parity. Though that is definitely a weird cases.)
> 
> *shrug* I'm just as happy with s/USE_OPENSSL/USE_OPENSSL_CERTS/ which would be
> the correct USE_* flag. I only went with the OS_* because it matches the
> corresponding JS code (which I suppose doesn't have access to USE_*). Arguably
> if we grow another platform where we use the platform certs, we will want to
go
> through this codepath and so the build error is helpful. (But we still
wouldn't
> get any error reminding us to update the JS side.)

why wouldn't we inject the same param into grit (js, css, html)?

[davidben, 2014-09-24 21:19:40]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-09-24 21:21:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/597913004/1

[commit-bot: I haz the power, 2014-09-24 21:34:12]

Committed patchset #1 (id:1) as ef84ecca174881073f31247778c4ab4d31a4b180

[commit-bot: I haz the power, 2014-09-24 21:34:54]

Patchset 1 (id:??) landed as
https://crrev.com/029701e55b16a7caa74ce010319c21288e4ac26a
Cr-Commit-Position: refs/heads/master@{#296527}