Remove cr.define and CallJavascriptFunctionUnsafe in certificate viewer.

Also migrating test to use Mocha, and eliminating the need to refer to
prod code using the cert_viewer namespace. The test code previously was
replacing a prod code function as a way to intercept the response
handler and make assertions, now replaced by an event instead.

Bug: 1028829
Change-Id: Ieb5210965b311743c003a28c425f75d6ac1be462
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2145934
Commit-Queue: dpapad <dpapad@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759518}

chrome/browser/resources/certificate_viewer.html

@@ -9,6 +9,7 @@
     <link rel="stylesheet" href="certificate_viewer.css">
     <script src="chrome://resources/js/load_time_data.js"></script>
     <script src="strings.js"></script>
+    <script src="chrome://resources/js/promise_resolver.js"></script>
     <script src="chrome://resources/js/cr.js"></script>
     <script src="chrome://resources/js/cr/ui.js"></script>
     <script src="chrome://resources/js/cr/ui/focus_outline_manager.js"></script>

chrome/browser/resources/certificate_viewer.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-cr.define('cert_viewer', function() {
+(function() {
   'use strict';
 
   /**
@@ -166,7 +166,8 @@ cr.define('cert_viewer', function() {
     clearCertificateFields();
     const item = $('hierarchy').selectedItem;
     if (item && item.detail.payload.index !== undefined) {
-      chrome.send('requestCertificateFields', [item.detail.payload.index]);
+      cr.sendWithPromise('requestCertificateFields', item.detail.payload.index)
+          .then(onCertificateFields);
     }
   }
 
@@ -175,7 +176,7 @@ cr.define('cert_viewer', function() {
    * @param {Object} certFields A dictionary containing the fields tree
    *     structure.
    */
-  function getCertificateFields(certFields) {
+  function onCertificateFields(certFields) {
     clearCertificateFields();
     const treeItem = $('cert-fields');
     treeItem.add(
@@ -183,6 +184,8 @@ cr.define('cert_viewer', function() {
     revealTree(treeItem);
     // Ensure the list is scrolled to the top by selecting the first item.
     treeItem.children[0].selected = true;
+    document.body.dispatchEvent(
+        new CustomEvent('certificate-fields-updated-for-tesing'));
   }
 
   /**
@@ -207,10 +210,5 @@ cr.define('cert_viewer', function() {
     }
   }
 
-  return {
-    initialize: initialize,
-    getCertificateFields: getCertificateFields,
-  };
-});
-
-document.addEventListener('DOMContentLoaded', cert_viewer.initialize);
+  document.addEventListener('DOMContentLoaded', initialize);
+})();

chrome/browser/ui/webui/certificate_viewer_webui.cc

@@ -310,18 +310,19 @@ CertificateViewerDialogHandler::~CertificateViewerDialogHandler() {
 void CertificateViewerDialogHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "exportCertificate",
-      base::BindRepeating(&CertificateViewerDialogHandler::ExportCertificate,
-                          base::Unretained(this)));
+      base::BindRepeating(
+          &CertificateViewerDialogHandler::HandleExportCertificate,
+          base::Unretained(this)));
   web_ui()->RegisterMessageCallback(
       "requestCertificateFields",
       base::BindRepeating(
-          &CertificateViewerDialogHandler::RequestCertificateFields,
+          &CertificateViewerDialogHandler::HandleRequestCertificateFields,
           base::Unretained(this)));
 }
 
-void CertificateViewerDialogHandler::ExportCertificate(
+void CertificateViewerDialogHandler::HandleExportCertificate(
     const base::ListValue* args) {
-  int cert_index = GetCertificateIndex(args);
+  int cert_index = GetCertificateIndex(args->GetList()[0].GetInt());
   if (cert_index < 0)
     return;
 
@@ -333,9 +334,11 @@ void CertificateViewerDialogHandler::ExportCertificate(
                        cert_chain_.end());
 }
 
-void CertificateViewerDialogHandler::RequestCertificateFields(
+void CertificateViewerDialogHandler::HandleRequestCertificateFields(
     const base::ListValue* args) {
-  int cert_index = GetCertificateIndex(args);
+  AllowJavascript();
+  const base::Value& callback_id = args->GetList()[0];
+  int cert_index = GetCertificateIndex(args->GetList()[1].GetInt());
   if (cert_index < 0)
     return;
 
@@ -452,17 +455,12 @@ void CertificateViewerDialogHandler::RequestCertificateFields(
           .Build());
 
   // Send certificate information to javascript.
-  web_ui()->CallJavascriptFunctionUnsafe("cert_viewer.getCertificateFields",
-                                         root_list);
+  ResolveJavascriptCallback(callback_id, root_list);
 }
 
 int CertificateViewerDialogHandler::GetCertificateIndex(
-    const base::ListValue* args) const {
-  int cert_index;
-  double val;
-  if (!(args->GetDouble(0, &val)))
-    return -1;
-  cert_index = static_cast<int>(val);
+    int requested_index) const {
+  int cert_index = requested_index;
   if (cert_index < 0 || cert_index >= static_cast<int>(cert_chain_.size()))
     return -1;
   return cert_index;

chrome/browser/ui/webui/certificate_viewer_webui.h

@@ -88,18 +88,18 @@ class CertificateViewerDialogHandler : public content::WebUIMessageHandler {
   // chain.
   //
   // The input is an integer index to the certificate in the chain to export.
-  void ExportCertificate(const base::ListValue* args);
+  void HandleExportCertificate(const base::ListValue* args);
 
-  // Gets the details for a specific certificate in the certificate chain. Calls
-  // the javascript function cert_viewer.getCertificateFields with a tree
-  // structure containing the fields and values for certain nodes.
+  // Gets the details for a specific certificate in the certificate chain.
+  // Responds with a tree structure containing the fields and values for certain
+  // nodes.
   //
   // The input is an integer index to the certificate in the chain to view.
-  void RequestCertificateFields(const base::ListValue* args);
+  void HandleRequestCertificateFields(const base::ListValue* args);
 
-  // Helper function to get the certificate index from |args|. Returns -1 if
-  // the index is out of range.
-  int GetCertificateIndex(const base::ListValue* args) const;
+  // Helper function to get the certificate index. Returns -1 if the index is
+  // out of range.
+  int GetCertificateIndex(int requested_index) const;
 
   // The dialog.
   CertificateViewerDialog* dialog_;

chrome/test/data/webui/BUILD.gn

@@ -159,7 +159,7 @@ js2gtest("browser_tests_js_webui") {
   }
 
   if (use_nss_certs) {
-    sources += [ "certificate_viewer_dialog_test.js" ]
+    sources += [ "certificate_viewer_dialog_browsertest.js" ]
   }
   if (enable_extensions) {
     sources += [

chrome/test/data/webui/certificate_viewer_dialog_browsertest.js

+// Copyright 2020 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.
+
+/**
+ * Test fixture for generated tests.
+ * @extends {testing.Test}
+ */
+function CertificateViewerUITest() {}
+
+CertificateViewerUITest.prototype = {
+  __proto__: testing.Test.prototype,
+
+  /** @override */
+  isAsync: true,
+
+  /** @override */
+  extraLibraries: [
+    '//third_party/mocha/mocha.js',
+    '//chrome/test/data/webui/mocha_adapter.js',
+    'test_util.js',
+    'certificate_viewer_dialog_test.js',
+  ],
+
+  /**
+   * Define the C++ fixture class and include it.
+   * @type {?string}
+   * @override
+   */
+  typedefCppFixture: 'CertificateViewerUITest',
+
+  /**
+   * Show the certificate viewer dialog.
+   */
+  testGenPreamble: function() {
+    GEN('ShowCertificateViewer();');
+  },
+};
+
+
+// Include the bulk of c++ code.
+// Certificate viewer UI tests are disabled on platforms with native certificate
+// viewers.
+GEN('#include "chrome/test/data/webui/certificate_viewer_ui_test-inl.h"');
+GEN('');
+
+// Constructors and destructors must be provided in .cc to prevent clang errors.
+GEN('CertificateViewerUITest::CertificateViewerUITest() {}');
+GEN('CertificateViewerUITest::~CertificateViewerUITest() {}');
+
+TEST_F('CertificateViewerUITest', 'DialogURL', function() {
+  mocha.grep('DialogURL').run();
+});
+
+TEST_F('CertificateViewerUITest', 'CommonName', function() {
+  mocha.grep('CommonName').run();
+});
+
+TEST_F('CertificateViewerUITest', 'Details', function() {
+  mocha.grep('Details').run();
+});

chrome/test/data/webui/certificate_viewer_dialog_test.js

@@ -3,43 +3,36 @@
 // found in the LICENSE file.
 
 /**
- * Test fixture for generated tests.
- * @extends {testing.Test}
+ * Find the first tree item (in the certificate fields tree) with a value.
+ * @param {!Element} tree Certificate fields subtree to search.
+ * @return {?Element} The first found element with a value, null if not found.
  */
-function CertificateViewerUITest() {}
-
-CertificateViewerUITest.prototype = {
-  __proto__: testing.Test.prototype,
-
-  /**
-   * Define the C++ fixture class and include it.
-   * @type {?string}
-   * @override
-   */
-  typedefCppFixture: 'CertificateViewerUITest',
-
-  /**
-   * Show the certificate viewer dialog.
-   */
-  testGenPreamble: function() {
-    GEN('ShowCertificateViewer();');
-  },
+function getElementWithValue(tree) {
+  for (var i = 0; i < tree.childNodes.length; i++) {
+    var element = tree.childNodes[i];
+    if (element.detail && element.detail.payload &&
+        element.detail.payload.val) {
+      return element;
+    }
+    if (element = getElementWithValue(element)) {
+      return element;
+    }
+  }
+  return null;
+}
 
-  /**
-   * Tests that the dialog opened to the correct URL.
-   */
-  testDialogUrl: function() {
+suite('CertificateViewer', function() {
+  // Tests that the dialog opened to the correct URL.
+  test('DialogURL', function() {
     assertEquals(chrome.getVariableValue('expectedUrl'), window.location.href);
-  },
+  });
 
-  /**
-   * Tests for the correct common name in the test certificate.
-   */
-  testCN: function() {
+  // Tests for the correct common name in the test certificate.
+  test('CommonName', function() {
     assertEquals('www.google.com', $('issued-cn').textContent);
-  },
+  });
 
-  testDetails: function() {
+  test('Details', function() {
     var certHierarchy = $('hierarchy');
     var certFields = $('cert-fields');
     var certFieldVal = $('cert-field-value');
@@ -53,11 +46,9 @@ CertificateViewerUITest.prototype = {
     // Select the first certificate on the chain and ensure the details show up.
     // Override the receive certificate function to catch when fields are
     // loaded.
-    var getCertificateFields = cert_viewer.getCertificateFields;
-    cert_viewer.getCertificateFields =
-        this.continueTest(WhenTestDone.ALWAYS, function(certFieldDetails) {
-          getCertificateFields(certFieldDetails);
-          cert_viewer.getCertificateFields = getCertificateFields;
+    return test_util
+        .eventToPromise('certificate-fields-updated-for-tesing', document.body)
+        .then(() => {
           assertLT(0, certFields.childNodes.length);
 
           // Test that a field can be selected to see the details for that
@@ -71,76 +62,5 @@ CertificateViewerUITest.prototype = {
           certFields.selectedItem = certFields.childNodes[0];
           assertEquals('', certFieldVal.textContent);
         });
-    certHierarchy.selectedItem = certHierarchy.childNodes[0];
-  }
-};
-
-
-/**
- * Test fixture for asynchronous tests.
- * @extends {CertificateViewerUITest}
- */
-function CertificateViewerUITestAsync() {}
-
-CertificateViewerUITestAsync.prototype = {
-  __proto__: CertificateViewerUITest.prototype,
-
-  /** @inheritDoc */
-  isAsync: true,
-};
-
-
-// Include the bulk of c++ code.
-// Certificate viewer UI tests are disabled on platforms with native certificate
-// viewers.
-GEN('#include "chrome/test/data/webui/certificate_viewer_ui_test-inl.h"');
-GEN('');
-
-// Constructors and destructors must be provided in .cc to prevent clang errors.
-GEN('CertificateViewerUITest::CertificateViewerUITest() {}');
-GEN('CertificateViewerUITest::~CertificateViewerUITest() {}');
-
-/**
- * Tests that the dialog opened to the correct URL.
- */
-TEST_F('CertificateViewerUITest', 'testDialogURL', function() {
-  this.testDialogUrl();
-});
-
-/**
- * Tests for the correct common name in the test certificate.
- */
-TEST_F('CertificateViewerUITest', 'testCN', function() {
-  this.testCN();
+  });
 });
-
-/**
- * Test the details pane of the certificate viewer. This verifies that a
- * certificate in the chain can be selected to view the fields. And that fields
- * can be selected to view their values.
- */
-TEST_F('CertificateViewerUITestAsync', 'testDetails', function() {
-  this.testDetails();
-});
-
-////////////////////////////////////////////////////////////////////////////////
-// Support functions
-
-/**
- * Find the first tree item (in the certificate fields tree) with a value.
- * @param {!Element} tree Certificate fields subtree to search.
- * @return {?Element} The first found element with a value, null if not found.
- */
-function getElementWithValue(tree) {
-  for (var i = 0; i < tree.childNodes.length; i++) {
-    var element = tree.childNodes[i];
-    if (element.detail && element.detail.payload &&
-        element.detail.payload.val) {
-      return element;
-    }
-    if (element = getElementWithValue(element)) {
-      return element;
-    }
-  }
-  return null;
-}

chrome/test/data/webui/certificate_viewer_ui_test-inl.h

@@ -20,7 +20,8 @@
 #include "net/cert/x509_util_nss.h"
 #include "net/test/test_certificate_data.h"
 
-// Test framework for chrome/test/data/webui/certificate_viewer_dialog_test.js.
+// Test framework for
+// chrome/test/data/webui/certificate_viewer_dialog_browsertest.js.
 class CertificateViewerUITest : public WebUIBrowserTest {
  public:
   CertificateViewerUITest();