Hook up import history to media scanner.

Hook up import history to media scanner.

Dedupe files while scanning against local history.
Hook up import history to media scanner in background.

BUG=420680
TEST=browser_test: FileManagerJsTest.*

jwd@ for historgrams.xml.

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

[Steve McKay, 2014-12-13 00:26:56]

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

[Steve McKay, 2014-12-13 00:28:20]

Add import history test double.

[Steve McKay, 2014-12-13 00:45:44]

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

https://codereview.chromium.org/805533003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:92:
this.historyLoaderPromise.then(
Tomasz. I just checked and this triggers the loading of history...even if a scan
never happens.

The fix is easy, inject the historyLoader_ instead of a Promise<ImportHistory>.
I'll upload it on Monday.

[Steve McKay, 2014-12-13 03:44:26]

Hide resolution of HistoryLoader promise behind a facade class.

[Steve McKay, 2014-12-13 03:46:18]

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

https://codereview.chromium.org/805533003/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:92:
this.historyLoaderPromise.then(
On 2014/12/13 00:45:44, Steve McKay wrote:
> Tomasz. I just checked and this triggers the loading of history...even if a
scan
> never happens.
> 
> The fix is easy, inject the historyLoader_ instead of a
Promise<ImportHistory>.
> I'll upload it on Monday.

K. This is fixed now.

[Steve McKay, 2014-12-13 16:18:06]

Don't create a new history loader instance for each access.

[mtomasz, 2014-12-15 00:51:20]

lgtm with nits.

https://codereview.chromium.org/805533003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/805533003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:9146: +    Chrome OS File Browser /
Cloud Import - The collective size, in megabytes,
nit: File Browser is a deprecated name. Please use Files app in descriptions.
But as for the histogram name it should stay FileBrowser.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:593: this.history_;
nit: This is undefined, but jsdoc allows ImportHistory or null.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:7: * @type
{!Object}
nit: How about @type {!Object.<!string, !Function>}. Is it correct here?

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:150: *
Verifies that scanning a simple single-level directory structure works.
nit: This unit tests rather tests if scanning ignores previously imported. Could
you update the comment?

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:152:
function testIgnoresPreviouslyImports(callback) {
nit: PreviouslyImports -> PreviousImports ?

[Ben Kwa, 2014-12-15 15:57:24]

lgtm

Couple of minor jsdoc related comments, otherwise, LG.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:613: this.history_ =
history;
I'm not sure how this passed type-check (perhaps the promise error filter is
overly permissive) but the type that's being assigned here is actually
Promise<!importer.ImportHistory>, which doesn't match with the type of
this.history_.

Putting this assignment in a chained .then block. would fix this.  I think just
changing the type of this.history_ to {Promise<!importer.ImportHistory} would
also work.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:237:
function(history) {
The history param needs a jsdoc.

[Steve McKay, 2014-12-15 19:30:33]

Revert metrics integration (since additional metics.js changes would be required
to make metrics work in background).

[Steve McKay, 2014-12-15 19:34:39]

Revert manifest.json.

[Steve McKay, 2014-12-15 19:35:03]

smckay@chromium.org changed reviewers:
- jwd@chromium.org

[Steve McKay, 2014-12-15 19:35:38]

Dropped Jesse Doherty from the CL as metrics changes need to be dealt with in a
separate CL.

https://codereview.chromium.org/805533003/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/805533003/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:9146: +    Chrome OS File Browser /
Cloud Import - The collective size, in megabytes,
On 2014/12/15 00:51:20, mtomasz wrote:
> nit: File Browser is a deprecated name. Please use Files app in descriptions.
> But as for the histogram name it should stay FileBrowser.

Done.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:593: this.history_;
On 2014/12/15 00:51:20, mtomasz wrote:
> nit: This is undefined, but jsdoc allows ImportHistory or null.

Done.

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/background.js:613: this.history_ =
history;
On 2014/12/15 15:57:24, Ben Kwa wrote:
> I'm not sure how this passed type-check (perhaps the promise error filter is
> overly permissive) but the type that's being assigned here is actually
> Promise<!importer.ImportHistory>, which doesn't match with the type of
> this.history_.
> 
> Putting this assignment in a chained .then block. would fix this.  I think
just
> changing the type of this.history_ to {Promise<!importer.ImportHistory} would
> also work.

Good eye!

Compiler didn't catch that because the it doesn't check anything inside a bound
anonymous function (which is very saddnessy). I pulled some of the meat out to a
proper method to increase compiler coverage.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:237:
function(history) {
On 2014/12/15 15:57:24, Ben Kwa wrote:
> The history param needs a jsdoc.

Done.

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

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:7: * @type
{!Object}
On 2014/12/15 00:51:20, mtomasz wrote:
> nit: How about @type {!Object.<!string, !Function>}. Is it correct here?

Done.

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:150: *
Verifies that scanning a simple single-level directory structure works.
On 2014/12/15 00:51:20, mtomasz wrote:
> nit: This unit tests rather tests if scanning ignores previously imported.
Could
> you update the comment? 

Done.

https://codereview.chromium.org/805533003/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:152:
function testIgnoresPreviouslyImports(callback) {
On 2014/12/15 00:51:20, mtomasz wrote:
> nit: PreviouslyImports -> PreviousImports ?

Done.

[Steve McKay, 2014-12-15 20:45:49]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2014-12-15 20:47:08]

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

[commit-bot: I haz the power, 2014-12-15 20:52:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-15 20:52:52]

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