MD Settings: Certificate manager, move "Import" button to the tab level.

chrome/test/data/webui/settings/certificate_manager_page_test.js

 // 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.
 
 cr.define('certificate_manager_page', function() {
   /**
    * A test version of CertificatesBrowserProxy. Provides helper methods
    * for allowing tests to know when a method was called, as well as
    * specifying mock responses.
    *
    * @constructor
    * @implements {settings.CertificatesBrowserProxy}
    * @extends {settings.TestBrowserProxy}
    */
   var TestCertificatesBrowserProxy = function() {
     settings.TestBrowserProxy.call(this, [
       'deleteCertificate',
       'editCaCertificateTrust',
       'exportCertificate',
       'exportPersonalCertificate',
       'exportPersonalCertificatePasswordSelected',
       'getCaCertificateTrust',
+      'importCaCertificate',
+      'importPersonalCertificate',
       'importPersonalCertificatePasswordSelected',
+      'importServerCertificate',
       'refreshCertificates',
       'viewCertificate',
     ]);
 
     /** @private {!CaTrustInfo} */
     this.caTrustInfo_ = {ssl: true, email: true, objSign: true};
 
     /** @private {?CertificatesError} */
     this.certificatesError_ = null;
   };
 
   TestCertificatesBrowserProxy.prototype = {
     __proto__: settings.TestBrowserProxy.prototype,
 
     /**
      * @param {!CaTrustInfo} caTrustInfo
      */
     setCaCertificateTrust: function(caTrustInfo) {
       this.caTrustInfo_ = caTrustInfo;
     },
 
     /** @override */
     getCaCertificateTrust: function(id) {
       this.methodCalled('getCaCertificateTrust', id);
       return Promise.resolve(this.caTrustInfo_);
     },
 
     /** @override */
+    importServerCertificate: function() {
+      this.methodCalled('importServerCertificate');
+      return Promise.resolve();
+    },
+
+    /** @override */
+    importCaCertificate: function() {
+      this.methodCalled('importCaCertificate');
+      return Promise.resolve('dummyName');
+    },
+
+    /** @override */
     editCaCertificateTrust: function(id, ssl, email, objSign) {
       this.methodCalled('editCaCertificateTrust', {
         id: id, ssl: ssl, email: email, objSign: objSign,
       });
       return this.fulfillRequest_();
     },
 
     /**
      * Forces some of the browser proxy methods to start returning errors.
      */
@@ skipping 20 matching lines @@
     },
 
     /** @override */
     exportPersonalCertificatePasswordSelected: function(password) {
       this.resolverMap_.get(
           'exportPersonalCertificatePasswordSelected').resolve(password);
       return this.fulfillRequest_();
     },
 
     /** @override */
+    importPersonalCertificate: function() {
+      this.methodCalled('importPersonalCertificate');
+      return Promise.resolve(true);
+    },
+
+    /** @override */
     importPersonalCertificatePasswordSelected: function(password) {
       this.resolverMap_.get(
           'importPersonalCertificatePasswordSelected').resolve(password);
       return this.fulfillRequest_();
     },
 
     /** @override */
     refreshCertificates: function() {
       this.methodCalled('refreshCertificates');
     },
@@ skipping 445 matching lines @@
               assertEquals(
                   settings.CertificateAction.EXPORT_PERSONAL,
                   detail.action);
               assertEquals(subentry.model.id, detail.subnode.id);
             });
       });
     });
   }
 
   function registerPageTests() {
-    /** @type {?SettingsCertificateManagerPage} */
+    /** @type {?SettingsCertificateManagerPageElement} */
     var page = null;
 
     /** @type {?TestCertificatesBrowserProxy} */
     var browserProxy = null;
 
     /** @enum {number} */
     var CertificateCategoryIndex = {
       PERSONAL: 0,
       SERVER: 1,
       CA: 2,
@@ skipping 60 matching lines @@
               assertCertificateListLength(CertificateCategoryIndex.OTHER, 0);
             });
       });
 
       /**
        * Dispatches a settings.CertificateActionEvent.
        * @param {!settings.CertificateAction} action The type of action to
        *     simulate.
        */
       function dispatchCertificateActionEvent(action) {
-        var eventDetail = /** @type {!CertificateActionEventDetail} */ ({
+        page.fire(
+            settings.CertificateActionEvent,
+            /** @type {!CertificateActionEventDetail} */ ({
           action: action,
           subnode: createSampleCertificateSubnode(),
           certificateType: settings.CertificateType.PERSONAL
-        });
-        page.dispatchEvent(new CustomEvent(
-            settings.CertificateActionEvent, {detail: eventDetail}));
+        }));
       }
 
       /**
        * Tests that a dialog opens as a response to a
        * settings.CertificateActionEvent.
        * @param {string} dialogTagName The type of dialog to test.
        * @param {!settings.CertificateAction} action The action that is supposed
        *     to trigger the dialog.
        */
       function testDialogOpensOnAction(dialogTagName, action) {
         assertFalse(!!page.shadowRoot.querySelector(dialogTagName));
         dispatchCertificateActionEvent(action);
         Polymer.dom.flush();
         assertTrue(!!page.shadowRoot.querySelector(dialogTagName));
       }
 
       test('OpensDialog_DeleteConfirmation', function() {
         testDialogOpensOnAction(
-              'settings-certificate-delete-confirmation-dialog',
-              settings.CertificateAction.DELETE);
+            'settings-certificate-delete-confirmation-dialog',
+            settings.CertificateAction.DELETE);
       });
 
       test('OpensDialog_PasswordEncryption', function() {
         testDialogOpensOnAction(
-              'settings-certificate-password-encryption-dialog',
-              settings.CertificateAction.EXPORT_PERSONAL);
+            'settings-certificate-password-encryption-dialog',
+            settings.CertificateAction.EXPORT_PERSONAL);
       });
 
       test('OpensDialog_PasswordDecryption', function() {
         testDialogOpensOnAction(
-              'settings-certificate-password-decryption-dialog',
-              settings.CertificateAction.IMPORT_PERSONAL);
+            'settings-certificate-password-decryption-dialog',
+            settings.CertificateAction.IMPORT);
       });
 
       test('OpensDialog_CaTrustEdit', function() {
         testDialogOpensOnAction(
-              'settings-ca-trust-edit-dialog', settings.CertificateAction.EDIT);
+            'settings-ca-trust-edit-dialog', settings.CertificateAction.EDIT);
       });
     });
   }
+
+  function registerCertificateListTests() {
+    /** @type {?SettingsCertificateListElement} */
+    var element = null;
+
+    /** @type {?TestCertificatesBrowserProxy} */
+    var browserProxy = null;
+
+    suite('CertificateListTests', function() {
+      setup(function() {
+        browserProxy = new TestCertificatesBrowserProxy();
+        settings.CertificatesBrowserProxyImpl.instance_ = browserProxy;
+        PolymerTest.clearBody();
+        element = document.createElement('settings-certificate-list');
+        document.body.appendChild(element);
+      });
+
+      teardown(function() { element.remove(); });
+

[Dan Beam, 2016/04/01 18:38:02]

nit: \n\n -> \n

[dpapad, 2016/04/01 21:13:17]

Done.

+
+      /**
+       * Tests the "Import" button functionality.
+       * @param {!settings.CertificateType} certificateType
+       * @param {string} proxyMethodName The name of the proxy method expected
+       *     to be called.
+       * @param {boolean} actionEventExpected Whether a
+       *     settings.CertificateActionEvent is expected to fire as a result
+       *     tapping the Import button.
+       */
+      function testImportForCertificateType(
+          certificateType, proxyMethodName, actionEventExpected) {
+        element.certificateType = certificateType
+        Polymer.dom.flush();
+
+        var importButton = element.$$('paper-button');
+        assertTrue(!!importButton);
+
+        var waitForActionEvent = actionEventExpected ?
+            eventToPromise(settings.CertificateActionEvent, element) :
+            Promise.resolve(null);
+
+        MockInteractions.tap(importButton);
+        return browserProxy.whenCalled(proxyMethodName).then(
+            function() {
+              return waitForActionEvent;
+            }).then(
+            function(event) {

[Dan Beam, 2016/04/01 18:38:02]

should this be

  .then(function(event) {

on the same line or more continuation indent?

[dpapad, 2016/04/01 21:13:17]

Done.

Personally I am equally OK with either way. My brain has been trained to parse
Promise-based syntax enough to not struggle with the \n positioning. I chose
this syntax because of a comment by @tommycli in a previous review for a similar
case, see
https://codereview.chromium.org/1815733004/diff/80001/chrome/browser/resource....

Ultimately it would be nice if we come to a conclusion about such cases, by
either of the following:
1) Allow flexible formatting, and stop striving to be 100% consistent on \n
positioning across the codebase (with regards to promise callbacks).
2) Find a single formatting style that keeps everyone happy and stick with it.

No1 sounds more viable and realistic IMO. Current situation of formatting \n
based on who is reviewing, leads to the same result as option 1 above (having
both occur in the code), but requires more time (from reviewee and reviewer)
than option 1.

+              if (actionEventExpected) {
+                assertEquals(
+                    settings.CertificateAction.IMPORT, event.detail.action);
+                assertEquals(certificateType, event.detail.certificateType);
+              }
+            });

[Dan Beam, 2016/04/01 18:38:02]

i'm not trying to enforce my ident on you, but

return browserProxy.whenCalled(proxyMethodName).then(function() {
  return waitForActionEvent;
}.then(function(event) {
  ...
});

seems less convoluted to me

[dpapad, 2016/04/01 21:13:17]

Done.

+      }
+
+      test('ImportButton_Personal', function() {
+        return testImportForCertificateType(
+            settings.CertificateType.PERSONAL,
+            'importPersonalCertificate', true);
+      });
+
+      test('ImportButton_Server', function() {
+        return testImportForCertificateType(
+            settings.CertificateType.SERVER, 'importServerCertificate',
+            false);
+      });
+
+      test('ImportButton_CA', function() {
+        return testImportForCertificateType(
+            settings.CertificateType.CA, 'importCaCertificate', true);
+      });
+    });
+  }
 
   return {
     registerTests: function() {
       registerCaTrustEditDialogTests();
       registerDeleteDialogTests();
       registerPasswordEncryptDialogTests();
       registerPasswordDecryptDialogTests();
       registerPageTests();
       registerCertificateSubentryTests();
+      registerCertificateListTests();
     },
   };
 });