Fix closure compilation for WebUI + mojo modules

Several WebUI pages have recently been migrated to mojo JS modules. In
the process, full closure type checking was inadvertently disabled due
to incorrect use of js_type_check closure_flags.

This re-enables full type checking for the affected pages and fixes any
existing flaws revealed as a result

Bug: 1004256
Change-Id: If11808a64501606a784495baeb2543cee33daf0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518089
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824164}

chrome/browser/resources/discards/BUILD.gn

@@ -7,6 +7,8 @@ import("//tools/polymer/polymer.gni")
 
 js_type_check("closure_compile") {
   is_polymer3 = true
+  closure_flags = default_closure_args + mojom_js_args +
+                  [ "js_module_root=" + rebase_path(".", root_build_dir) ]
   deps = [
     ":database_tab",
     ":discards",
@@ -15,7 +17,6 @@ js_type_check("closure_compile") {
     ":graph_doc",
     ":graph_tab_template",
   ]
-  closure_flags = mojom_js_args
 }
 
 js_library("discards") {

chrome/browser/resources/discards/generate_graph_tab.py

@@ -12,6 +12,15 @@ import string
 import sys
 
 
+def strip_js_imports(js_contents):
+  """The input JS may use imports for Closure compilation. These must be
+  stripped from the output since the resulting data: URL cannot use imports
+  within its webview."""
+  def not_an_import(line):
+    return not line.startswith('import ')
+  return '\n'.join(filter(not_an_import, js_contents.splitlines()))
+
+
 def main():
   argument_parser = argparse.ArgumentParser()
   argument_parser.add_argument('output_file', help='The file to write to')
@@ -27,7 +36,8 @@ def main():
   output_template = string.Template(open(args.output_template, 'r').read());
 
   # Stamp the javacript contents into the HTML template.
-  html_doc = html_template.substitute({'javascript_file': js_file_contents});
+  html_doc = html_template.substitute(
+      {'javascript_file': strip_js_imports(js_file_contents)})
 
   # Construct the data: URL that contains the combined doc.
   data_url = "data:text/html;base64,%s" % base64.b64encode(

chrome/browser/resources/discards/graph_doc.js

@@ -6,6 +6,11 @@
 // resources. The only communication to and from this implementation and the
 // WebUI is through postMessage.
 
+// Note that these imports are stripped by a build step before being packaged.
+// They're only present to help Closure compiler do type checks and must be
+// referenced only within Closure annotations.
+import {FavIconInfo, FrameInfo, GraphChangeStreamInterface, PageInfo, ProcessInfo, WorkerInfo} from './chrome/browser/ui/webui/discards/discards.mojom-webui.js';
+
 // Radius of a node circle.
 const /** number */ kNodeRadius = 6;
 

chrome/browser/resources/downloads/BUILD.gn

@@ -72,8 +72,8 @@ preprocess_grit("preprocess") {
   in_files = [
     "browser_proxy.js",
     "constants.js",
+    "data.js",
     "downloads.js",
-    "externs.js",
     "icon_loader.js",
     "search_service.js",
   ]
@@ -127,15 +127,22 @@ grit("downloads_resources") {
 
 js_type_check("closure_compile") {
   is_polymer3 = true
+  closure_flags =
+      default_closure_args + mojom_js_args + [
+        "js_module_root=" + rebase_path(".", root_build_dir),
+        "js_module_root=" + rebase_path(
+                "$root_gen_dir/mojom-webui/chrome/browser/ui/webui/downloads",
+                root_build_dir),
+      ]
   deps = [
     ":browser_proxy",
     ":constants",
+    ":data",
     ":item",
     ":manager",
     ":search_service",
     ":toolbar",
   ]
-  closure_flags = mojom_js_args
 }
 
 js_library("browser_proxy") {
@@ -143,12 +150,15 @@ js_library("browser_proxy") {
     "//chrome/browser/ui/webui/downloads:mojo_bindings_webui_js",
     "//ui/webui/resources/js:cr.m",
   ]
-  externs_list = [ "externs.js" ]
 }
 
 js_library("constants") {
 }
 
+js_library("data") {
+  deps = [ "//chrome/browser/ui/webui/downloads:mojo_bindings_webui_js" ]
+}
+
 js_library("icon_loader") {
   deps = [
     "//ui/webui/resources/js:assert.m",
@@ -162,6 +172,7 @@ js_library("item") {
   deps = [
     ":browser_proxy",
     ":constants",
+    ":data",
     ":icon_loader",
     "//ui/webui/resources/cr_elements/cr_toast:cr_toast_manager.m",
     "//ui/webui/resources/js:assert.m",
@@ -175,6 +186,7 @@ js_library("item") {
 js_library("manager") {
   deps = [
     ":browser_proxy",
+    ":data",
     ":item",
     ":search_service",
     ":toolbar",

chrome/browser/resources/downloads/externs.js → chrome/browser/resources/downloads/data.js

@@ -3,16 +3,11 @@
 // found in the LICENSE file.
 
 /**
- * @fileoverview Externs for objects sent from C++ to JS for chrome://downloads.
- * @externs
+ * @fileoverview Typedef to allow augmentation of Data objects sent from C++ to
+ * JS for chrome://downloads.
  */
 
-// eslint-disable-next-line no-var
-var downloads = {};
+import {Data as MojomData} from './downloads.mojom-webui.js';
 
-/**
- * The type of the download object. The definition is based on the Data struct
- * in downloads.mojom.
- * @typedef {downloads.mojom.Data | {hideDate: boolean}}
- */
-downloads.Data;
+/** @typedef {MojomData | {hideDate: boolean}} */
+export let Data;

chrome/browser/resources/downloads/item.js

@@ -26,6 +26,7 @@ import {beforeNextRender, html, Polymer} from 'chrome://resources/polymer/v3_0/p
 
 import {BrowserProxy} from './browser_proxy.js';
 import {DangerType, States} from './constants.js';
+import {Data} from './data.js';
 import {PageHandlerInterface} from './downloads.mojom-webui.js';
 import {IconLoader} from './icon_loader.js';
 
@@ -42,7 +43,7 @@ Polymer({
   overrideCustomEquivalent: true,
 
   properties: {
-    /** @type {!downloads.Data} */
+    /** @type {!Data} */
     data: Object,
 
     /** @private */

chrome/browser/resources/downloads/manager.js

@@ -23,6 +23,7 @@ import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bun
 
 import {BrowserProxy} from './browser_proxy.js';
 import {States} from './constants.js';
+import {Data} from './data.js';
 import {PageCallbackRouter, PageHandlerInterface} from './downloads.mojom-webui.js';
 import {SearchService} from './search_service.js';
 
@@ -55,7 +56,7 @@ Polymer({
       value: false,
     },
 
-    /** @private {!Array<!downloads.Data>} */
+    /** @private {!Array<!Data>} */
     items_: {
       type: Array,
       value() {
@@ -168,7 +169,7 @@ Polymer({
 
   /**
    * @param {number} index
-   * @param {!Array<downloads.Data>} items
+   * @param {!Array<Data>} items
    * @private
    */
   insertItems_(index, items) {
@@ -339,7 +340,7 @@ Polymer({
 
   /**
    * @param {number} index
-   * @param {!downloads.Data} data
+   * @param {!Data} data
    * @private
    */
   updateItem_(index, data) {

chrome/browser/resources/downloads/toolbar.js

@@ -18,6 +18,7 @@ import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
 import {BrowserProxy} from './browser_proxy.js';
+import {Data} from './data.js';
 import {PageHandlerInterface} from './downloads.mojom-webui.js';
 import {SearchService} from './search_service.js';
 
@@ -33,7 +34,7 @@ Polymer({
       observer: 'updateClearAll_',
     },
 
-    /** @type {!Array<!downloads.Data>} */
+    /** @type {!Array<!Data>} */
     items: {
       type: Array,
       value: Array,

chrome/browser/resources/engagement/BUILD.gn

@@ -6,8 +6,13 @@ import("//third_party/closure_compiler/compile_js.gni")
 
 js_type_check("closure_compile") {
   uses_js_modules = true
+  closure_flags = default_closure_args + mojom_js_args + [
+                    "js_module_root=" + rebase_path(".", root_build_dir),
+                    "js_module_root=" + rebase_path(
+                            "$root_gen_dir/mojom-webui/components/site_engagement/core/mojom",
+                            root_build_dir),
+                  ]
   deps = [ ":site_engagement" ]
-  closure_flags = mojom_js_args
 }
 
 js_library("site_engagement") {

chrome/browser/resources/omnibox/BUILD.gn

@@ -8,7 +8,8 @@ import("//tools/grit/grit_rule.gni")
 
 js_type_check("closure_compile") {
   uses_js_modules = true
-  closure_flags = mojom_js_args
+  closure_flags = default_closure_args + mojom_js_args +
+                  [ "js_module_root=" + rebase_path(".", root_build_dir) ]
   deps = [
     ":omnibox",
     ":omnibox_element",

mojo/public/tools/bindings/generators/mojom_js_generator.py

@@ -613,15 +613,16 @@ class Generator(generator.Generator):
       if mojom.IsEnumKind(kind):
         return "number"
       prefix = "" if mojom.IsNullableKind(kind) else "!"
-      if mojom.IsStructKind(kind) or mojom.IsUnionKind(kind):
-        return prefix + "Object"
       if mojom.IsArrayKind(kind):
         return prefix + ("Array<%s>" % get_type_name(kind.kind))
-      if mojom.IsMapKind(kind):
-        return "%sMap<%s, %s>|%sObject<%s, %s>" % (
+      if mojom.IsMapKind(kind) and self._IsStringableKind(kind.key_kind):
+        return "(%sMap<%s, %s>|%sObject<%s, %s>)" % (
             prefix, get_type_name(kind.key_kind), get_type_name(
                 kind.value_kind), prefix, get_type_name(
                     kind.key_kind), get_type_name(kind.value_kind))
+      if mojom.IsMapKind(kind):
+        return "{}Map<{}, {}>".format(prefix, get_type_name(kind.key_kind),
+                                      get_type_name(kind.value_kind))
       return prefix + self._GetTypeNameForNewBindings(kind,
                                                       for_module=for_module)