MD Settings: Certificate manager, error dialog.

MD Settings: Certificate manager, error dialog.

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

[dpapad, 2016-03-17 00:59:34]

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

[dpapad, 2016-03-17 00:59:35]

Screenshot example at http://imgur.com/rYJCdIz.

Similar to previous CLs, I am adding the dialog, but it is not hooked up yet.
Also unlike previous CLs, this dialog is pretty simple (does not make any calls
to a browser proxy) so could not find anything worth unit-testing. In follow up
CLs though, when this dialog is hooked up, I will add tests to make sure it is
indeed shown when an error occurs.

[tommycli, 2016-03-17 19:14:19]

lgtm sans below. I have a larger question about the dialog usage model, but I
don't think it needs to block this CL.

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.html
(right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.html:21:
<paper-button id="ok" class="action-button" on-tap="close_">
This is generally called onOkTap_?

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js
(right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:7:
* The user can only close the dialog, there is no other possible intercation.
nit: possible interaction.

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:19:
this.$.dialog.open();
Not a big fan of the open on attached model. It seems a bit 'magical' to me.

I realize this is consistent with the already-approved
certificate-delete-confirmation-dialog. I'd still prefer an explicit open() fn
though. This can be refactored in a separate CL if you agree with me.

Additionally? What's the usage model for this on-attached open dialog? Are you
planning to dynamically create and delete this element?

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:23:
close_: function() {
nit: as mentioned before, probably rename to onOkTap_

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/certificates_handler.cc (right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/certificates_handler.cc:114:
IDS_SETTINGS_CERTIFICATE_MANAGER_ERROR_NOT_CA);
All the string IDS changes are the only non-whitespace changes right?

[dpapad, 2016-03-17 22:19:45]

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.html
(right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.html:21:
<paper-button id="ok" class="action-button" on-tap="close_">
On 2016/03/17 at 19:14:19, tommycli wrote:
> This is generally called onOkTap_?

Done.

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js
(right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:7:
* The user can only close the dialog, there is no other possible intercation.
On 2016/03/17 at 19:14:19, tommycli wrote:
> nit: possible interaction.

Done.

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:19:
this.$.dialog.open();
On 2016/03/17 at 19:14:19, tommycli wrote:
> Not a big fan of the open on attached model. It seems a bit 'magical' to me.
> 
> I realize this is consistent with the already-approved
certificate-delete-confirmation-dialog. I'd still prefer an explicit open() fn
though. This can be refactored in a separate CL if you agree with me.
> 
> Additionally? What's the usage model for this on-attached open dialog? Are you
planning to dynamically create and delete this element?

The usage of model of those dialogs will become clear in next CL (you can
already see it at https://codereview.chromium.org/1745713002). In short, I don't
see a value of manually calling open() in this usage model.

More specifically (referring to next CL, certificate_entry.html/js):
 - There are 5 dialogs that initially are NOT in the DOM, using dom-if. For
performance reasons, it makes little sense to have 5 dialogs per certificate
entry rendered and invisible, so dom-if is necessary.
 - Dialogs are dom-ifed to true, only when they need to be shown, so calling
open() on attach() is one way of achieving that. Manually calling open() right
after flipping the dom-if boolean to true is another way to do this, but seems
unnecessary.
 - Once the dialog is closed, it is immediately removed from the DOM, using the
iron-overlay-closed listener.

I don't see any value of the following tri-state
1) Dialog not in the DOM
2) Dialog in the DOM and not visible (open() has not been called, or close() was
called).
3) Dialog in the DOM and visible (open() has been called) 

So my usage pattern reduces the above 3 states to only 2
1) Dialog in the DOM and visible
2) Dialog not in the DOM (therefore not visible)

I am open to discussion on this usage pattern. It seems that discussion is
better suited for the follow up CL, as you guessed.

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:23:
close_: function() {
On 2016/03/17 at 19:14:19, tommycli wrote:
> nit: as mentioned before, probably rename to onOkTap_

Done.

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/certificates_handler.cc (right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/certificates_handler.cc:114:
IDS_SETTINGS_CERTIFICATE_MANAGER_ERROR_NOT_CA);
On 2016/03/17 at 19:14:19, tommycli wrote:
> All the string IDS changes are the only non-whitespace changes right?

Yes. Basically I replaced IDS_CERT with IDS_SETTINGS_CERTIFICATE, and then I ran
clang-format.

[tommycli, 2016-03-17 22:37:42]

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js
(right):

https://codereview.chromium.org/1812673002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js:19:
this.$.dialog.open();
On 2016/03/17 22:19:45, dpapad wrote:
> On 2016/03/17 at 19:14:19, tommycli wrote:
> > Not a big fan of the open on attached model. It seems a bit 'magical' to me.
> > 
> > I realize this is consistent with the already-approved
> certificate-delete-confirmation-dialog. I'd still prefer an explicit open() fn
> though. This can be refactored in a separate CL if you agree with me.
> > 
> > Additionally? What's the usage model for this on-attached open dialog? Are
you
> planning to dynamically create and delete this element?
> 
> The usage of model of those dialogs will become clear in next CL (you can
> already see it at https://codereview.chromium.org/1745713002). In short, I
don't
> see a value of manually calling open() in this usage model.
> 
> More specifically (referring to next CL, certificate_entry.html/js):
>  - There are 5 dialogs that initially are NOT in the DOM, using dom-if. For
> performance reasons, it makes little sense to have 5 dialogs per certificate
> entry rendered and invisible, so dom-if is necessary.
>  - Dialogs are dom-ifed to true, only when they need to be shown, so calling
> open() on attach() is one way of achieving that. Manually calling open() right
> after flipping the dom-if boolean to true is another way to do this, but seems
> unnecessary.
>  - Once the dialog is closed, it is immediately removed from the DOM, using
the
> iron-overlay-closed listener.
> 
> I don't see any value of the following tri-state
> 1) Dialog not in the DOM
> 2) Dialog in the DOM and not visible (open() has not been called, or close()
was
> called).
> 3) Dialog in the DOM and visible (open() has been called) 
> 
> So my usage pattern reduces the above 3 states to only 2
> 1) Dialog in the DOM and visible
> 2) Dialog not in the DOM (therefore not visible)
> 
> I am open to discussion on this usage pattern. It seems that discussion is
> better suited for the follow up CL, as you guessed.

Yes, I am looking forward to it trololol.

[dpapad, 2016-03-18 00:58:03]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-18 00:58:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/1812673002/#ps100001
(title: "Nit")

[commit-bot: I haz the power, 2016-03-18 00:58:25]

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

[commit-bot: I haz the power, 2016-03-18 01:02:08]

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

[commit-bot: I haz the power, 2016-03-18 01:02:09]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dpapad, 2016-03-18 01:47:51]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-03-18 01:48:21]

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

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

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-18 04:52:22]

Description was changed from

==========
MD Settings: Certificate manager, error dialog.

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

to

==========
MD Settings: Certificate manager, error dialog.

BUG=547073

Committed: https://crrev.com/576dabdc2ea5abc890fb03822bc30c04b942aeec
Cr-Commit-Position: refs/heads/master@{#381884}
==========

[commit-bot: I haz the power, 2016-03-18 04:52:23]

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