New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 860355



Sign in to add a comment
link

Issue 914197: Closure compile import_controller_unittest

Reported by noel@chromium.org, Dec 12 Project Member

Issue description

- fix dependent FilesApp foreground BUILD rules
  - they lack backend externs_list= dependencies rules but they use
    backend classes: Crosini, ProgressCenter, and so on
  - some BUILD rules include the world, aka ui:file_manager_ui, but
    they neither need nor use the world, so prune the dependencies
    down to the actual things needed

- fix lack of externs file defines & decls for Importer components
  - externs for MediaImportHandler are half-baked, wrong, missing
  - fix brokeness, and at least make them right
  - adding full externs for the various components is interesting,
    but is a better subject of future work

- background mock_media_scanner foreground test visible
  - allow this class to be used in the foreground unittests, and
    only one test needs it (the subject of this bug)

- make the import_controller_unittest.js Closure-sane
  - this file is woefully out-of-date and a reason why we need
     to enusre our code is continously Closure checked 
  - it is full of @types for an "impoter." class and similar
    - spelling mistakes in the @type defines "ImportControler"
  - it is full of undefined @types annotations
    - the @type class / type referred to no longer exist
  - defines @return on functions that have no return value
 
- finally, remove the current html unittest
  - closure compile the code and auto-generate the unittest
 

Comment 1 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2dc2e737e9c0950ffdd8a2855f46dda5e13428a9

commit 2dc2e737e9c0950ffdd8a2855f46dda5e13428a9
Author: Noel Gordon <noel@chromium.org>
Date: Wed Dec 12 02:43:03 2018

Fix foreground file_tasks BUILD rule

 - FileTasks uses Crostini in its contructor
  - add externs_list deps to externs backend Crostini

No change in test behavior.

Test: browser_tests --gtest_filter="FileManagerJsTest*FileTasks*"
Bug:  914197 
Change-Id: I3c8c925cd7ffa24420c2986b624dcc5870b08783
Reviewed-on: https://chromium-review.googlesource.com/c/1372990
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615794}
[modify] https://crrev.com/2dc2e737e9c0950ffdd8a2855f46dda5e13428a9/ui/file_manager/file_manager/foreground/js/BUILD.gn

Comment 2 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/620164346f4488cdb1604b19e82ae65a09d268e3

commit 620164346f4488cdb1604b19e82ae65a09d268e3
Author: Noel Gordon <noel@chromium.org>
Date: Wed Dec 12 04:06:13 2018

Fix foreground file_manager_commands and import_controller BUILD rules

 - ImportController @types refer to {CommandHandlerDeps}
  - add externs_list deps UI externs/command_handler_deps.js
 - And the same for FileManagerCommands

No change in test behavior.

Bug:  914197 
Change-Id: I9f71b677f22961d0e14ac55834ca0a9b0cea180a
Reviewed-on: https://chromium-review.googlesource.com/c/1373051
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615810}
[modify] https://crrev.com/620164346f4488cdb1604b19e82ae65a09d268e3/ui/file_manager/file_manager/foreground/js/BUILD.gn

Comment 3 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a5a88b153d150440d511f513f8cbf25e34114c8

commit 4a5a88b153d150440d511f513f8cbf25e34114c8
Author: Noel Gordon <noel@chromium.org>
Date: Wed Dec 12 05:06:21 2018

Fix foreground file_transfer_controller BUILD rule

 - FileTransferController uses ProgressCenter in its contructor
   - add externs_list deps to externs backend ProgressCenter

No change in test behavior.

Bug:  914197 
Change-Id: I12d91732edd9fec52c44af54d9c88a6e9f98719a
Reviewed-on: https://chromium-review.googlesource.com/c/1373050
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615815}
[modify] https://crrev.com/4a5a88b153d150440d511f513f8cbf25e34114c8/ui/file_manager/file_manager/foreground/js/BUILD.gn

Comment 4 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa221f1b03c3bd2d01957f12f2922cf73a0a6cda

commit aa221f1b03c3bd2d01957f12f2922cf73a0a6cda
Author: Noel Gordon <noel@chromium.org>
Date: Wed Dec 12 13:17:14 2018

Make mock_media_scanner available to foreground tests

Foreground import_controller_unittest needs a mock MediaScanner. Allow
background {MockMediaScanner} to be used by foreground tests.

No change in test behavior.

Tbr: slangley@
Bug:  914197 
Change-Id: I146bff5e4ef5f12df5a060b3efedb1c2e1ea378e
Reviewed-on: https://chromium-review.googlesource.com/c/1373589
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615870}
[modify] https://crrev.com/aa221f1b03c3bd2d01957f12f2922cf73a0a6cda/ui/file_manager/file_manager/background/js/BUILD.gn

Comment 5 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98b9f1efa71855c361059ecbf9bb8e598bc6fda5

commit 98b9f1efa71855c361059ecbf9bb8e598bc6fda5
Author: Noel Gordon <noel@chromium.org>
Date: Thu Dec 13 01:37:11 2018

Add background externs media_import_handler.js to the Closure BUILD

This change adds media_import_handler.js to the Closure BUILD and type
checks it and also the externs it uses. Immediate result: many Closure
compile errors since the types it uses do not have an externs file, or
the constructor signatures are incomplete or just plain wrong [1].

[1] importer.MediaImportHandler.ImportTask @extends TaskQueue.BaseTask
but the constructor signature in the externs file does not (Closure is
not silly, and spots that error). Next issue: TaskQueue.* doesn't have
an externs file, which one might use to fix the first error.

Typical fix would be to use Impl pattern, add three new externs files,
etc. The result would be a large, intrusive change. Simpler, type-safe
methods are sought and are readily available.

Closure externs can forward declare dependent class types, even define
their constructors, to allow the constructor signature of the class of
interest to be fully defined, complete with the dependent @param types
that Closure can enforce.

Use this fact in externs media_import_handler.js: forward declare, and
define the constructors / interfaces as needed, of the dependent types
namely, TaskQueue.*, DispositionChecker.*, that do not have an externs
file. Define importer.MediaImportHandler with @params for Closure type
checking.

What about the implementations? Simply update the implementation JSDoc
with one additional annotation line:

  @return undefined

With that, Closure type-checks the constructor definition by comparing
it with the signatures defined in the externs file, and concludes that
it does match, that it does not "re-define" any externs file types.

No change in test behavior.

Bug:  914197 
Change-Id: I15c02cd4ab83c19bef1882e77702e975fa1fdc2e
Reviewed-on: https://chromium-review.googlesource.com/c/1373893
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616151}
[modify] https://crrev.com/98b9f1efa71855c361059ecbf9bb8e598bc6fda5/ui/file_manager/externs/background/import_runner.js
[modify] https://crrev.com/98b9f1efa71855c361059ecbf9bb8e598bc6fda5/ui/file_manager/externs/background/media_import_handler.js
[modify] https://crrev.com/98b9f1efa71855c361059ecbf9bb8e598bc6fda5/ui/file_manager/file_manager/background/js/BUILD.gn
[modify] https://crrev.com/98b9f1efa71855c361059ecbf9bb8e598bc6fda5/ui/file_manager/file_manager/background/js/duplicate_finder.js
[modify] https://crrev.com/98b9f1efa71855c361059ecbf9bb8e598bc6fda5/ui/file_manager/file_manager/background/js/media_import_handler.js
[modify] https://crrev.com/98b9f1efa71855c361059ecbf9bb8e598bc6fda5/ui/file_manager/file_manager/background/js/task_queue.js

Comment 6 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d54599dd95e2ed173adacd3bea194ba0cf9503e1

commit d54599dd95e2ed173adacd3bea194ba0cf9503e1
Author: Noel Gordon <noel@chromium.org>
Date: Thu Dec 13 03:31:56 2018

import_controller_unittest: add a default metadata helper

Add a default metadata helper routine, and use it at call sites.

Bug:  914197 
Change-Id: Ib1de53438db145e5f266db92cce35bef211c83f9
Reviewed-on: https://chromium-review.googlesource.com/c/1375170
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616193}
[modify] https://crrev.com/d54599dd95e2ed173adacd3bea194ba0cf9503e1/ui/file_manager/file_manager/foreground/js/import_controller_unittest.js

Comment 7 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9fdbed2fa2e6b03ce7f34a8a9bcdbe9fbec30ceb

commit 9fdbed2fa2e6b03ce7f34a8a9bcdbe9fbec30ceb
Author: Noel Gordon <noel@chromium.org>
Date: Thu Dec 13 10:28:57 2018

import_controller_unittest: clean-up preamble and SetUp

 - add standard JSDoc used elsewhere for window.metrics
 - use local variable testFileSystem, to exchange 2 lines for 3
 - volumeManager.getCurrentProfileVolumeInfo can return null so
   add an assert that it's never null in these tests

No change in test behavior.

Tbr: sammc@
Bug:  914197 
Change-Id: I2ea6314b57f55224f7c26050591bd551b6f56bb8
Reviewed-on: https://chromium-review.googlesource.com/c/1375199
Reviewed-by: Noel Gordon <noel@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616262}
[modify] https://crrev.com/9fdbed2fa2e6b03ce7f34a8a9bcdbe9fbec30ceb/ui/file_manager/file_manager/foreground/js/import_controller_unittest.js

Comment 8 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ca6800c1b1886228c03b465823e5f88444a7f86

commit 6ca6800c1b1886228c03b465823e5f88444a7f86
Author: Noel Gordon <noel@chromium.org>
Date: Thu Dec 13 14:03:53 2018

import_controller_unittest: spell correct, remove, update @types

 - spell correct impoter => importer
 - remove @param {!DirectoryEntry} destination
   - the affected functions don't have parameters
 - fix obvious @type errors
   - event listener functions receive an {!Event}
   - arrays are @type {!Array<whatever>} for sure
 - remove importer.CommandUpdate
   - this @type does not exist in the code base
 - importer.ImportControler => importer.ImportController
 - add TODO to determine class @override types
 - auto-format the code

Bug:  914197 
Change-Id: Ib225ed6030e70c0676f04f4f730e72e6b48eec4b
Reviewed-on: https://chromium-review.googlesource.com/c/1375201
Commit-Queue: Noel Gordon <noel@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616299}
[modify] https://crrev.com/6ca6800c1b1886228c03b465823e5f88444a7f86/ui/file_manager/file_manager/foreground/js/import_controller_unittest.js

Comment 9 by bugdroid1@chromium.org, Dec 13

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7935ad7fcaaed7392a7c0efff7625ade0354d045

commit 7935ad7fcaaed7392a7c0efff7625ade0354d045
Author: Noel Gordon <noel@chromium.org>
Date: Thu Dec 13 23:46:43 2018

Closure compile import_controller_unittest

 - remove import_controller_unittest.html
 - add BUILD rules to auto-generate and compile this unittest
 - update comments, remove unused functions, code re-format
 - fix all Closure compile errors

No change in test behavior.

Bug:  914197 
Change-Id: Ida12573fc640dcc97325b1240bedbbf970ff1f35
Reviewed-on: https://chromium-review.googlesource.com/c/1375301
Reviewed-by: Stuart Langley <slangley@chromium.org>
Commit-Queue: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616501}
[modify] https://crrev.com/7935ad7fcaaed7392a7c0efff7625ade0354d045/chrome/browser/chromeos/file_manager/file_manager_jstest.cc
[modify] https://crrev.com/7935ad7fcaaed7392a7c0efff7625ade0354d045/ui/file_manager/file_manager/foreground/js/BUILD.gn
[delete] https://crrev.com/125a005b04c3a68f5c763df5abfa661b3ae70292/ui/file_manager/file_manager/foreground/js/import_controller_unittest.html
[modify] https://crrev.com/7935ad7fcaaed7392a7c0efff7625ade0354d045/ui/file_manager/file_manager/foreground/js/import_controller_unittest.js

Comment 10 by noel@chromium.org, Dec 14

Status: Fixed (was: Started)

Sign in to add a comment