Consolidate closure flags used for chromeos/components/media_app_ui

There's a strict set of flags that aren't yet applied across test code.

This extends the flags to utility files used in tests (but not yet the
browsertest.js files themselves). Fixes the problems. And makes those
flags readily available to other system apps.

Bug: 1100755
Change-Id: Ib1dbb094bbfc5d3f42fe7d7dab19f9200449c09d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2275364Reviewed-by: Rachel Carpenter <carpenterr@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#783851}

chromeos/components/media_app_ui/BUILD.gn

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import("//chrome/test/base/js2gtest.gni")
+import("//chromeos/components/web_applications/system_apps.gni")
 import("//mojo/public/tools/bindings/mojom.gni")
 import("//third_party/closure_compiler/compile_js.gni")
 
@@ -86,20 +87,11 @@ mojom("mojo_bindings") {
   sources = [ "media_app_ui.mojom" ]
 }
 
-# Note we compile with reportUnknownTypes while it works, but if dependencies
-# get more complex, we should remove it and only enable in developer builds.
+media_test_closure_flags = system_app_closure_flags_strict + [ "hide_warnings_for=chromeos/components/media_app_ui/media_app_ui.mojom-lite-for-compile.js" ]
+
 js_type_check("closure_compile_tests") {
   testonly = true
-  closure_flags = default_closure_args + [
-                    "jscomp_error=strictCheckTypes",
-                    "jscomp_error=reportUnknownTypes",
-                    "language_in=ECMASCRIPT_2018",
-
-                    # TODO(crbug/1048973): Remove these when the mojo bindings
-                    # js is updated to pass a closure compile check.
-                    "hide_warnings_for=mojo/public/js/",
-                    "hide_warnings_for=chromeos/components/media_app_ui/media_app_ui.mojom-lite-for-compile.js",
-                  ]
+  closure_flags = media_test_closure_flags
   deps = [
     ":test_driver_api_js",
     ":test_driver_js",

chromeos/components/media_app_ui/resources/js/BUILD.gn

@@ -2,36 +2,10 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import("//chromeos/components/web_applications/system_apps.gni")
 import("//third_party/closure_compiler/compile_js.gni")
 
-# Lint checks are not well documented, but pick up some useful stuff. Currently
-# require it to be requested in developer builds only.
-enable_lint_checks = false
-
-# Note we compile with reportUnknownTypes while it works, but if dependencies
-# get more complex, we should remove it and only enable in developer builds.
-media_closure_flags = default_closure_args + [
-                        "conformance_configs " + rebase_path(
-                                "../../../web_applications/closure_conformance_checks.txt",
-                                root_build_dir),
-                        "jscomp_error=conformanceViolations",
-
-                        "jscomp_error=strictCheckTypes",
-                        "jscomp_error=reportUnknownTypes",
-                        "language_in=ECMASCRIPT_2018",
-
-                        # TODO(crbug/1048973): Remove these when the mojo bindings
-                        # js is updated to pass a closure compile check.
-                        "hide_warnings_for=mojo/public/js/",
-                        "hide_warnings_for=chromeos/components/media_app_ui/media_app_ui.mojom-lite-for-compile.js",
-                      ]
-
-if (enable_lint_checks) {
-  media_closure_flags += [
-    "jscomp_error=lintChecks",
-    "hide_warnings_for=mojo/public/interfaces/bindings",
-  ]
-}
+media_closure_flags = system_app_closure_flags_strict + [ "hide_warnings_for=chromeos/components/media_app_ui/media_app_ui.mojom-lite-for-compile.js" ]
 
 group("closure_compile") {
   deps = [

chromeos/components/media_app_ui/resources/js/launch.js

@@ -584,7 +584,7 @@ async function loadOtherRelatedFiles(
 /**
  * Sets state for the files opened in the current directory.
  * @param {!FileSystemDirectoryHandle} directory
- * @param {!{file: !File, handle: !FileSystemFileHandle}} focusFile
+ * @param {{file: !File, handle: !FileSystemFileHandle}} focusFile
  */
 function setCurrentDirectory(directory, focusFile) {
   // Load currentFiles into the guest.

chromeos/components/media_app_ui/resources/js/receiver.js

@@ -192,7 +192,7 @@ const DELEGATE = {
    * @param {!mediaApp.AbstractFile} abstractFile
    * @return {!Promise<undefined>}
    */
-  async saveCopy(/** !mediaApp.AbstractFile */ abstractFile) {
+  async saveCopy(abstractFile) {
     /** @type {!SaveCopyMessage} */
     const msg = {blob: abstractFile.blob, suggestedName: abstractFile.name};
     await parentMessagePipe.sendMessage(Message.SAVE_COPY, msg);

chromeos/components/media_app_ui/test/dom_testing_helpers.js

@@ -12,7 +12,7 @@
  *
  * @param {string} query
  * @param {!Array<string>=} opt_path
- * @return {Promise<!Element>}
+ * @return {!Promise<!Element>}
  */
 async function waitForNode(query, opt_path) {
   /** @type {!HTMLElement|!ShadowRoot} */

chromeos/components/media_app_ui/test/driver.js

@@ -21,15 +21,15 @@ class GuestDriver {
    *
    * @param {string} query the querySelector to run in the guest.
    * @param {string=} opt_property a property to request on the found element.
-   * @param {Object=} opt_commands test commands to execute on the element.
+   * @param {!Object=} opt_commands test commands to execute on the element.
    * @return Promise<string> JSON.stringify()'d value of the property, or
    *   tagName if unspecified.
    */
   async waitForElementInGuest(query, opt_property, opt_commands = {}) {
-    /** @type {TestMessageQueryData} */
+    /** @type {!TestMessageQueryData} */
     const message = {testQuery: query, property: opt_property};
     await testMessageHandlersReady;
-    const result = /** @type {TestMessageResponseData} */ (
+    const result = /** @type {!TestMessageResponseData} */ (
         await guestMessagePipe.sendMessage(
             'test', {...message, ...opt_commands}));
     return result.testQueryResult;
@@ -41,7 +41,7 @@ class GuestDriver {
  * @param {string} testCase
  */
 async function runTestInGuest(testCase) {
-  /** @type {TestMessageRunTestCase} */
+  /** @type {!TestMessageRunTestCase} */
   const message = {testCase};
   await testMessageHandlersReady;
   await guestMessagePipe.sendMessage('run-test-case', message);
@@ -54,7 +54,7 @@ async function runTestInGuest(testCase) {
  */
 async function getFileErrors() {
   const message = {getFileErrors: true};
-  const response = /** @type {TestMessageResponseData} */ (
+  const response = /** @type {!TestMessageResponseData} */ (
       await guestMessagePipe.sendMessage('test', message));
   return response.testQueryResult;
 }
@@ -64,7 +64,7 @@ class FakeWritableFileStream {
   constructor(/** !Blob= */ data = new Blob()) {
     this.data = data;
 
-    /** @type {!Array<!{position: number, size: (number|undefined)}>} */
+    /** @type {!Array<{position: number, size: (number|undefined)}>} */
     this.writes = [];
 
     /** @type {function(!Blob)} */
@@ -77,6 +77,10 @@ class FakeWritableFileStream {
   /** @override */
   async write(data) {
     const position = 0;  // Assume no seeks.
+    if (!data) {
+      this.writes.push({position, size: 0});
+      return;
+    }
     this.writes.push({position, size: data.size});
     this.data = new Blob([
       this.data.slice(0, position),
@@ -101,7 +105,7 @@ class FakeWritableFileStream {
 /** @implements FileSystemHandle  */
 class FakeFileSystemHandle {
   /**
-   * @param {!string=} name
+   * @param {string=} name
    */
   constructor(name = 'fake_file.png') {
     this.isFile = true;
@@ -121,10 +125,10 @@ class FakeFileSystemHandle {
 /** @implements FileSystemFileHandle  */
 class FakeFileSystemFileHandle extends FakeFileSystemHandle {
   /**
-   * @param {!string=} name
-   * @param {!string=} type
+   * @param {string=} name
+   * @param {string=} type
    * @param {number=} lastModified
-   * @param {!Blob} blob
+   * @param {!Blob=} blob
    */
   constructor(
       name = 'fake_file.png', type = '', lastModified = 0, blob = new Blob()) {
@@ -133,10 +137,10 @@ class FakeFileSystemFileHandle extends FakeFileSystemHandle {
 
     this.lastWritable.data = blob;
 
-    /** @type {!string} */
+    /** @type {string} */
     this.type = type;
 
-    /** @type {!number} */
+    /** @type {number} */
     this.lastModified = lastModified;
 
     /** @type {?DOMException} */
@@ -170,7 +174,7 @@ class FakeFileSystemFileHandle extends FakeFileSystemHandle {
 /** @implements FileSystemDirectoryHandle  */
 class FakeFileSystemDirectoryHandle extends FakeFileSystemHandle {
   /**
-   * @param {!string=} name
+   * @param {string=} name
    */
   constructor(name = 'fake-dir') {
     super(name);
@@ -204,7 +208,7 @@ class FakeFileSystemDirectoryHandle extends FakeFileSystemHandle {
   /** @override */
   async getFile(name, options) {
     const fileHandle = this.files.find(f => f.name === name);
-    if (!fileHandle && options.create === true) {
+    if (!fileHandle && options && options.create === true) {
       // Simulate creating a new file, assume it is an image. This is needed for
       // renaming files to ensure it has the right mime type, the real
       // implementation copies the mime type from the binary.
@@ -244,7 +248,7 @@ class FakeFileSystemDirectoryHandle extends FakeFileSystemHandle {
  *   name: (string|undefined),
  *   type: (string|undefined),
  *   lastModified: (number|undefined),
- *   arrayBuffer: (function(): (Promise<ArrayBuffer>)|undefined)
+ *   arrayBuffer: (function(): (!Promise<!ArrayBuffer>)|undefined)
  * }}
  */
 let FileDesc;
@@ -252,11 +256,11 @@ let FileDesc;
 /**
  * Creates a mock directory with the provided files in it.
  * @param {!Array<!FileDesc>=} files
- * @return {Promise<FakeFileSystemDirectoryHandle>}
+ * @return {!Promise<!FakeFileSystemDirectoryHandle>}
  */
 async function createMockTestDirectory(files = [{}]) {
   const directory = new FakeFileSystemDirectoryHandle();
-  for (const /** FileDesc */ file of files) {
+  for (const /** !FileDesc */ file of files) {
     const fileBlob = file.arrayBuffer !== undefined ?
         new Blob([await file.arrayBuffer()]) :
         new Blob();
@@ -295,11 +299,11 @@ async function loadMultipleFiles(files) {
 
 /**
  * Creates a mock LaunchParams object from the provided `files`.
- * @param {!Array<FileSystemHandle>} files
- * @return {LaunchParams}
+ * @param {!Array<!FileSystemHandle>} files
+ * @return {!LaunchParams}
  */
 function handlesToLaunchParams(files) {
-  return /** @type{LaunchParams} */ ({files});
+  return /** @type{!LaunchParams} */ ({files});
 }
 
 /**
@@ -309,7 +313,7 @@ function handlesToLaunchParams(files) {
  * @param {!Array<!FakeFileSystemFileHandle>} directoryContents
  * @param {!Array<!FakeFileSystemFileHandle>=} multiSelectionFiles If provided,
  *     holds additional files selected in the files app at launch time.
- * @return {!Promise<FakeFileSystemDirectoryHandle>}
+ * @return {!Promise<!FakeFileSystemDirectoryHandle>}
  */
 async function launchWithHandles(directoryContents, multiSelectionFiles = []) {
   /** @type {?FakeFileSystemFileHandle} */
@@ -340,8 +344,8 @@ function fileToFileHandle(file) {
 /**
  * Helper to invoke launchWithHandles after wrapping `files` in fake handles.
  * @param {!Array<!File>} files
- * @param {!Array<!number>=} selectedIndexes
- * @return {!Promise<FakeFileSystemDirectoryHandle>}
+ * @param {!Array<number>=} selectedIndexes
+ * @return {!Promise<!FakeFileSystemDirectoryHandle>}
  */
 async function launchWithFiles(files, selectedIndexes = []) {
   const fileHandles = files.map(fileToFileHandle);
@@ -354,7 +358,7 @@ async function launchWithFiles(files, selectedIndexes = []) {
  * Creates an `Error` with the name field set.
  * @param {string} name
  * @param {string} msg
- * @return {Error}
+ * @return {!Error}
  */
 function createNamedError(name, msg) {
   const error = new Error(msg);
@@ -380,7 +384,7 @@ async function loadFilesWithoutSendingToGuest(directory, file) {
  * @param {?string} testCase
  */
 function assertFilesToBe(expectedFiles, testCase) {
-  return assertFilenamesToBe(expectedFiles.map(f => f.name).join(), testCase);
+  assertFilenamesToBe(expectedFiles.map(f => f.name).join(), testCase);
 }
 
 /**
@@ -402,30 +406,31 @@ function assertFilenamesToBe(expectedFilenames, testCase) {
  * Wraps `chai.assert.match` allowing tests to use `assertMatch`.
  * @param {string} string the string to match
  * @param {string} regex an escaped regex compatible string
- * @param {string|undefined} opt_message logged if the assertion fails
+ * @param {string=} opt_message logged if the assertion fails
  */
-function assertMatch(string, regex, opt_message) {
+function assertMatch(string, regex, opt_message = undefined) {
   chai.assert.match(string, new RegExp(regex), opt_message);
 }
 
 /**
  * Use to match error stack traces.
  * @param {string} stackTrace the stacktrace
- * @param {Array<string>} regexLines a list of escaped regex compatible strings,
- *     used to compare with the stacktrace.
- * @param {string|undefined} opt_message logged if the assertion fails
+ * @param {!Array<string>} regexLines a list of escaped regex compatible
+ *     strings, used to compare with the stacktrace.
+ * @param {string=} opt_message logged if the assertion fails
  */
-function assertMatchErrorStack(stackTrace, regexLines, opt_message) {
+function assertMatchErrorStack(
+    stackTrace, regexLines, opt_message = undefined) {
   const regex = `(.|\\n)*${regexLines.join('(.|\\n)*')}(.|\\n)*`;
   assertMatch(stackTrace, regex, opt_message);
 }
 
 /**
  * Returns the files loaded in the most recent call to `loadFiles()`.
- * @return {Promise<?Array<!ReceivedFile>>}
+ * @return {!Promise<?Array<!ReceivedFile>>}
  */
 async function getLoadedFiles() {
-  const response = /** @type {LastLoadedFilesResponse} */ (
+  const response = /** @type {!LastLoadedFilesResponse} */ (
       await guestMessagePipe.sendMessage('get-last-loaded-files'));
   if (response.fileList) {
     return response.fileList;
@@ -444,9 +449,11 @@ function simulateLosingAccessToDirectory() {
 
 /**
  * @param {!FakeFileSystemDirectoryHandle} directory
+ * @return {{handle: !FakeFileSystemFileHandle, file: !File}}
  */
 function launchWithFocusFile(directory) {
   const focusFile = {
+    /** @type {!FakeFileSystemFileHandle} */
     handle: directory.files[0],
     file: directory.files[0].getFileSync()
   };
@@ -464,7 +471,7 @@ async function assertSingleFileLaunch(directory, totalFiles) {
 
   await sendFilesToGuest();
 
-  const loadedFiles = await getLoadedFiles();
+  const loadedFiles = assertCast(await getLoadedFiles());
   // The untrusted context only loads the first file.
   chai.assert.equal(1, loadedFiles.length);
   // All files are in the `FileSystemDirectoryHandle`.

chromeos/components/media_app_ui/test/driver_api.js

@@ -12,7 +12,7 @@ let TestMessageResponseData;
  *     getFileErrors: (boolean|undefined),
  *     navigate: (string|undefined),
  *     overwriteLastFile: (string|undefined),
- *     pathToRoot: (Array<string>|undefined),
+ *     pathToRoot: (!Array<string>|undefined),
  *     property: (string|undefined),
  *     renameLastFile: (string|undefined),
  *     requestFullscreen: (boolean|undefined),
@@ -30,6 +30,6 @@ let TestMessageRunTestCase;
  * guest app using `loadFiles()`. We pass `ReceivedFileList.files` since passing
  * `ReceivedFileList` through different contexts prunes methods and fails due to
  * observers.
- * @typedef {{fileList: ?Array<ReceivedFile>}}
+ * @typedef {{fileList: ?Array<!ReceivedFile>}}
  */
 let LastLoadedFilesResponse;

chromeos/components/media_app_ui/test/guest_query_receiver.js

@@ -10,15 +10,21 @@ let lastReceivedFileList = null;
 
 /**
  * Test cases registered by GUEST_TEST.
- * @type {Map<string, function(): Promise<undefined>>}
+ * @type {!Map<string, function(): !Promise<undefined>>}
  */
 const guestTestCases = new Map();
 
+/**
+ * @return {!mediaApp.AbstractFile}
+ */
+function firstReceivedItem() {
+  return assertCast(assertCast(lastReceivedFileList).item(0));
+}
+
 /**
  * Acts on received TestMessageQueryData.
- *
- * @param {TestMessageQueryData} data
- * @return {!Promise<TestMessageResponseData>}
+ * @param {!TestMessageQueryData} data
+ * @return {!Promise<!TestMessageResponseData>}
  */
 async function runTestQuery(data) {
   let result = 'no result';
@@ -32,51 +38,53 @@ async function runTestQuery(data) {
       try {
         await element.requestFullscreen();
         result = 'hooray';
-      } catch (/** @type {TypeError} */ typeError) {
+      } catch (/** @type {!TypeError} */ typeError) {
         result = typeError.message;
       }
     }
   } else if (data.navigate !== undefined) {
     if (data.navigate === 'next') {
-      await lastReceivedFileList.loadNext();
+      await assertCast(lastReceivedFileList).loadNext();
       result = 'loadNext called';
     } else if (data.navigate === 'prev') {
-      await lastReceivedFileList.loadPrev();
+      await assertCast(lastReceivedFileList).loadPrev();
       result = 'loadPrev called';
     } else {
       result = 'nothing called';
     }
   } else if (data.overwriteLastFile) {
     const testBlob = new Blob([data.overwriteLastFile]);
-    await lastReceivedFileList.item(0).overwriteOriginal(testBlob);
+    await assertCast(firstReceivedItem().overwriteOriginal)
+        .call(firstReceivedItem(), testBlob);
     result = 'overwriteOriginal resolved';
   } else if (data.deleteLastFile) {
     try {
       const deleteResult =
-          await lastReceivedFileList.item(0).deleteOriginalFile();
+          await assertCast(firstReceivedItem().deleteOriginalFile)
+              .call(firstReceivedItem());
       if (deleteResult === DeleteResult.FILE_MOVED) {
         result = 'deleteOriginalFile resolved file moved';
       } else {
         result = 'deleteOriginalFile resolved success';
       }
-    } catch (/** @type{Error} */ error) {
+    } catch (/** @type{!Error} */ error) {
       result = `deleteOriginalFile failed Error: ${error}`;
     }
   } else if (data.renameLastFile) {
     try {
       const renameResult =
-          await lastReceivedFileList.item(0).renameOriginalFile(
-              data.renameLastFile);
+          await assertCast(firstReceivedItem().renameOriginalFile)
+              .call(firstReceivedItem(), data.renameLastFile);
       if (renameResult === RenameResult.FILE_EXISTS) {
         result = 'renameOriginalFile resolved file exists';
       } else {
         result = 'renameOriginalFile resolved success';
       }
-    } catch (/** @type{Error} */ error) {
+    } catch (/** @type{!Error} */ error) {
       result = `renameOriginalFile failed Error: ${error}`;
     }
   } else if (data.saveCopy) {
-    const existingFile = lastReceivedFileList.item(0);
+    const existingFile = assertCast(lastReceivedFileList).item(0);
     if (!existingFile) {
       result = 'saveCopy failed, no file loaded';
     } else {
@@ -84,15 +92,16 @@ async function runTestQuery(data) {
       result = 'boo yah!';
     }
   } else if (data.getFileErrors) {
-    result = lastReceivedFileList.files.map(file => file.error).join();
+    result =
+        assertCast(lastReceivedFileList).files.map(file => file.error).join();
   }
   return {testQueryResult: result};
 }
 
 /**
  * Acts on TestMessageRunTestCase.
- * @param {TestMessageRunTestCase} data
- * @return {!Promise<TestMessageResponseData>}
+ * @param {!TestMessageRunTestCase} data
+ * @return {!Promise<!TestMessageResponseData>}
  */
 async function runTestCase(data) {
   const testCase = guestTestCases.get(data.testCase);
@@ -107,7 +116,7 @@ async function runTestCase(data) {
  * Registers a test that runs in the guest context. To indicate failure, the
  * test throws an exception (e.g. via assertEquals).
  * @param {string} testName
- * @param {function(): Promise<undefined>} testCase
+ * @param {function(): !Promise<undefined>} testCase
  */
 function GUEST_TEST(testName, testCase) {
   guestTestCases.set(testName, testCase);
@@ -127,7 +136,7 @@ async function signalTestHandlersReady() {
     try {
       await parentMessagePipe.sendMessage('test-handlers-ready', {});
       return;
-    } catch (/** @type {GenericErrorResponse} */ e) {
+    } catch (/** @type {!GenericErrorResponse} */ e) {
       if (e.message !== EXPECTED_ERROR) {
         console.error('Unexpected error in signalTestHandlersReady', e);
         return;
@@ -136,9 +145,10 @@ async function signalTestHandlersReady() {
   }
 }
 
+/** Installs the MessagePipe handlers for receiving test queries. */
 function installTestHandlers() {
   parentMessagePipe.registerHandler('test', (data) => {
-    return runTestQuery(/** @type {TestMessageQueryData} */ (data));
+    return runTestQuery(/** @type {!TestMessageQueryData} */ (data));
   });
   // Turn off error rethrowing for tests so the test runner doesn't mark
   // our error handling tests as failed.
@@ -150,14 +160,14 @@ function installTestHandlers() {
   });
 
   parentMessagePipe.registerHandler('run-test-case', (data) => {
-    return runTestCase(/** @type{TestMessageRunTestCase} */ (data));
+    return runTestCase(/** @type{!TestMessageRunTestCase} */ (data));
   });
 
   parentMessagePipe.registerHandler('get-last-loaded-files', () => {
     //  Note: the `ReceivedFileList` has methods stripped since it gets sent
     //  over a pipe so just send the underlying files.
-    return /** @type {LastLoadedFilesResponse} */ (
-        {fileList: lastReceivedFileList.files});
+    return /** @type {!LastLoadedFilesResponse} */ (
+        {fileList: assertCast(lastReceivedFileList).files});
   });
 
   // Log errors, rather than send them to console.error. This allows the error
@@ -168,10 +178,15 @@ function installTestHandlers() {
 
   // Install spies.
   const realLoadFiles = loadFiles;
-  loadFiles = async (/** !ReceivedFileList */ fileList) => {
+  /**
+   * @param {!ReceivedFileList} fileList
+   * @return {!Promise<undefined>}
+   */
+  async function watchLoadFiles(fileList) {
     lastReceivedFileList = fileList;
     return realLoadFiles(fileList);
-  };
+  }
+  loadFiles = watchLoadFiles;
   signalTestHandlersReady();
 }
 

chromeos/components/web_applications/system_apps.gni

+import("//third_party/closure_compiler/compile_js.gni")
+
+declare_args() {
+  # Whether to enable additional linting of closure type annotations.
+  # Lint checks are not well documented, but pick up some useful stuff.
+  enable_system_app_lint_checks = false
+}
+
+system_app_closure_flags =
+    default_closure_args + [
+      "conformance_configs " +
+          rebase_path("closure_conformance_checks.txt", root_build_dir),
+      "jscomp_error=conformanceViolations",
+
+      "jscomp_error=strictCheckTypes",
+      "language_in=ECMASCRIPT_2018",
+
+      # TODO(crbug/1048973): Remove these when the mojo bindings
+      # js is updated to pass a closure compile check.
+      "hide_warnings_for=mojo/public/js/",
+    ]
+
+if (enable_system_app_lint_checks) {
+  system_app_closure_flags += [
+    "jscomp_error=lintChecks",
+    "hide_warnings_for=mojo/public/interfaces/bindings",
+  ]
+}
+
+system_app_closure_flags_strict =
+    system_app_closure_flags + [ "jscomp_error=reportUnknownTypes" ]