Prototype implementation of MediaImportHandler.

Prototype implementation of MediaImportHandler.

The MediaImportHandler is the overall framework class for coordinating the
import of media from attached media devices to Drive.  This initial
implementation sketches out the process, using FileOperationManager to provide
the actual copy functionality.
- UI and checks for opt-in to Drive->Photos sync and Photos->Drive sync are just
  stubbed out as TODOs.
- The use of FileOperationManager means that the import process is surfaced as a
  normal copy operaion, which is not the final goal.  Follow-up work is needed
  to implement a separate import queue and associated task management and UI.

BUG=420680
Committed: https://crrev.com/157ed95ab034b76ac8491b9f926051d6e9ac9055
Cr-Commit-Position: refs/heads/master@{#307076}

[Ben Kwa, 2014-12-03 22:31:44]

kenobi@chromium.org changed reviewers:
+ hirono@chromium.org

[hirono, 2014-12-04 04:44:26]

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:41:
'/root/',
nit: Indent should be 2 for array literals.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:50:
function tearDown() {
You don't need to add tearDown if you don't use it.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:69:
'/DCIM/photos0/IMG00001.jpg',
nit: Indent should be 2 for array literals.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:80:
return fileSystem.entries[filename]; });
nit: Just a style issue. Please put "});" at the next line.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:109:
function equalFileLists(list0, list1) {
Maybe creating assertEntryListEquals is better for logging.
We can output which entries are different in the two lists.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:110:
if (list0.length != list1.length) {
nit: We prefer !== in our code.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:110:
if (list0.length != list1.length) {
nit: We can drop {} if the condition and the body are one line.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:120:
return paths0.every(function(path) {
Is it OK ['/A', '/A', '/B'] equals ['/A', '/B', '/B'] ?

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js (right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js:88:
errorIf(files.length !== 0);
Please use assert and reportPromise.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js:257:
errorIf(false);
Please use assert and reportPromise.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js (right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:37:
MockFileSystem.create = function(volumeId) {
It looks convenient. Could you please move the lines to the constructor of
MockFileSystem?

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:55: if (subpath &&
!(subpath in this.entries)) {
nit: Please drop {}

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:61: if
(!/\/$/.test(path)) {
nit: Please drop {}

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:62:
this.entries[path] = new MockFileEntry(this, path);
Entry should take metadata.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:245: var result =
this.filesystem.entries[fullPath];
Thank you for implementing this!

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:21:
/** @type {!Array<string>} */
!Array.<string>

Maybe @const and @private?

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:24:
/** @type {Function|undefined} */
Please assign null, and update the type annotation to @type {?Function}.
We would not like to mix null and undefined, and would like to use null for
defined but uninitialized variable.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:40:
* @param {Array.<Entry>} sourceEntries Entries of the source files.
{!Array.<Entry>} ?

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:41:
* @param {DirectoryEntry} targetEntry The destination entry of the target
{!DirectoryEntry} ?

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:62:
* @return {Promise<Object>} A promise that is resolved the next time #paste is
Promise.<Object>

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:67:
this.pasteResolver = resolve;
Please ensure this.pasteResolver == null.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:86:
return this.generatedTaskIds.indexOf(id) != -1;
nit: We prefer !==

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:7: *
@constructor
@struct?

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:11:
this.results = [];
Please add '_' at the end for private members.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:16: *
@return {!Promise<!Array<!FileEntry>>}
!Promise.<!Array.<!FileEntry>>

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:24: *
@param {!Array<!FileEntry>} entries
!Array.<!FileEntry>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:88: * @type
{MediaImportHandler}
@type {!MediaImporthandler}
@const

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:43: * @return
{!Promise<!DirectoryEntry>}
!Promise.<!DirectoryEntry>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:58:
drive.fileSystem.root.getDirectory(
Does it work?
IIUC, drive.fileSystem.root just has '/root' and '/other'. And the unparented
directory is located under '/drive/other'.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:70: * @return
{!Promise<!DirectoryEntry>}
!Promise.<!DirectoryEntry>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:77: * @return
{!Promise<!DirectoryEntry>} The created directory.
!Promise.<!DirectoryEntry>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:108:
.catch(function(e) { console.error(e); });
It turns rejected promise to fulfilled promise. The error does not convey to the
outside of the function. Is it intentional?

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:114: *
@return {!Promise<!DirectoryEntry>}
!Promise.<!DirectoryEntry>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:122: * @param
{!DirectoryEntry} directory Source dir containing media for import.
Could you please add comment the task is not a task of FileOperationManager.
When instantiate the task, FileOperationManager generates another CopyTask in
it. It looks a bit confusing.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:138:
MediaImportHandler.getImportDestination(),
We use 2 spaces indent for array literals.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array<!Entry>} entries A list of file and directory entries.  File
!Array.<!Entry>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:18: * @return
{!Promise<!Array<!FileEntry>>}
!Promise.<!Array.<!FileEntry>>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:34: * @param
{!Array<!Array<!Entry>>} array
!Array.<!Array.<!Entry>>

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:35: * @return
{!Array<!Entry>}
!Array.<!Entry>

[Ben Kwa, 2014-12-04 07:43:49]

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:21:
/** @type {!Array<string>} */
On 2014/12/04 04:44:24, hirono wrote:
> !Array.<string>
> 
> Maybe @const and @private?

Regarding the {Foo.<Bar>} vs {Foo<Bar>} syntax, the compiler recently started
accepting the latter form, which looks cleaner.  Do you guys want to adopt it? 
Or do you want me to just stick with {Foo.<Bar>} for consistency?

[Ben Kwa, 2014-12-04 07:48:08]

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js (right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js:88:
errorIf(files.length !== 0);
On 2014/12/04 04:44:23, hirono wrote:
> Please use assert and reportPromise.

Okay, but can I make that change in a separate CL, since it's orthogonal to what
I'm doing here?

[hirono, 2014-12-04 08:42:41]

On 2014/12/04 07:43:49, Ben Kwa wrote:
>
https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
> File
> chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js
> (right):
> 
>
https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
>
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:21:
> /** @type {!Array<string>} */
> On 2014/12/04 04:44:24, hirono wrote:
> > !Array.<string>
> > 
> > Maybe @const and @private?
> 
> Regarding the {Foo.<Bar>} vs {Foo<Bar>} syntax, the compiler recently started
> accepting the latter form, which looks cleaner.  Do you guys want to adopt it?

> Or do you want me to just stick with {Foo.<Bar>} for consistency?

Ah, I didn't know about it. So please keep the annotation as it is.

[hirono, 2014-12-04 08:42:54]

On 2014/12/04 07:48:08, Ben Kwa wrote:
>
https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
> File chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js
(right):
> 
>
https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
> chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js:88:
> errorIf(files.length !== 0);
> On 2014/12/04 04:44:23, hirono wrote:
> > Please use assert and reportPromise.
> 
> Okay, but can I make that change in a separate CL, since it's orthogonal to
what
> I'm doing here?

SGTM.

[Ben Kwa, 2014-12-04 19:53:02]

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:41:
'/root/',
On 2014/12/04 04:44:22, hirono wrote:
> nit: Indent should be 2 for array literals.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:50:
function tearDown() {
On 2014/12/04 04:44:22, hirono wrote:
> You don't need to add tearDown if you don't use it.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:69:
'/DCIM/photos0/IMG00001.jpg',
On 2014/12/04 04:44:22, hirono wrote:
> nit: Indent should be 2 for array literals.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:80:
return fileSystem.entries[filename]; });
On 2014/12/04 04:44:23, hirono wrote:
> nit: Just a style issue. Please put "});" at the next line.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:109:
function equalFileLists(list0, list1) {
On 2014/12/04 04:44:23, hirono wrote:
> Maybe creating assertEntryListEquals is better for logging.
> We can output which entries are different in the two lists.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:110:
if (list0.length != list1.length) {
On 2014/12/04 04:44:22, hirono wrote:
> nit: We prefer !== in our code.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:110:
if (list0.length != list1.length) {
On 2014/12/04 04:44:23, hirono wrote:
> nit: We can drop {} if the condition and the body are one line.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_import_handler_unittest.js:120:
return paths0.every(function(path) {
On 2014/12/04 04:44:22, hirono wrote:
> Is it OK ['/A', '/A', '/B'] equals ['/A', '/B', '/B'] ?

Good catch, thanks!  Fixed.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js (right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js:88:
errorIf(files.length !== 0);
On 2014/12/04 04:44:23, hirono wrote:
> Please use assert and reportPromise.

Acknowledged.  Will fix in a follow-up CL.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/media_scanner_unittest.js:257:
errorIf(false);
On 2014/12/04 04:44:23, hirono wrote:
> Please use assert and reportPromise.

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js (right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:37:
MockFileSystem.create = function(volumeId) {
On 2014/12/04 04:44:23, hirono wrote:
> It looks convenient. Could you please move the lines to the constructor of
> MockFileSystem?

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:55: if (subpath &&
!(subpath in this.entries)) {
On 2014/12/04 04:44:24, hirono wrote:
> nit: Please drop {}

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:61: if
(!/\/$/.test(path)) {
On 2014/12/04 04:44:24, hirono wrote:
> nit: Please drop {}

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:62:
this.entries[path] = new MockFileEntry(this, path);
On 2014/12/04 04:44:23, hirono wrote:
> Entry should take metadata.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_entry.js:245: var result =
this.filesystem.entries[fullPath];
On 2014/12/04 04:44:23, hirono wrote:
> Thank you for implementing this!

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:21:
/** @type {!Array<string>} */
On 2014/12/04 04:44:24, hirono wrote:
> !Array.<string>
> 
> Maybe @const and @private?

Acknowledged- sticking with {Foo<Bar>}, as discussed offline.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:24:
/** @type {Function|undefined} */
On 2014/12/04 04:44:24, hirono wrote:
> Please assign null, and update the type annotation to @type {?Function}.
> We would not like to mix null and undefined, and would like to use null for
> defined but uninitialized variable.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:40:
* @param {Array.<Entry>} sourceEntries Entries of the source files.
On 2014/12/04 04:44:24, hirono wrote:
> {!Array.<Entry>} ?

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:41:
* @param {DirectoryEntry} targetEntry The destination entry of the target
On 2014/12/04 04:44:24, hirono wrote:
> {!DirectoryEntry} ?

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:62:
* @return {Promise<Object>} A promise that is resolved the next time #paste is
On 2014/12/04 04:44:24, hirono wrote:
> Promise.<Object>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:67:
this.pasteResolver = resolve;
On 2014/12/04 04:44:24, hirono wrote:
> Please ensure this.pasteResolver == null.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_file_operation_manager.js:86:
return this.generatedTaskIds.indexOf(id) != -1;
On 2014/12/04 04:44:24, hirono wrote:
> nit: We prefer !==

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js
(right):

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:7: *
@constructor
On 2014/12/04 04:44:24, hirono wrote:
> @struct?

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:11:
this.results = [];
On 2014/12/04 04:44:25, hirono wrote:
> Please add '_' at the end for private members.

Done.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:16: *
@return {!Promise<!Array<!FileEntry>>}
On 2014/12/04 04:44:25, hirono wrote:
> !Promise.<!Array.<!FileEntry>>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:24: *
@param {!Array<!FileEntry>} entries
On 2014/12/04 04:44:24, hirono wrote:
> !Array.<!FileEntry>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:88: * @type
{MediaImportHandler}
On 2014/12/04 04:44:25, hirono wrote:
> @type {!MediaImporthandler}
> @const

I fixed the nullable thing - thanks.
For the const part, the convention I'm accustomed to is to mark something as
const only if it's effectively immutable, which is not my intention (e.g. the
import task queue is going to go in there).

Also, I took the liberty of going through the rest of the FileBrowserBackground
initialization code and cleaning/fixing up some of the other type annotations,
PTAL.  Thanks!

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:43: * @return
{!Promise<!DirectoryEntry>}
On 2014/12/04 04:44:26, hirono wrote:
> !Promise.<!DirectoryEntry>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:58:
drive.fileSystem.root.getDirectory(
On 2014/12/04 04:44:26, hirono wrote:
> Does it work?
> IIUC, drive.fileSystem.root just has '/root' and '/other'. And the unparented
> directory is located under '/drive/other'.

Yup, this works - it's basically just getting the '/root' entry from the drive
filesystem root.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:70: * @return
{!Promise<!DirectoryEntry>}
On 2014/12/04 04:44:25, hirono wrote:
> !Promise.<!DirectoryEntry>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:77: * @return
{!Promise<!DirectoryEntry>} The created directory.
On 2014/12/04 04:44:25, hirono wrote:
> !Promise.<!DirectoryEntry>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:108:
.catch(function(e) { console.error(e); });
On 2014/12/04 04:44:26, hirono wrote:
> It turns rejected promise to fulfilled promise. The error does not convey to
the
> outside of the function. Is it intentional?

Oops, this was debug code that I meant to remove.  Thanks!

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:114: *
@return {!Promise<!DirectoryEntry>}
On 2014/12/04 04:44:25, hirono wrote:
> !Promise.<!DirectoryEntry>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:122: * @param
{!DirectoryEntry} directory Source dir containing media for import.
On 2014/12/04 04:44:26, hirono wrote:
> Could you please add comment the task is not a task of FileOperationManager.
> When instantiate the task, FileOperationManager generates another CopyTask in
> it. It looks a bit confusing.

Done.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:138:
MediaImportHandler.getImportDestination(),
On 2014/12/04 04:44:26, hirono wrote:
> We use 2 spaces indent for array literals.

Done.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array<!Entry>} entries A list of file and directory entries.  File
On 2014/12/04 04:44:26, hirono wrote:
> !Array.<!Entry>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:18: * @return
{!Promise<!Array<!FileEntry>>}
On 2014/12/04 04:44:26, hirono wrote:
> !Promise.<!Array.<!FileEntry>>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:34: * @param
{!Array<!Array<!Entry>>} array
On 2014/12/04 04:44:26, hirono wrote:
> !Array.<!Array.<!Entry>>

Acknowledged.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:35: * @return
{!Array<!Entry>}
On 2014/12/04 04:44:26, hirono wrote:
> !Array.<!Entry>

Acknowledged.

[Ben Kwa, 2014-12-04 23:03:21]

+cc smckay

Spoke with Steve offline and he suggested two changes:
- move the MediaImportHandler into the importer namespace;
- pull the import destination code out of the MediaImportHandler and make it
pluggable.

PTAL.

[Steve McKay, 2014-12-04 23:47:58]

smckay@chromium.org changed reviewers:
+ smckay@chromium.org

[Steve McKay, 2014-12-04 23:47:59]

https://codereview.chromium.org/762593006/diff/60001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js
(right):

https://codereview.chromium.org/762593006/diff/60001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/mocks/mock_media_scanner.js:10:
function MockMediaScanner() {
This is a Stub, not a mock. Though my standard advice is to not use *specific*
test double names, just use the "Test" prefix.

MockMediaScanner > TestMediaScanner.

Test double names: http://www.martinfowler.com/bliki/TestDouble.html

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array<!Entry>} entries A list of file and directory entries.  File
!Array.<!Entry>.

[Dan Beam, 2014-12-04 23:54:10]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2014-12-04 23:54:10]

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array<!Entry>} entries A list of file and directory entries.  File
On 2014/12/04 23:47:59, Steve McKay wrote:
> !Array.<!Entry>.

either syntax works and just <> is shorter.  any reason why you're asking for
.<>?  I've been considering changing all code in chrome from .<> to <>.

[Steve McKay, 2014-12-04 23:58:01]

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array<!Entry>} entries A list of file and directory entries.  File
On 2014/12/04 23:54:10, Dan Beam wrote:
> On 2014/12/04 23:47:59, Steve McKay wrote:
> > !Array.<!Entry>.
> 
> either syntax works and just <> is shorter.  any reason why you're asking for
> .<>?  I've been considering changing all code in chrome from .<> to <>.

".<>" is the form used by Files.app. If the team wants to switch fine by me, but
the team needs to make that decision.

[Ben Kwa, 2014-12-04 23:59:19]

On 2014/12/04 23:58:01, Steve McKay wrote:
>
https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
> File ui/file_manager/file_manager/background/js/media_scanner.js (right):
> 
>
https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
> ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
> {!Array<!Entry>} entries A list of file and directory entries.  File
> On 2014/12/04 23:54:10, Dan Beam wrote:
> > On 2014/12/04 23:47:59, Steve McKay wrote:
> > > !Array.<!Entry>.
> > 
> > either syntax works and just <> is shorter.  any reason why you're asking
for
> > .<>?  I've been considering changing all code in chrome from .<> to <>.
> 
> ".<>" is the form used by Files.app. If the team wants to switch fine by me,
but
> the team needs to make that decision.

I actually spoke with Hirono-san about this and he was fine with Foo<Bar>.

[Steve McKay, 2014-12-05 00:27:55]

On 2014/12/04 23:59:19, Ben Kwa wrote:
> On 2014/12/04 23:58:01, Steve McKay wrote:
> >
>
https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
> > File ui/file_manager/file_manager/background/js/media_scanner.js (right):
> > 
> >
>
https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
> > ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
> > {!Array<!Entry>} entries A list of file and directory entries.  File
> > On 2014/12/04 23:54:10, Dan Beam wrote:
> > > On 2014/12/04 23:47:59, Steve McKay wrote:
> > > > !Array.<!Entry>.
> > > 
> > > either syntax works and just <> is shorter.  any reason why you're asking
> for
> > > .<>?  I've been considering changing all code in chrome from .<> to <>.
> > 
> > ".<>" is the form used by Files.app. If the team wants to switch fine by me,
> but
> > the team needs to make that decision.
> 
> I actually spoke with Hirono-san about this and he was fine with Foo<Bar>.

Ping files-app to see what Tomasz thinks. If he doesn't object, I've got no
beef.

[Steve McKay, 2014-12-05 01:09:39]

A few more comments, most about how I'd like to see the design evolve.

But I'm happy with this submitting as is as I'm basically blocked on this
change.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:17: *
Progress center to submit the progressing item.
Comment doesn't seem to match the val.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:19: * @const
Use @const or don't for your "final" fields. Just be consistent.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:35: * @param
{!importer.MediaImportHandler.DestinationFactory=} opt_destination A
Why not just make opt_destination required, then let the foreground work out
(with the user) where to import. Then you can nuke all of the destination
related stuff.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:70:
this.source_ = source;
Missing jsdoc on the fields.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:78:
destination(),
I think this is all okay for the early prototype.

That said: The "Import" task is really just a copy task, no? We should do the
file scanning outside of the Task, then construct the Task once all of the input
is available. Then you've got a much simpler Task that is pretty much data only.
Besides, I think we'll want to do the scanning prior to the user initiating
import. Why?

1) We can show the user what they're about to do (Copy 123 files taking up nGB
of space)?
2) We can pre-determine if there is sufficient space on the local device to copy
the media.
3) We can show the user what is going on while we're scanning (think about a
banner with info on "Finding files to import.....found 16, 17, ....).

Basically it seems like a separate task unto it self.

Here's how I think the whole thing would go down.

1) Foreground discovers a device that is eligible for import.
2) Foreground asks background's import manager, hey, scan this thing.
3) Import manager returns a new ScanResult that is a live view on the scanning
process....it fires change events so the UI can be updated when results change.
4) Foreground shows the user what is happening. User is not confused, User is
rather happy.
5) Scan finishes.
6) User approves import, foreground asks import manager to kick off import,
giving it back the scan result.
7) Import manager returns an ImportResult object so that the Foreground can show
progress on the import task, and deal with errors, if any, as they arise.

There are corner cases along the way we'll need to deal with, like insufficient
storage. That'll need to happen in the Foreground too. So I think we need to
keep a pipeline open between foreground and background during all major
operations.


Thoughts?

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:121: var
rootName = 'root';
Why not just inline it, or put it in a CONSTANT?

[hirono, 2014-12-05 05:57:37]

lgtm with nits. Please also get lgtm from @smckay.

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/762593006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:88: * @type
{MediaImportHandler}
On 2014/12/04 19:53:01, Ben Kwa wrote:
> On 2014/12/04 04:44:25, hirono wrote:
> > @type {!MediaImporthandler}
> > @const
> 
> I fixed the nullable thing - thanks.
> For the const part, the convention I'm accustomed to is to mark something as
> const only if it's effectively immutable, which is not my intention (e.g. the
> import task queue is going to go in there).
> 
> Also, I took the liberty of going through the rest of the
FileBrowserBackground
> initialization code and cleaning/fixing up some of the other type annotations,
> PTAL.  Thanks!

Thank you for fixing them!

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:129: return
new Promise(function(resolve, reject) {
You can do "return drive.resolveDisplayRoot();"

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/compiled_resources.gyp (right):

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp:59:
'../../background/js/media_import_handler.js',
Please also add media_scanner.js

[Ben Kwa, 2014-12-05 17:22:50]

Done.  One small note: cleaning up the
importer.MediaImportHandler.defaultDestination.getDriveRoot_ function as hirono@
suggested, necessitated a change in the MediaImportHandler unittest.

The new getDriveRoot_ implementation does not work on top of the mock file
system that the test uses, so I just had to inject a fake import destination.

Thanks!

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (right):

https://codereview.chromium.org/762593006/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array<!Entry>} entries A list of file and directory entries.  File
On 2014/12/04 23:58:00, Steve McKay wrote:
> On 2014/12/04 23:54:10, Dan Beam wrote:
> > On 2014/12/04 23:47:59, Steve McKay wrote:
> > > !Array.<!Entry>.
> > 
> > either syntax works and just <> is shorter.  any reason why you're asking
for
> > .<>?  I've been considering changing all code in chrome from .<> to <>.
> 
> ".<>" is the form used by Files.app. If the team wants to switch fine by me,
but
> the team needs to make that decision.

This is being discussed offline, but looks for now like the team is leaning
towards the new syntax, so this stands for now.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:17: *
Progress center to submit the progressing item.
On 2014/12/05 01:09:39, Steve McKay wrote:
> Comment doesn't seem to match the val.

Doh, I completely missed updating this jsdoc.  Fixed.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:19: * @const
On 2014/12/05 01:09:39, Steve McKay wrote:
> Use @const or don't for your "final" fields. Just be consistent.

Done.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:35: * @param
{!importer.MediaImportHandler.DestinationFactory=} opt_destination A
On 2014/12/05 01:09:39, Steve McKay wrote:
> Why not just make opt_destination required, then let the foreground work out
> (with the user) where to import. Then you can nuke all of the destination
> related stuff.

I figured this was a good intermediate step that enabled us to get the overall
import process up and running.  Adding a bunch of UI to enable the user to
select a location sounds good, but I'd like to do it in a follow-up CL.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:70:
this.source_ = source;
On 2014/12/05 01:09:39, Steve McKay wrote:
> Missing jsdoc on the fields.

Done.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:78:
destination(),
I agree with the (eventual) separation between the scanning and copying tasks. 
That said, the ImportTask is a copy task, but it is not necessarily a
fileOperationUtil.CopyTask (if that's what you meant).  That detail is still
being worked out.

All the other stuff makes sense.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:121: var
rootName = 'root';
On 2014/12/05 01:09:39, Steve McKay wrote:
> Why not just inline it, or put it in a CONSTANT?

This was here to make it clear what had to change in order for us to switch to
unparented imports.  That's looking less and less likely, so I just switched to
the code that hirono@ suggested below.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:129: return
new Promise(function(resolve, reject) {
On 2014/12/05 05:57:37, hirono wrote:
> You can do "return drive.resolveDisplayRoot();"

Thanks!  Done.

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/compiled_resources.gyp (right):

https://codereview.chromium.org/762593006/diff/80001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp:59:
'../../background/js/media_import_handler.js',
On 2014/12/05 05:57:37, hirono wrote:
> Please also add media_scanner.js

Done.

[Steve McKay, 2014-12-05 19:01:17]

Perfectly fine prototype!
LGTM

[Ben Kwa, 2014-12-05 19:04:11]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2014-12-05 19:05:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/762593006/100001

[commit-bot: I haz the power, 2014-12-05 19:08:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-05 19:08:40]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/90554)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ben Kwa, 2014-12-05 19:56:14]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2014-12-05 19:57:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/762593006/120001

[commit-bot: I haz the power, 2014-12-05 20:53:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-05 20:54:27]

Patchset 7 (id:??) landed as
https://crrev.com/157ed95ab034b76ac8491b9f926051d6e9ac9055
Cr-Commit-Position: refs/heads/master@{#307076}