MD Settings: Certificate manager, password encryption dialog.

MD Settings: Certificate manager, password encryption dialog.

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

[dpapad, 2016-03-11 02:51:17]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-03-11 02:51:22]

Patchset #1 (id:20001) has been deleted

[dpapad, 2016-03-11 19:26:19]

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

[dpapad, 2016-03-11 19:26:19]

Dialog screenshots http://imgur.com/a/uaP4O.

FYI, this is CL is part of the plan described at
https://codereview.chromium.org/1772203002#msg2. Previous similar CLs (adding a
dialog)
 - https://codereview.chromium.org/1772203002
 - https://codereview.chromium.org/1784553004

[dschuyler, 2016-03-11 21:18:16]

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:286: <message
name="IDS_SETTINGS_CERTIFICATE_MANAGER_ENCRYPT_PASSWORD_TITLE" desc="Description
of password prompt in certificate manager for exporting a personal certificate">
Suggestion: This IDS is long, would IDS_SETTINGS_ENTER_ENCRYPT_PASSWORD suffice?
(If you're cool with that change, maybe do similar shortenings on the other IDS
being added below).

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:287: Please enter a password to encrypt this
certificate file.
Nit: I would not expect a period at the end of a dialog title.
Nit 2: This reads like a sentence, would a reasonable dialog title be "Create
Password"?

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.html
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.html:30:
<div>$i18n{certificateManagerEncryptPasswordDescription}</div>
Please try styling with with class="row" to make 
the min-height 40px and see if we can remove the
.password-buttons class above.
Maybe make the paper-inputs above class="row" as
well if that helps.  (We may need a two-line row
at 52px for the paper-inputs, maybe).

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.js
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.js:19:
/** @private {string} */
The {string} entry is redundant with type: String below.
(Closure understands basic Polymer property types).

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.js:25:
/** @private {string} */
{string} unneeded

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:193: <structure
name="IDR_SETTINGS_CERTIFICATES_CERTIFICATE_PASSWORD_ENCRYPTION_DIALOG_JS"
Optional: other IDRs seem to just have 
IDR_SETTINGS_ and then an all caps version
of the file name.  Would this be cool for this
IDR:
IDR_SETTINGS_CERTIFICATE_PASSWORD_ENCRYPTION_DIALOG_JS

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:227:
{"certificateManagerConfirmPasswordLabel",
Please try for labels that are close to the
English i18n string.  e.g. "confirmPassword".
If the string particularly long, maybe shorten
to the essence of the sentence.

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

https://codereview.chromium.org/1782113004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/certificate_manager_page_test.js:231:
MockInteractions.keyEventOn(passwordInputElement, 'input', 32);
If this is a space bar key code, please 
make a const to help document what's 
happening.  (Or document it in each place
it's used, but I think it's better documentation
to put var kSpaceBar = 32 above and use that).

[dpapad, 2016-03-14 19:34:50]

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:286: <message
name="IDS_SETTINGS_CERTIFICATE_MANAGER_ENCRYPT_PASSWORD_TITLE" desc="Description
of password prompt in certificate manager for exporting a personal certificate">
On 2016/03/11 at 21:18:15, dschuyler wrote:
> Suggestion: This IDS is long, would IDS_SETTINGS_ENTER_ENCRYPT_PASSWORD
suffice?
> (If you're cool with that change, maybe do similar shortenings on the other
IDS being added below).

I am trying to minimize the chance for conflicts within this file by following
this naming convention

IDS_SETTINGS_<page-namespace>_<string-description>
where page-namespace => CERTIFICATE_MANAGER

Completely removing <page-namespace> from above increases chances of collisions
within MD Settings. How about just shortening CERTIFICATE_MANAGER to CERT?

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:287: Please enter a password to encrypt this
certificate file.
On 2016/03/11 at 21:18:15, dschuyler wrote:
> Nit: I would not expect a period at the end of a dialog title.
> Nit 2: This reads like a sentence, would a reasonable dialog title be "Create
Password"?

Nit1: Done. Although other dialog titles for the same section are using
punctuation (for example see IDS_SETTINGS_CERTIFICATE_MANAGER_DELETE_OTHER_TITLE
which ends in a question mark).

Nit2: Since there are no mocks for those dialogs, I am simply copying the
strings from old Options, for now. I prefer to not spend time altering/improving
the strings until Alan has seen those dialogs and finalizes the strings.

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.html
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.html:30:
<div>$i18n{certificateManagerEncryptPasswordDescription}</div>
On 2016/03/11 at 21:18:15, dschuyler wrote:
> Please try styling with with class="row" to make 
> the min-height 40px and see if we can remove the
> .password-buttons class above.
> Maybe make the paper-inputs above class="row" as
> well if that helps.  (We may need a two-line row
> at 52px for the paper-inputs, maybe).

I tried the suggestion about using CSS class "row".

Class row brings in 3 styles, align-items:center, display:flex, min-height:
40px. Out of these three, only min-height is relevant to the div here. The div
has no children so display flex makes little sense, and center align also does
not apply here (text should be left-aligned).

Similar for adding "row" in the paper-buttons. Is there a way to share only the
useful part of min-height without inheriting unwanted CSS rules (maybe using a
CSS var for min-height)?

Can we leave such polish for later CLs, since the focus for now is on landing
functionality?

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.js
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.js:19:
/** @private {string} */
On 2016/03/11 at 21:18:15, dschuyler wrote:
> The {string} entry is redundant with type: String below.
> (Closure understands basic Polymer property types).

Done.

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.js:25:
/** @private {string} */
On 2016/03/11 at 21:18:15, dschuyler wrote:
> {string} unneeded

Done.

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:193: <structure
name="IDR_SETTINGS_CERTIFICATES_CERTIFICATE_PASSWORD_ENCRYPTION_DIALOG_JS"
On 2016/03/11 at 21:18:15, dschuyler wrote:
> Optional: other IDRs seem to just have 
> IDR_SETTINGS_ and then an all caps version
> of the file name.  Would this be cool for this
> IDR:
> IDR_SETTINGS_CERTIFICATE_PASSWORD_ENCRYPTION_DIALOG_JS

Done.

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:227:
{"certificateManagerConfirmPasswordLabel",
On 2016/03/11 at 21:18:15, dschuyler wrote:
> Please try for labels that are close to the
> English i18n string.  e.g. "confirmPassword".
> If the string particularly long, maybe shorten
> to the essence of the sentence.

loadTimeData is a flat hashmap (no hierarchy). Collisions become more likely by
removing "namespace" information from the name ("certificateManager"), and the
worst part in this case (unlike IDS_FOO) is that there is no compile-time error
if loadTimeData collisions happen. I am trying to avoid that, by prefixing with
certificateManager.

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

https://codereview.chromium.org/1782113004/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/certificate_manager_page_test.js:231:
MockInteractions.keyEventOn(passwordInputElement, 'input', 32);
On 2016/03/11 at 21:18:15, dschuyler wrote:
> If this is a space bar key code, please 
> make a const to help document what's 
> happening.  (Or document it in each place
> it's used, but I think it's better documentation
> to put var kSpaceBar = 32 above and use that).

Done.

[dschuyler, 2016-03-14 21:20:14]

LGTM

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:286: <message
name="IDS_SETTINGS_CERTIFICATE_MANAGER_ENCRYPT_PASSWORD_TITLE" desc="Description
of password prompt in certificate manager for exporting a personal certificate">
On 2016/03/14 19:34:49, dpapad wrote:
> On 2016/03/11 at 21:18:15, dschuyler wrote:
> > Suggestion: This IDS is long, would IDS_SETTINGS_ENTER_ENCRYPT_PASSWORD
> suffice?
> > (If you're cool with that change, maybe do similar shortenings on the other
> IDS being added below).
> 
> I am trying to minimize the chance for conflicts within this file by following
> this naming convention
> 
> IDS_SETTINGS_<page-namespace>_<string-description>
> where page-namespace => CERTIFICATE_MANAGER
> 
> Completely removing <page-namespace> from above increases chances of
collisions
> within MD Settings. How about just shortening CERTIFICATE_MANAGER to CERT?

My suggestion would still be "", instead of "CERTIFICATE_MANGER" or "CERT.  
This is just a suggestion though and won't hold up the LG.

https://codereview.chromium.org/1782113004/diff/80001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:287: Please enter a password to encrypt this
certificate file.
On 2016/03/14 19:34:49, dpapad wrote:
> On 2016/03/11 at 21:18:15, dschuyler wrote:
> > Nit: I would not expect a period at the end of a dialog title.
> > Nit 2: This reads like a sentence, would a reasonable dialog title be
"Create
> Password"?
> 
> Nit1: Done. Although other dialog titles for the same section are using
> punctuation (for example see
IDS_SETTINGS_CERTIFICATE_MANAGER_DELETE_OTHER_TITLE
> which ends in a question mark).
> 
> Nit2: Since there are no mocks for those dialogs, I am simply copying the
> strings from old Options, for now. I prefer to not spend time
altering/improving
> the strings until Alan has seen those dialogs and finalizes the strings.

Acknowledged.

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.html
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificate_password_encryption_dialog.html:30:
<div>$i18n{certificateManagerEncryptPasswordDescription}</div>
On 2016/03/14 19:34:49, dpapad wrote:
> On 2016/03/11 at 21:18:15, dschuyler wrote:
> > Please try styling with with class="row" to make 
> > the min-height 40px and see if we can remove the
> > .password-buttons class above.
> > Maybe make the paper-inputs above class="row" as
> > well if that helps.  (We may need a two-line row
> > at 52px for the paper-inputs, maybe).
> 
> I tried the suggestion about using CSS class "row".
> 
> Class row brings in 3 styles, align-items:center, display:flex, min-height:
> 40px. Out of these three, only min-height is relevant to the div here. The div
> has no children so display flex makes little sense, and center align also does
> not apply here (text should be left-aligned).
> 
> Similar for adding "row" in the paper-buttons. Is there a way to share only
the
> useful part of min-height without inheriting unwanted CSS rules (maybe using a
> CSS var for min-height)?
> 
> Can we leave such polish for later CLs, since the focus for now is on landing
> functionality?

Thank you for trying it.  Yeah, let's revisit this separately.
I'd like to have a common set of classes that we use and avoid
local styling where practical.

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:227:
{"certificateManagerConfirmPasswordLabel",
On 2016/03/14 19:34:50, dpapad wrote:
> On 2016/03/11 at 21:18:15, dschuyler wrote:
> > Please try for labels that are close to the
> > English i18n string.  e.g. "confirmPassword".
> > If the string particularly long, maybe shorten
> > to the essence of the sentence.
> 
> loadTimeData is a flat hashmap (no hierarchy). Collisions become more likely
by
> removing "namespace" information from the name ("certificateManager"), and the
> worst part in this case (unlike IDS_FOO) is that there is no compile-time
error
> if loadTimeData collisions happen. I am trying to avoid that, by prefixing
with
> certificateManager.

Ah, that makes sense.  Maybe just scratch out the "Label" suffix?
Assuming that the prefix is enough to give the context (avoid 
collisions).

[dpapad, 2016-03-15 01:18:17]

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1782113004/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:227:
{"certificateManagerConfirmPasswordLabel",
On 2016/03/14 at 21:20:13, dschuyler wrote:
> On 2016/03/14 19:34:50, dpapad wrote:
> > On 2016/03/11 at 21:18:15, dschuyler wrote:
> > > Please try for labels that are close to the
> > > English i18n string.  e.g. "confirmPassword".
> > > If the string particularly long, maybe shorten
> > > to the essence of the sentence.
> > 
> > loadTimeData is a flat hashmap (no hierarchy). Collisions become more likely
by
> > removing "namespace" information from the name ("certificateManager"), and
the
> > worst part in this case (unlike IDS_FOO) is that there is no compile-time
error
> > if loadTimeData collisions happen. I am trying to avoid that, by prefixing
with
> > certificateManager.
> 
> Ah, that makes sense.  Maybe just scratch out the "Label" suffix?
> Assuming that the prefix is enough to give the context (avoid 
> collisions).

Done. Removed the "Label" suffix.

[dpapad, 2016-03-15 01:18:22]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-15 01:18:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1782113004/#ps120001
(title: "Address comment.")

[commit-bot: I haz the power, 2016-03-15 01:18:41]

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

[commit-bot: I haz the power, 2016-03-15 04:01:35]

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

[commit-bot: I haz the power, 2016-03-15 04:01:36]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dpapad, 2016-03-15 17:06:28]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-15 17:06:46]

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

[commit-bot: I haz the power, 2016-03-15 18:40:13]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-03-15 18:41:39]

Description was changed from

==========
MD Settings: Certificate manager, password encryption dialog.

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

to

==========
MD Settings: Certificate manager, password encryption dialog.

BUG=547073

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

[commit-bot: I haz the power, 2016-03-15 18:41:41]

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