MD Settings: Certificate manager, CA edit trust dialog.

MD Settings: Certificate manager, CA edit trust dialog.

BUG=547073
Committed: https://crrev.com/06cfbbc7d47fe660a38cbd2b4a2ed0bc2cc966e7
Cr-Commit-Position: refs/heads/master@{#380292}

[dpapad, 2016-03-09 02:22:01]

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

[dpapad, 2016-03-09 02:22:01]

Screenshot of the dialog, http://imgur.com/lInpCBL.

The approach I am following for "Certificate Manager" is to
1) Add all components that don't have other dependencies (leaves in the
dependency graph),
   which includes all dialogs (around 4-5 dialogs), in separate CLs along with
   relevant tests.
2) Add <certificate-entry> (which you can see behind the dialog in the
screenshot,
   but is not included in this CL).
3) Lastly I will update certificate-manager-page to put everything together.
   Current contents of certificate_manager_page.html/js are irrelevant (I am
only
   updating as needed to be able to import this file from
cr_settings_browser_tests.js).
4) Polish + ChromeOS only stuff.

This CL is the 1st CL belonging to step 1 in the above list.

[Dan Beam, 2016-03-09 21:46:38]

lgtm

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.html
(right):

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.html:18:
</style>
nit: do you need 2 style tags or can this just be

<style include="settings-shared">
  paper-checkbox {
    display: block;
  }
</style>

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.html:25:
<paper-checkbox id="ssl" checked$=[[trustInfo_.ssl]]>
$= should be =, also should be "[[trustInfo_.ssl]]"
^                              ^                  ^
https://github.com/PolymerElements/iron-checked-element-behavior/blob/master/...

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js
(left):

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:16:
*    </iron-animated-pages>
note: when I removed all @group and @element tags, I left examples and the weird

@fileoverview
description on next line

I would recommend you gain consensus with the settings team first before
departing from consistency

https://codereview.chromium.org/1772203002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/certificate_manager_page_test.js (right):

https://codereview.chromium.org/1772203002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:24:
this.caTrustInfo_ = { ssl: true, email: true, objSign: true };
no spaces between braces

https://codereview.chromium.org/1772203002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:62: });
why can't we just return the result of this .resolve() call?

[dpapad, 2016-03-09 22:33:56]

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.html
(right):

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.html:18:
</style>
On 2016/03/09 at 21:46:37, Dan Beam wrote:
> nit: do you need 2 style tags or can this just be
> 
> <style include="settings-shared">
>   paper-checkbox {
>     display: block;
>   }
> </style>

Done. Using only one <style> tag seems to work.

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/ca_trust_edit_dialog.html:25:
<paper-checkbox id="ssl" checked$=[[trustInfo_.ssl]]>
On 2016/03/09 at 21:46:38, Dan Beam wrote:
> $= should be =, also should be "[[trustInfo_.ssl]]"
> ^                              ^                  ^
>
https://github.com/PolymerElements/iron-checked-element-behavior/blob/master/...

Done.

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js
(left):

https://codereview.chromium.org/1772203002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/certificate_manager_page/certificate_manager_page.js:16:
*    </iron-animated-pages>
On 2016/03/09 at 21:46:38, Dan Beam wrote:
> note: when I removed all @group and @element tags, I left examples and the
weird
> 
> @fileoverview
> description on next line
> 
> I would recommend you gain consensus with the settings team first before
departing from consistency

Reverted this, and started a thread to figure out if there is consensus.

https://codereview.chromium.org/1772203002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/certificate_manager_page_test.js (right):

https://codereview.chromium.org/1772203002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:24:
this.caTrustInfo_ = { ssl: true, email: true, objSign: true };
On 2016/03/09 at 21:46:38, Dan Beam wrote:
> no spaces between braces

Done.

https://codereview.chromium.org/1772203002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/certificate_manager_page_test.js:62: });
On 2016/03/09 at 21:46:38, Dan Beam wrote:
> why can't we just return the result of this .resolve() call?

There are two separate promises being resolved here.
1) The promise that is returned from whenCalled('editCaCertificateTrust'), which
is only used by the test to know when this method was called, and what
parameters were passed to it from the element being tested (so that the test can
assert that the correct params are passed to C++).
2) The promise that is returned at line 63 below, which is used by the Polymer
element being tested. That promise *does not* expect any params in the success
callback, which is why I am not reusing the previous promise here.

[dpapad, 2016-03-09 22:38:09]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-09 22:38:09]

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/1772203002/#ps120001
(title: "Addressing comments.")

[commit-bot: I haz the power, 2016-03-09 22:38:34]

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

[commit-bot: I haz the power, 2016-03-09 23:02:19]

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

[commit-bot: I haz the power, 2016-03-09 23:02:20]

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...)

[dpapad, 2016-03-09 23:52:51]

The CQ bit was checked by dpapad@chromium.org

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

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/1772203002/#ps140001
(title: "Fix presubmit error.")

[commit-bot: I haz the power, 2016-03-09 23:53:29]

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

[commit-bot: I haz the power, 2016-03-10 01:37:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-10 01:38:08]

Description was changed from

==========
MD Settings: Certificate manager, CA edit trust dialog.

BUG=547073
==========

to

==========
MD Settings: Certificate manager, CA edit trust dialog.

BUG=547073

Committed: https://crrev.com/06cfbbc7d47fe660a38cbd2b4a2ed0bc2cc966e7
Cr-Commit-Position: refs/heads/master@{#380292}
==========

[commit-bot: I haz the power, 2016-03-10 01:38:10]

Patchset 8 (id:??) landed as
https://crrev.com/06cfbbc7d47fe660a38cbd2b4a2ed0bc2cc966e7
Cr-Commit-Position: refs/heads/master@{#380292}