Files.app: Add a deduplication step to avoid importing duplicate media.

Files.app: Add a deduplication step to avoid importing duplicate media.

Add a DuplicateFinder and introduce it into the cloud import pipeline.  Note: Deduplication takes place in-place on the source files, which could be slow for MTP devices.  Profiling will be necessary to test this.  The duplicate check returns a simple yes/no answer.  Non-duplicate media is then imported as before.

Destination is now promoted to a top-level concept.  MediaImportHandler has a new function which enables clients to register new import destinations.  Each destination requires a DestinationFactory and an optional DuplicateFinder.  The import destination is selected at import time by its ID.

The Observer pattern on the ImportTask class was tweaked to support auxiliary data being passed with each observer call.  This is to allow richer updates (e.g. an entry-changed update that informs observers when a file was copied, and sends along the source and new destination).

A new importer.TestLogger had to be created and inserted into some tests in order to circumvent the running of the regular RunTimeLogger, which uses functionality that doesn't exist at test time (i.e. chrome.syncFileSystem.onFileStatusChanged)

MockEntry et al. were  augmented to do more "realistic" moves and copies, needed to test the new two-stage import.

Unit tests were cleaned up.

BUG=420680
TEST=browser_test: FileManagerJsTest.MediaImportHandlerTest

Committed: https://crrev.com/1bdfe457d14248e3c23423feb470eafbcf984743
Cr-Commit-Position: refs/heads/master@{#313503}

[Ben Kwa, 2015-01-26 19:10:12]

kenobi@chromium.org changed reviewers:
+ mtomasz@chromium.org, smckay@chromium.org

[Ben Kwa, 2015-01-26 19:10:12]

After further discussion with smckay@, it was decided to skip the initial
tmp-copy step.  The dedupe will now take place in-place on the mounted media
device.

[mtomasz, 2015-01-26 20:14:50]

lgtm with nits

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:24: * An
duplicate finder that always reports no duplicates.
nit: An -> A

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:57:
this.returnValue = false;
nit: jsdoc missing

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:81:
'MediaImportHandler: Destination ' + id + 'already exists.');
nit: 'a -> ' a

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:93:
'MediaImportHandler: Unknown destination ' + destination + ' for import');
nit: t') -> t.')

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_import_handler_unittest.js
(right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler_unittest.js:63:
// Set up destination
nit: n -> n.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:581: /** @type
{importer.Logger} */
nit: @private?

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:602:
importer.setupTestLogger = function() {
optional nit: Testing code should not be compiled into production package if
possible.

[Steve McKay, 2015-01-26 20:26:26]

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:21: *
@typedef {{
There's a bit of sprawl here. You've got the new DuplicateFinder interface, a
DestinationFactory typedef, a DestinationHelpers typedef and the existing
importer.Destination enum.

This all screams out to me for a class to pull it all together.

Since we already have an importer.Destination enum, how about calling the whole
thing "importer.Repository" or even maybe "importer.CloudStorage".

Then DefaultImportHandler can just take a list of these classes in its
constructor....and since for now, we only support one destination, keep it
simple...just pass a single instance...no list. We can make it a list (and index
it) once we add support for FSPs.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:335: // If
the given file is a duplicate, don't import it again.  Just
Please add a TODO here to update import history. We'll want to mark the dupe as
synced.

[Ben Kwa, 2015-01-27 19:59:42]

Patchset #2 (id:20001) has been deleted

[Ben Kwa, 2015-01-27 20:00:32]

Done.  The destination stuff was much simplified.  PTAL.  Thanks!

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/duplicate_finder.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:24: * An
duplicate finder that always reports no duplicates.
On 2015/01/26 20:14:49, mtomasz wrote:
> nit: An -> A

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/duplicate_finder.js:57:
this.returnValue = false;
On 2015/01/26 20:14:49, mtomasz wrote:
> nit: jsdoc missing

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_import_handler.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:21: *
@typedef {{
On 2015/01/26 20:26:25, Steve McKay wrote:
> There's a bit of sprawl here. You've got the new DuplicateFinder interface, a
> DestinationFactory typedef, a DestinationHelpers typedef and the existing
> importer.Destination enum.
> 
> This all screams out to me for a class to pull it all together.
> 
> Since we already have an importer.Destination enum, how about calling the
whole
> thing "importer.Repository" or even maybe "importer.CloudStorage".
> 
> Then DefaultImportHandler can just take a list of these classes in its
> constructor....and since for now, we only support one destination, keep it
> simple...just pass a single instance...no list. We can make it a list (and
index
> it) once we add support for FSPs.

As discussed offline, we're going the simple route for now:
- all the extra classes and registration were removed.
- the DuplicateFinder is DI'd into the MediaImportHandler.  We just default to
using the DriveDuplicateFinder for now.
- we will extend the framework to handle third-party destinations and
duplicate-finders later.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:81:
'MediaImportHandler: Destination ' + id + 'already exists.');
On 2015/01/26 20:14:49, mtomasz wrote:
> nit: 'a -> ' a

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:93:
'MediaImportHandler: Unknown destination ' + destination + ' for import');
On 2015/01/26 20:14:49, mtomasz wrote:
> nit: t') -> t.')

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:335: // If
the given file is a duplicate, don't import it again.  Just
On 2015/01/26 20:26:25, Steve McKay wrote:
> Please add a TODO here to update import history. We'll want to mark the dupe
as
> synced.

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/media_import_handler_unittest.js
(right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler_unittest.js:63:
// Set up destination
On 2015/01/26 20:14:49, mtomasz wrote:
> nit: n -> n.

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:581: /** @type
{importer.Logger} */
On 2015/01/26 20:14:49, mtomasz wrote:
> nit: @private?

Done.

https://codereview.chromium.org/881463003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/importer_common.js:602:
importer.setupTestLogger = function() {
On 2015/01/26 20:14:49, mtomasz wrote:
> optional nit: Testing code should not be compiled into production package if
> possible.

Moved into a separate file.

[Steve McKay, 2015-01-27 20:27:51]

LGTM w/ nits.

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

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:40: */
Missing dupeFinder @param.

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:42:
function(progressCenter, historyLoader, opt_duplicateFinder) {
Make dupeFinder required. It's easy enough to just inject the instance.

In general, just say no to opt args...they increase the number of possible
realities...which is bad.

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:379:
resolver.resolve(destinationEntry);
copy_ is typed to return !Promise, untyped.

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:103: result,
importer.Destination.GOOGLE_DRIVE);
The second arg isn't a Destination, it is basically a directory factory.

[Ben Kwa, 2015-01-28 00:43:28]

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

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:40: */
On 2015/01/27 20:27:51, Steve McKay wrote:
> Missing dupeFinder @param.

Done.

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:42:
function(progressCenter, historyLoader, opt_duplicateFinder) {
On 2015/01/27 20:27:51, Steve McKay wrote:
> Make dupeFinder required. It's easy enough to just inject the instance.
> 
> In general, just say no to opt args...they increase the number of possible
> realities...which is bad.

Done.

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:379:
resolver.resolve(destinationEntry);
On 2015/01/27 20:27:51, Steve McKay wrote:
> copy_ is typed to return !Promise, untyped.

Done.

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/import_controller.js (right):

https://codereview.chromium.org/881463003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/import_controller.js:103: result,
importer.Destination.GOOGLE_DRIVE);
On 2015/01/27 20:27:51, Steve McKay wrote:
> The second arg isn't a Destination, it is basically a directory factory.

Done.

[Ben Kwa, 2015-01-28 00:43:51]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2015-01-28 00:44:56]

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

[commit-bot: I haz the power, 2015-01-28 01:51:58]

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

[commit-bot: I haz the power, 2015-01-28 01:52:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Kwa, 2015-01-28 14:13:06]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2015-01-28 14:14:10]

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

[commit-bot: I haz the power, 2015-01-28 14:18:15]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-01-28 14:19:03]

Patchset 4 (id:??) landed as
https://crrev.com/1bdfe457d14248e3c23423feb470eafbcf984743
Cr-Commit-Position: refs/heads/master@{#313503}