MD Settings: Hooking up native certificate manager.

MD Settings: Hooking up native certificate manager.

BUG=598353
Committed: https://crrev.com/34eb8daa6961a55f9e6455c4abffdc1fbdc2daa6
Cr-Commit-Position: refs/heads/master@{#384353}

[dpapad, 2016-03-30 21:04:28]

Patchset #6 (id:100001) has been deleted

[dpapad, 2016-03-30 21:04:32]

Patchset #5 (id:80001) has been deleted

[dpapad, 2016-03-30 21:04:36]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-03-30 21:04:39]

Patchset #3 (id:40001) has been deleted

[dpapad, 2016-03-30 21:04:43]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-03-30 21:04:50]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-03-30 21:40:23]

Description was changed from

==========
MD Settings: Hooking up native certificate manager, WIP.

BUG=598353
==========

to

==========
MD Settings: Hooking up native certificate manager.

BUG=598353
==========

[dpapad, 2016-03-30 21:54:46]

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

[dpapad, 2016-03-30 21:54:46]

Green tests at patch 1.

[dpapad, 2016-03-30 21:57:54]

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

[dpapad, 2016-03-30 21:57:54]

+msw for chrome/browser/ui/BUILD.gn

[dpapad, 2016-03-30 23:52:03]

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.html:80: <if
expr="use_nss_certs or is_win or is_macosx">
Fixed.

[Dan Beam, 2016-03-31 00:20:57]

i would do

if use_nss
if mac or windows

OR

if use_nss
else if mac or windows

consistently everywhere, IMO

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/resourc...
File
chrome/browser/resources/settings/privacy_page/privacy_page_browser_proxy.js
(right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page_browser_proxy.js:13:
showManageSSLCertificates: function() {},
arguable nit: SSL -> Ssl

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_resources.grd:478: <if expr="(not
use_nss_certs) and (is_win or is_macosx)">
nit: maybe just "is_win or is_macosx"

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:375: }
nit:

if (use_nss_certs) {
  ...
}
if (is_mac || is_win) {
  ...
}

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.cc (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.cc:12: 
remove \n

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.h (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.h:26: 
remove \n

https://codereview.chromium.org/1838273003/diff/160001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2814: 'chrome_browser_ui_non_nss_sources': [
nit: chrome_browser_ui_native_certificates_sources ?

[Dan Beam, 2016-03-31 00:24:41]

eh, these are all just nits

lgtm with consistency :)

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.html:14: <if
expr="use_nss_certs">
note: there actually is an <else> if you want to use it, btw

<if expr="use_nss_certs">
  <then>
    ...
  </then>
  <else><if expr="is_win or is_macosx">
      ...
  </if></else>
</if>

it's a little verbose, though... (also don't know if there's a better way to do
elseif)

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:44: #elif  defined(OS_WIN) ||
 defined(OS_MACOSX)
nit: too many spaces

[dpapad, 2016-03-31 01:15:03]

Tried to keep consistency as much as possible.

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_resources.grd:478: <if expr="(not
use_nss_certs) and (is_win or is_macosx)">
On 2016/03/31 at 00:20:57, Dan Beam wrote:
> nit: maybe just "is_win or is_macosx"

Done.

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:375: }
On 2016/03/31 at 00:20:57, Dan Beam wrote:
> nit:
> 
> if (use_nss_certs) {
>   ...
> }
> if (is_mac || is_win) {
>   ...
> }

Done.

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.cc (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.cc:12: 
On 2016/03/31 at 00:20:57, Dan Beam wrote:
> remove \n

Done.

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.h (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.h:26: 
On 2016/03/31 at 00:20:57, Dan Beam wrote:
> remove \n

Done.

https://codereview.chromium.org/1838273003/diff/160001/chrome/chrome_browser_...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1838273003/diff/160001/chrome/chrome_browser_...
chrome/chrome_browser_ui.gypi:2814: 'chrome_browser_ui_non_nss_sources': [
On 2016/03/31 at 00:20:57, Dan Beam wrote:
> nit: chrome_browser_ui_native_certificates_sources ?

I prefer the "non_nss_sources" over "native_certificates_sources", because it is
not clear that "native certificates" is a complement of "nss certificates",
whereas "non nss" clearly communicates that it is a complement.

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.html:14: <if
expr="use_nss_certs">
On 2016/03/31 at 00:24:40, Dan Beam wrote:
> note: there actually is an <else> if you want to use it, btw
> 
> <if expr="use_nss_certs">
>   <then>
>     ...
>   </then>
>   <else><if expr="is_win or is_macosx">
>       ...
>   </if></else>
> </if>
> 
> it's a little verbose, though... (also don't know if there's a better way to
do elseif)

As discussed, the this only seems to work for grd files, but not for HTML.

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1838273003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:44: #elif  defined(OS_WIN) ||
 defined(OS_MACOSX)
On 2016/03/31 at 00:24:40, Dan Beam wrote:
> nit: too many spaces

Fixed.

[dpapad, 2016-03-31 01:17:03]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-31 01:17:03]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1838273003/#ps200001
(title: "Addressing comments")

[commit-bot: I haz the power, 2016-03-31 01:17:20]

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

[commit-bot: I haz the power, 2016-03-31 01:26:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 01:26:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Dan Beam, 2016-03-31 04:22:33]

still lgtm, this seems fine to me

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:44: #elif  defined(OS_WIN) ||
defined(OS_MACOSX)
still too many spaces before OS_WIN

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:58: #elif  defined(OS_WIN) ||
defined(OS_MACOSX)
and here

[msw, 2016-03-31 17:33:25]

lgtm modulo existing nits and double-checking my comment/question.

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.cc (right):

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.cc:20:
web_ui()->RegisterMessageCallback(
I kinda surprised this doesn't conflict with
chrome/browser/ui/webui/options/browser_options_handler.cc:779, but I trust that
it works for you...

[Dan Beam, 2016-03-31 17:41:32]

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.cc (right):

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.cc:20:
web_ui()->RegisterMessageCallback(
On 2016/03/31 17:33:25, msw wrote:
> I kinda surprised this doesn't conflict with
> chrome/browser/ui/webui/options/browser_options_handler.cc:779, but I trust
that
> it works for you...

/settings/ and /options/ are mutually exclusive (they'll never be running at the
same time or even in the same context/page)

[dpapad, 2016-03-31 17:44:11]

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:44: #elif  defined(OS_WIN) ||
defined(OS_MACOSX)
On 2016/03/31 at 04:22:33, Dan Beam wrote:
> still too many spaces before OS_WIN

Done.

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:58: #elif  defined(OS_WIN) ||
defined(OS_MACOSX)
On 2016/03/31 at 04:22:33, Dan Beam wrote:
> and here

Done.

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/native_certificates_handler.cc (right):

https://codereview.chromium.org/1838273003/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/native_certificates_handler.cc:20:
web_ui()->RegisterMessageCallback(
On 2016/03/31 at 17:33:25, msw wrote:
> I kinda surprised this doesn't conflict with
chrome/browser/ui/webui/options/browser_options_handler.cc:779, but I trust that
it works for you...

It works because browser_options_handler.cc is used for chrome://settings and
this one is used for chrome://md-settings, they never co-exist.

[dpapad, 2016-03-31 17:44:15]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-31 17:44:16]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1838273003/#ps220001
(title: "Spaces.")

[commit-bot: I haz the power, 2016-03-31 17:44:49]

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

[commit-bot: I haz the power, 2016-03-31 19:06:07]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2016-03-31 19:07:35]

Description was changed from

==========
MD Settings: Hooking up native certificate manager.

BUG=598353
==========

to

==========
MD Settings: Hooking up native certificate manager.

BUG=598353

Committed: https://crrev.com/34eb8daa6961a55f9e6455c4abffdc1fbdc2daa6
Cr-Commit-Position: refs/heads/master@{#384353}
==========

[commit-bot: I haz the power, 2016-03-31 19:07:37]

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