Closure compile all but one js unittest in ui/file_manager/common

Adds
 - FileManagerJsTest.ImporterCommonTest
 - FileManagerJsTest.LRUCacheTest
 - FileManagerJsTest.AsyncUtilTest
 - FileManagerJsTest.FilesAppEntryTypes
 - FileManagerJsTest.UtilTest

Also adds test_importer_common.js to be typechecked, which isn't
itself a unit test. This exposed a weird dependency group on
ui/file_manager/exerns/volume_info.js - encapsulate those dependencies
as well as we can. Remove the dependency on cr.EventListenerType: it's
a @typedef {...|*} so adds no type safety and requires a dependency on
"//ui/webui/resources/js/cr:event_target" which is not an extern.

Bug: 860355
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I479f8e7b22b56ea315c27fabc5e7ccdeb6277728
Reviewed-on: https://chromium-review.googlesource.com/c/1250762
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597017}

chrome/browser/chromeos/file_manager/file_manager_jstest.cc

@@ -46,8 +46,7 @@ IN_PROC_BROWSER_TEST_F(FileManagerJsTest, FileOperationManagerTest) {
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImporterCommonTest) {
-  RunTest(base::FilePath(
-      FILE_PATH_LITERAL("common/js/importer_common_unittest.html")));
+  RunGeneratedTest("/common/js/importer_common_unittest.html");
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImportHistoryTest) {
@@ -76,8 +75,7 @@ IN_PROC_BROWSER_TEST_F(FileManagerJsTest, MediaScannerTest) {
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, LRUCacheTest) {
-  RunTest(base::FilePath(
-      FILE_PATH_LITERAL("common/js/lru_cache_unittest.html")));
+  RunGeneratedTest("/common/js/lru_cache_unittest.html");
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, MediaImportHandlerTest) {
@@ -101,8 +99,7 @@ IN_PROC_BROWSER_TEST_F(FileManagerJsTest, ImportControllerTest) {
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, AsyncUtilTest) {
-  RunTest(base::FilePath(
-      FILE_PATH_LITERAL("common/js/async_util_unittest.html")));
+  RunGeneratedTest("/common/js/async_util_unittest.html");
 }
 
 // TODO(sashab): Re-enable when https://crbug.com/842880 is fixed.
@@ -210,12 +207,11 @@ IN_PROC_BROWSER_TEST_F(FileManagerJsTest, FileListSelectionModelTest) {
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, FilesAppEntryTypes) {
-  RunTest(base::FilePath(
-      FILE_PATH_LITERAL("common/js/files_app_entry_types_unittest.html")));
+  RunGeneratedTest("/common/js/files_app_entry_types_unittest.html");
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, UtilTest) {
-  RunTest(base::FilePath(FILE_PATH_LITERAL("common/js/util_unittest.html")));
+  RunGeneratedTest("/common/js/util_unittest.html");
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerJsTest, Crostini) {

ui/file_manager/BUILD.gn

@@ -51,6 +51,7 @@ group("closure_compile") {
 
 group("unit_test_data") {
   deps = [
+    "file_manager/common/js:unit_tests",
     "file_manager/foreground/js:unit_tests",
     "gallery/js:unit_tests",
     "gallery/js/image_editor:unit_tests",

ui/file_manager/audio_player/js/BUILD.gn

@@ -20,7 +20,6 @@ js_library("closure_compile_externs") {
     "$externs_path/chrome_extensions.js",
     "$externs_path/metrics_private.js",
     "../../externs/audio_player_foreground.js",
-    "../../externs/entry_location.js",
     "../../externs/platform.js",
     "//third_party/analytics/externs.js",
   ]

ui/file_manager/base/js/BUILD.gn

@@ -15,18 +15,13 @@ js_type_check("closure_compile") {
 js_library("filtered_volume_manager") {
   deps = [
     "//ui/file_manager/externs:file_manager_private",
+    "//ui/file_manager/externs:volume_manager",
     "//ui/file_manager/file_manager/common/js:async_util",
     "//ui/file_manager/file_manager/common/js:files_app_entry_types",
     "//ui/file_manager/file_manager/common/js:volume_manager_common",
     "//ui/webui/resources/js:cr",
-    "//ui/webui/resources/js/cr:event_target",
     "//ui/webui/resources/js/cr/ui:array_data_model",
   ]
-  externs_list = [
-    "//ui/file_manager/externs/background/volume_manager_factory.js",
-    "//ui/file_manager/externs/entry_location.js",
-    "//ui/file_manager/externs/volume_info.js",
-    "//ui/file_manager/externs/volume_info_list.js",
-    "//ui/file_manager/externs/volume_manager.js",
-  ]
+  externs_list =
+      [ "//ui/file_manager/externs/background/volume_manager_factory.js" ]
 }

ui/file_manager/externs/BUILD.gn

@@ -28,3 +28,16 @@ js_library("file_manager_private") {
     "$externs_path/file_system_provider.js",
   ]
 }
+
+js_library("volume_manager") {
+  sources = []
+
+  # Encapsulate volume_manager.js and its dependencies. Note this should really
+  # depend on volume_manager_common.js as well, but that's not an extern.
+  externs_list = [
+    "entry_location.js",
+    "volume_info.js",
+    "volume_info_list.js",
+    "volume_manager.js",
+  ]
+}

ui/file_manager/externs/volume_manager.js

@@ -75,7 +75,7 @@ VolumeManager.prototype.getLocationInfo = function(entry) {};
 /**
  * Adds an event listener to the target.
  * @param {string} type The name of the event.
- * @param {EventListenerType} handler The handler for the event. This is
+ * @param {function(!Event)} handler The handler for the event. This is
  *     called when the event is dispatched.
  */
 VolumeManager.prototype.addEventListener = function(type, handler) {};
@@ -83,7 +83,7 @@ VolumeManager.prototype.addEventListener = function(type, handler) {};
 /**
  * Removes an event listener from the target.
  * @param {string} type The name of the event.
- * @param {EventListenerType} handler The handler for the event.
+ * @param {function(!Event)} handler The handler for the event.
  */
 VolumeManager.prototype.removeEventListener = function(type, handler) {};
 

ui/file_manager/file_manager/common/js/BUILD.gn

@@ -3,14 +3,14 @@
 # found in the LICENSE file.
 
 import("//third_party/closure_compiler/compile_js.gni")
+import("//third_party/closure_compiler/js_unit_tests.gni")
 
 # TODO(tapted): This entire folder should move to //ui/file_manager/base.
 visibility = [ "//ui/file_manager/*" ]
 
-js_type_check("closure_compile") {
+js_type_check("closure_compile_module") {
   deps = [
     ":async_util",
-    ":closure_compile_externs",
     ":error_util",
     ":file_type",
     ":files_app_entry_types",
@@ -27,24 +27,34 @@ js_type_check("closure_compile") {
   ]
 }
 
-js_library("closure_compile_externs") {
-  sources = []
-  externs_list = [
-    "../../../externs/background/file_browser_background.js",
-    "../../../externs/background_window.js",
-  ]
+js_library("async_util") {
 }
 
-js_library("async_util") {
+js_library("async_util_unittest") {
+  deps = [
+    ":async_util",
+    ":unittest_util",
+  ]
 }
 
 js_library("error_util") {
 }
 
 js_library("files_app_entry_types") {
+  deps = [
+    "../../../externs:file_manager_private",
+  ]
   externs_list = [ "../../../externs/volume_info.js" ]
 }
 
+js_library("files_app_entry_types_unittest") {
+  deps = [
+    ":files_app_entry_types",
+    ":unittest_util",
+    ":volume_manager_common",
+  ]
+}
+
 js_library("file_type") {
   deps = [
     ":files_app_entry_types",
@@ -52,20 +62,53 @@ js_library("file_type") {
   ]
 }
 
+# These importer files actually belong here. Nothing outside the Files app uses
+# them, so restrict visibility. TODO(tapted): Simplify visibility when
+# everything else moves to //ui/file_manager/base.
 js_library("importer_common") {
+  visibility = []
+  visibility = [ "//ui/file_manager/file_manager/*" ]
   deps = [
     ":file_type",
     ":volume_manager_common",
+    "../../../externs:volume_manager",
   ]
   externs_list = [
+    "//third_party/analytics/externs.js",
     "../../../externs/volume_info_list.js",
-    "../../../externs/volume_manager.js",
+    "../../../externs/background_window.js",
+    "../../../externs/background/file_browser_background.js",
+  ]
+}
+
+js_library("test_importer_common") {
+  deps = [
+    ":importer_common",
+    ":unittest_util",
+  ]
+  visibility = []
+  visibility = [ "//ui/file_manager/file_manager/*" ]
+}
+
+js_library("importer_common_unittest") {
+  deps = [
+    ":mock_entry",
+    ":test_importer_common",
+    ":util",
+    "//ui/file_manager/file_manager/background/js:mock_volume_manager",
   ]
 }
 
 js_library("lru_cache") {
 }
 
+js_library("lru_cache_unittest") {
+  deps = [
+    ":lru_cache",
+    "//ui/webui/resources/js:webui_resource_test",
+  ]
+}
+
 js_library("metrics") {
   deps = [
     ":metrics_base",
@@ -122,8 +165,40 @@ js_library("util") {
   ]
 }
 
+js_library("util_unittest") {
+  deps = [
+    ":mock_entry",
+    ":unittest_util",
+    ":util",
+  ]
+}
+
 js_library("volume_manager_common") {
   deps = [
     "//ui/webui/resources/js:assert",
   ]
 }
+
+js_unit_tests("unit_tests") {
+  deps = [
+    ":async_util_unittest",
+    ":files_app_entry_types_unittest",
+    ":importer_common_unittest",
+    ":lru_cache_unittest",
+    ":util_unittest",
+  ]
+}
+
+js_type_check("test_support_type_check") {
+  deps = [
+    ":test_importer_common",
+  ]
+}
+
+group("closure_compile") {
+  deps = [
+    ":closure_compile_module",
+    ":test_support_type_check",
+    ":unit_tests_type_check",
+  ]
+}

ui/file_manager/file_manager/common/js/async_util_unittest.html

-<!-- Copyright 2014 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.
-  -->
-<script src="async_util.js"></script>
-<script src="unittest_util.js"></script>
-
-<script src="async_util_unittest.js"></script>

ui/file_manager/file_manager/common/js/async_util_unittest.js

@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 /**
- * @type {string} Result
+ * @type {?string} Result
  */
 var result;
 

ui/file_manager/file_manager/common/js/files_app_entry_types_unittest.html

-<!DOCTYPE html>
-<!-- Copyright 2018 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.
-  -->
-<html>
-  <body>
-  <script src="../../common/js/unittest_util.js"></script>
-
-  <script src="files_app_entry_types.js"></script>
-  <script src="volume_manager_common.js"></script>
-  <script src="files_app_entry_types_unittest.js"></script>
-  </body>
-</html>

ui/file_manager/file_manager/common/js/importer_common_unittest.html

-<!DOCTYPE html>
-<!-- Copyright 2014 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.
-  -->
-
-<html>
-<body>
-  <script src="../../../../../ui/webui/resources/js/cr.js"></script>
-  <script src="../../../../../ui/webui/resources/js/cr/event_target.js"></script>
-  <script src="../../../../../ui/webui/resources/js/cr/ui/array_data_model.js"></script>
-  <script src="../../../../../ui/webui/resources/js/load_time_data.js"></script>
-  <script src="volume_manager_common.js"></script>
-  <script src="files_app_entry_types.js"></script>
-  <script src="../../common/js/lru_cache.js"></script>
-  <script src="../../background/js/entry_location_impl.js"></script>
-  <script src="../../background/js/metadata_proxy.js"></script>
-  <script src="../../background/js/mock_file_operation_manager.js"></script>
-  <script src="../../background/js/mock_media_scanner.js"></script>
-  <script src="../../background/js/volume_info_impl.js"></script>
-  <script src="../../background/js/volume_info_list_impl.js"></script>
-  <script src="../../background/js/volume_manager_factory.js"></script>
-  <script src="../../background/js/volume_manager_impl.js"></script>
-  <script src="../../background/js/volume_manager_util.js"></script>
-  <script src="../../background/js/mock_volume_manager.js"></script>
-  <script src="unittest_util.js"></script>
-  <script src="test_importer_common.js"></script>
-  <script src="mock_entry.js"></script>
-  <script src="async_util.js"></script>
-  <script src="file_type.js"></script>
-  <script src="util.js"></script>
-  <script src="importer_common.js"></script>
-
-  <script src="importer_common_unittest.js"></script>
-</body>
-</html>

ui/file_manager/file_manager/common/js/importer_common_unittest.js

@@ -3,28 +3,28 @@
 // found in the LICENSE file.
 
 /** @type {!MockVolumeManager} */
-var volumeManager;
+let volumeManager;
 
 /** @type {!VolumeInfo} */
-var cameraVolume;
+let cameraVolume;
 
 /** @type {!VolumeInfo} */
-var sdVolume;
+let sdVolume;
 
-/** @type {!VolumeInfo} */
-var driveVolume;
+/** @type {VolumeInfo} */
+let driveVolume;
 
 /** @type {!MockFileEntry} */
-var cameraFileEntry;
+let cameraFileEntry;
 
 /** @type {!MockFileEntry} */
-var rawFileEntry;
+let rawFileEntry;
 
 /** @type {!MockFileEntry} */
-var sdFileEntry;
+let sdFileEntry;
 
 /** @type {!MockFileEntry} */
-var driveFileEntry;
+let driveFileEntry;
 
 // Sadly, boilerplate setup necessary to include test support classes.
 loadTimeData.data = {
@@ -204,7 +204,7 @@ function testRotateLogs(callback) {
             // Verify the *active* log is deleted.
             assertEquals(0, fileName.search(/[0-9]{6}-import-debug-0.log/),
                 'Filename (' + fileName + ') does not match next pattern.');
-            driveFileEntry.assertRemoved();
+            driveFileEntry.asMock().assertRemoved();
 
             return importer.ChromeLocalStorage.getInstance()
                   .get(importer.Setting.LAST_KNOWN_LOG_ID)
@@ -216,10 +216,16 @@ function testRotateLogs(callback) {
 
 function testRotateLogs_RemembersInitialActiveLog(callback) {
   var nextLogId = 1;
-  var fileFactory = assertFalse;  // Should not be called.
-  var promise = importer.rotateLogs(nextLogId, assertFalse)
-      .then(
-          function() {
+
+  // Should not be called.
+  var fileFactory = (namePromise) => {
+    assertFalse(true);
+    return Promise.resolve();
+  };
+
+  var promise =
+      importer.rotateLogs(nextLogId, fileFactory)
+          .then(function() {
             return importer.ChromeLocalStorage.getInstance()
                   .get(importer.Setting.LAST_KNOWN_LOG_ID)
                   .then(assertEquals.bind(null, nextLogId));
@@ -244,7 +250,7 @@ function testDeflateAppUrl() {
   // And finally that we can reconstitute the original.
   assertEquals(url, importer.inflateAppUrl(deflated),
       'Deflated then inflated URLs must match original URL.');
-};
+}
 
 function testHasMediaDirectory(callback) {
   var dir = createDirectoryEntry(sdVolume, '/DCIM');
@@ -267,12 +273,11 @@ function assertIsNotMediaDir(path) {
 }
 
 function createFileEntry(volume, path) {
-  var entry = new MockFileEntry(
-      volume.fileSystem,
-      path, {
-        size: 1234,
-        modificationTime: new Date().toString()
-      });
+  var entry =
+      new MockFileEntry(volume.fileSystem, path, /** @type{Metadata} */ ({
+                          size: 1234,
+                          modificationTime: new Date().toString()
+                        }));
   // Ensure the file entry has a volumeID...necessary for lookups
   // via the VolumeManager.
   entry.volumeId = volume.volumeId;

ui/file_manager/file_manager/common/js/lru_cache_unittest.html

-<!DOCTYPE html>
-<!-- Copyright 2014 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.
-  -->
-
-<html>
-<body>
-  <script src="lru_cache.js"></script>
-  <script src="lru_cache_unittest.js"></script>
-</body>
-</html>

ui/file_manager/file_manager/common/js/mock_entry.js

@@ -281,6 +281,15 @@ MockFileEntry.prototype.clone = function(path, opt_filesystem) {
       opt_filesystem || this.filesystem, path, this.metadata, this.content);
 };
 
+/**
+ * Helper to expose methods mixed in via MockEntry to the type checker.
+ *
+ * @return {!MockEntry}
+ */
+MockFileEntry.prototype.asMock = function() {
+  return /** @type{!MockEntry} */ (/** @type(*) */ (this));
+};
+
 /**
  * Mock class for DirectoryEntry.
  *

ui/file_manager/file_manager/common/js/test_importer_common.js

@@ -9,6 +9,7 @@ var importer = importer || {};
  * Sets up a logger for use in unit tests.  The test logger doesn't attempt to
  * access chrome's sync file system.  Call this during setUp.
  * @return {!importer.TestLogger}
+ * @suppress{accessControls} For testing.
  */
 importer.setupTestLogger = function() {
   var logger = new importer.TestLogger();

ui/file_manager/file_manager/common/js/unittest_util.js

@@ -101,11 +101,11 @@ function TestCallRecorder() {
   this.calls_ = [];
 
   /**
-   * The recording funciton. Bound in our constructor to ensure we always
+   * The recording function. Bound in our constructor to ensure we always
    * return the same object. This is necessary as some clients may make use
    * of object equality.
    *
-   * @type {function()}
+   * @type {function(*)}
    */
   this.callback = this.recordArguments_.bind(this);
 }

ui/file_manager/file_manager/common/js/util_unittest.html

-<!DOCTYPE html>
-<!-- Copyright 2018 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.
-  -->
-
-<html>
-<body>
-  <script src="mock_entry.js"></script>
-  <script src="util.js"></script>
-  <script src="util_unittest.js"></script>
-</body>
-</html>

ui/file_manager/file_manager/foreground/js/metadata/BUILD.gn

@@ -148,10 +148,7 @@ js_library("metadata_model") {
     ":metadata_provider",
     ":multi_metadata_provider",
     "../../../common/js:util",
-  ]
-  externs_list = [
-    "../../../../externs/volume_info_list.js",
-    "../../../../externs/volume_manager.js",
+    "//ui/file_manager/externs:volume_manager",
   ]
 }
 
@@ -190,8 +187,8 @@ js_library("multi_metadata_provider") {
     ":external_metadata_provider",
     ":file_system_metadata_provider",
     ":metadata_provider",
+    "//ui/file_manager/externs:volume_manager",
   ]
-  externs_list = [ "../../../../externs/volume_manager.js" ]
 }
 
 js_library("thumbnail_model") {

ui/file_manager/file_manager/foreground/js/ui/BUILD.gn

@@ -59,7 +59,6 @@ js_library("closure_compile_externs") {
     "../../../../externs/css_rule.js",
     "../../../../externs/drag_target.js",
     "../../../../externs/entries_changed_event.js",
-    "../../../../externs/entry_location.js",
     "../../../../externs/gallery_foreground.js",
     "../../../../externs/menu_item_update_event.js",
     "../../../../externs/paper_elements.js",
@@ -308,8 +307,8 @@ js_library("location_line") {
     "../../../common/js:metrics",
     "../../../common/js:util",
     "../../../common/js:volume_manager_common",
+    "//ui/file_manager/externs:volume_manager",
   ]
-  externs_list = [ "../../../../externs/volume_manager.js" ]
 }
 
 js_library("multi_profile_share_dialog") {

ui/file_manager/gallery/js/BUILD.gn

@@ -135,10 +135,7 @@ js_library("gallery_util") {
     "../../file_manager/common/js:file_type",
     "../../file_manager/common/js:util",
     "../../file_manager/common/js:volume_manager_common",
-  ]
-  externs_list = [
-    "../../externs/volume_manager.js",
-    "../../externs/volume_info_list.js",
+    "//ui/file_manager/externs:volume_manager",
   ]
 }
 

ui/file_manager/video_player/js/BUILD.gn

@@ -23,7 +23,6 @@ js_library("closure_compile_externs") {
     "$externs_path/media_player_private.js",
     "$externs_path/metrics_private.js",
     "../../externs/chrome_cast.js",
-    "../../externs/entry_location.js",
     "../../externs/platform.js",
     "//third_party/analytics/externs.js",
   ]

ui/file_manager/video_player/js/cast/BUILD.gn

@@ -23,11 +23,7 @@ js_library("closure_compile_externs") {
     "../../../externs/app_window_common.js",
     "../../../externs/background/volume_manager_factory.js",
     "../../../externs/chrome_cast.js",
-    "../../../externs/entry_location.js",
     "../../../externs/platform.js",
-    "../../../externs/volume_info.js",
-    "../../../externs/volume_info_list.js",
-    "../../../externs/volume_manager.js",
     "//third_party/analytics/externs.js",
   ]
 }