MD Settings: Certificate manager, error dialog.

chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js

Index: chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js
diff --git a/chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js b/chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js
new file mode 100644
index 0000000000000000000000000000000000000000..02b7abcae8af0843dc2abde2d87e41698bc9a4bb
--- /dev/null
+++ b/chrome/browser/resources/settings/certificate_manager_page/certificates_error_dialog.js
@@ -0,0 +1,37 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview A dialog for showing SSL certificate related error messages.
+ * The user can only close the dialog, there is no other possible intercation.

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

nit: possible interaction.

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

Done.

+ */
+Polymer({
+  is: 'settings-certificates-error-dialog',
+
+  properties: {
+    /** @type {!CertificatesError|!CertificatesImportError} */
+    model: Object,
+  },
+
+  /** @override */
+  attached: function() {
+    this.$.dialog.open();

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

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?

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

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.

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

Yes, I am looking forward to it trololol.

+  },
+
+  /** @private */
+  close_: function() {

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

nit: as mentioned before, probably rename to onOkTap_

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

Done.

+    this.$.dialog.close();
+  },
+
+  /**
+   * @param {{certificateName: string, error: string}} importError
+   * @return {string}
+   * @private
+   */
+  getCertificateErrorText_: function(importError) {
+    return loadTimeData.getStringF(
+        'certificateImportErrorFormat',
+        importError.certificateName, importError.error);
+  },
+});