Move content deduplication into the scan process.

Move content deduplication into the scan process.

Update GA events to record timing on content hashing and searching only when it exceeds a threshold.
Report MEGABYTES_IMPORTED when successfully completing import.

BUG=420680
// corrected bug.
TEST=browser_test: FileManagerJsTest.*

Committed: https://crrev.com/f57f39ca1ad4a2f03a876b776e437dc27452c8ca
Cr-Commit-Position: refs/heads/master@{#319336}

[Steve McKay, 2015-03-04 23:04:11]

Self review

[Steve McKay, 2015-03-05 00:43:57]

Patchset #2 (id:20001) has been deleted

[Steve McKay, 2015-03-05 00:44:13]

Patchset #1 (id:1) has been deleted

[Steve McKay, 2015-03-05 00:45:44]

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

[Steve McKay, 2015-03-05 00:45:45]

mtomasz@ for the background changes.

https://codereview.chromium.org/980603003/diff/40001/ui/file_manager/file_man...
File ui/file_manager/file_manager/background/js/media_scanner.js (left):

https://codereview.chromium.org/980603003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:40:
importer.ScanResult = function() {};
This just moved down next to the implementation.

https://codereview.chromium.org/980603003/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:77: *  
duplicateFileCount: number,
Only material change is that this changed to an object of dupe counts.

[mtomasz, 2015-03-05 04:36:53]

lgtm with nits

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:62: if
(elapsedTime >= 5000) {
nit: Can we move to a constant?

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:131: if
(elapsedTime >= 1000) {
nit: Constant?

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:174: throw new
Error('Unsupported destination: ' + destination);
nit: assert or return Promise.reject('Unsupported...')?

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:181: .then(
nit: indent

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:241: *    
!Promise<importer.Disposition>}
nit: Shall it be !importer.Disposition?

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:144:
testHistory.importedPaths[
nit: Line wrappings look off.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:37:
importer.MediaImportHandler =
nit: Will it fit in one line? If not, then we break usually at function(\n.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:449:
this.sendImportStats_(true);
nit: enum or /* arg_name */ for this true? It's impossible to guess what it
stands for.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:299: * @return
{!Array.<!FileEntry>}
nit: We recently switched to skipping the dot. Array. -> Array<, Promise.< ->
Promise<.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:34: // This
is the default disposition checker.
nit: This could really fit in two lines and be easier to read.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:175: 
nit: remove.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:21: CONTENT_DUPLICATE:
'content-dupe',
nit: It's hard for me to understand this enum. What's SCAN_DUPLICATE? Isn't
history duplicate detected during scan?

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:23: ORIGINAL:
'original',
nit: What does it mean that disposition is original? How about renaming to
simply NEW?

[Ben Kwa, 2015-03-05 20:05:10]

LGTM with a few nits.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:183:
function(duplicate) {
nit: param needs jsdoc

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:189:
function(duplicate) {
nit: param needs jsdoc

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:228:
function(results) {
nit: This function doesn't need a @this or a bind.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:121: //
Verifies the correct result when a duplicate doesn't exist.
"when a content duplicate exists."

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:138: //
Verifies the correct result when a duplicate doesn't exist.
"when a history duplicate exists."

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:455: * @param
{boolean} completedSuccessfully True if task
suggestion: You could reuse the TaskQueue.UpdateType instead of a boolean.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:466: // We
just don't have the information needed to do this accurately.
The processedBytes_ counter should be a fairly accurate count of number of bytes
imported for cancelled tasks.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:468:
this.tracker_.send(
We also need to do this.tracker_.send(metrics.ImportEvents.ENDED);

[Steve McKay, 2015-03-05 21:38:47]

Respond to review comments.

[Steve McKay, 2015-03-05 21:38:51]

The CQ bit was checked by smckay@chromium.org

[Steve McKay, 2015-03-05 21:38:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, kenobi@chromium.org
Link to the patchset: https://codereview.chromium.org/980603003/#ps80001 (title:
"Respond to review comments.")

[Steve McKay, 2015-03-05 21:39:04]

Dones. Submitting.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:62: if
(elapsedTime >= 5000) {
On 2015/03/05 04:36:52, mtomasz wrote:
> nit: Can we move to a constant?

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:131: if
(elapsedTime >= 1000) {
On 2015/03/05 04:36:52, mtomasz wrote:
> nit: Constant?

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:174: throw new
Error('Unsupported destination: ' + destination);
On 2015/03/05 04:36:52, mtomasz wrote:
> nit: assert or return Promise.reject('Unsupported...')?

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:181: .then(
On 2015/03/05 04:36:52, mtomasz wrote:
> nit: indent

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:183:
function(duplicate) {
On 2015/03/05 20:05:10, Ben Kwa wrote:
> nit: param needs jsdoc

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:189:
function(duplicate) {
On 2015/03/05 20:05:10, Ben Kwa wrote:
> nit: param needs jsdoc

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:228:
function(results) {
On 2015/03/05 20:05:10, Ben Kwa wrote:
> nit: This function doesn't need a @this or a bind.

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder.js:241: *    
!Promise<importer.Disposition>}
On 2015/03/05 04:36:52, mtomasz wrote:
> nit: Shall it be !importer.Disposition?

Done.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:121: //
Verifies the correct result when a duplicate doesn't exist.
On 2015/03/05 20:05:10, Ben Kwa wrote:
> "when a content duplicate exists."

Removed line entirely.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:138: //
Verifies the correct result when a duplicate doesn't exist.
On 2015/03/05 20:05:10, Ben Kwa wrote:
> "when a history duplicate exists."

Ditto.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/duplicate_finder_unittest.js:144:
testHistory.importedPaths[
On 2015/03/05 04:36:52, mtomasz wrote:
> nit: Line wrappings look off.

Done.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:37:
importer.MediaImportHandler =
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: Will it fit in one line? If not, then we break usually at function(\n.

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:449:
this.sendImportStats_(true);
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: enum or /* arg_name */ for this true? It's impossible to guess what it
> stands for.

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:455: * @param
{boolean} completedSuccessfully True if task
On 2015/03/05 20:05:10, Ben Kwa wrote:
> suggestion: You could reuse the TaskQueue.UpdateType instead of a boolean.

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:466: // We
just don't have the information needed to do this accurately.
On 2015/03/05 20:05:10, Ben Kwa wrote:
> The processedBytes_ counter should be a fairly accurate count of number of
bytes
> imported for cancelled tasks.

Perfect!

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:468:
this.tracker_.send(
On 2015/03/05 20:05:10, Ben Kwa wrote:
> We also need to do this.tracker_.send(metrics.ImportEvents.ENDED);

Removed that as there are plenty of surrogate events.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:299: * @return
{!Array.<!FileEntry>}
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: We recently switched to skipping the dot. Array. -> Array<, Promise.< ->
> Promise<.

Done.

FYI, this was just moved from another spot in the file. Sucks that this code
review doesn't have move detection.

Maybe someone should sweep through the codebase...remove these en masse.

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

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:34: // This
is the default disposition checker.
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: This could really fit in two lines and be easier to read.

Acknowledged.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:175: 
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: remove.

Done.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:21: CONTENT_DUPLICATE:
'content-dupe',
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: It's hard for me to understand this enum. What's SCAN_DUPLICATE? Isn't
> history duplicate detected during scan?

Scan duplicates are for duplicates detected only within a single scan result.

https://codereview.chromium.org/980603003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:23: ORIGINAL:
'original',
On 2015/03/05 04:36:53, mtomasz wrote:
> nit: What does it mean that disposition is original? How about renaming to
> simply NEW?

New doesn't seem much more explanatory.

[commit-bot: I haz the power, 2015-03-05 21:39:13]

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

[commit-bot: I haz the power, 2015-03-05 22:05:11]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-03-05 22:06:10]

Patchset 3 (id:??) landed as
https://crrev.com/f57f39ca1ad4a2f03a876b776e437dc27452c8ca
Cr-Commit-Position: refs/heads/master@{#319336}