MediaScanner improvements

MediaScanner improvements.

Background: In a subsequent CL we'll want to plug the scanner into the progress center (as a "task" I believe) so the user can see that there is some activity going on. This is one reason why we don't just gather up all the files in a single swoop, but do some acrobatics to handle each file individually.

* Update MediaScanner to be non-reusable (1 instance per scan).
* Add separate MediaScanner and ScanResult interfaces.
* MediaScanner now tracks scan duration (which will be reported in a subsequent UMA).
* MediaScanner now gathers local storage requirements so that cloud import can report insufficient storage to user...and prohibit import.

BUG=420680
TEST=browser_test: FileManagerJsTest.*

Committed: https://crrev.com/41ec9ff4ad115e94ed3e9b2d96c42e5f6eb07f2c
Cr-Commit-Position: refs/heads/master@{#308045}

[Steve McKay, 2014-12-11 02:19:21]

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

[mtomasz, 2014-12-11 04:14:32]

https://codereview.chromium.org/795833002/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/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler_unittest.js:56:
return mediaScanner
nit: ; missing at the end

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:98:
'happy',
nit: 2 spaces indent

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:237:
assertTrue(scanner.inState(importer.ScanState.NOT_STARTED));
nit: I'm not convinced about this push model for checking values. I've never
seen this pattern in Chromium code. Why not simply a getter, which is widely
used in the entire code base?

Having a getter allows us to use the state vlaue also in switch/case, as well as
in ASSERT_EQ, DCHECK_EQ, DCHECK_NE (for C++). It also allows us to copy the
value, etc, etc.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:239:
'foo.jpg',
ditto. Please check also other places.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:242:
makeTestFilesystemRoot('testScanState')
nit: This may be not in this CL, but we use FileSystem naming in Chromium JS
rather than Filesystem, as the API is called File System API.
https://developer.mozilla.org/en-US/docs/WebGuide/API/File_System

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:275:
result.fullPath + ' is not a file');
nit: . at the end of logs if they form a sentence.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:277:
expected.indexOf(result.fullPath) != -1,
nit: !== and ===. Please check other places too.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/mock_media_scanner.js:15: /**
@private {!importer.ScanState} */
nit: If it's private, it should be state_. Same for other members.

[mtomasz, 2014-12-11 05:07:22]

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:20:
this.scannerFactory_ = scannerFactory;
nit: It's a method, how about renaming to createScanner? this.scannerFactory_()
is not as easy to understand as this.createScanner_().

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array.<!Entry>} entries Must be non-empty.
nit: Do we need this non-empty restriction? It looks to me unnecessary. If
empty, then we could just finish immediately with no results. As a result, we
don't need to test and worry about this edge case. WDYT?

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:48:
importer.ScanResult.prototype.getRequiredBytes;
nit: How about getTotalBytes? It's simpler. Just total size in bytes of all
scanned files.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:81:
this.scanStarted_ = 0;
nit: I had to look at line #95 to understand meaning of this member. It's type
is not DateTime, and the name doesn't say anything about time, so I didn't know
what's the purpose of it. I'd suggest either:
1. Rename to startTime/scanStartedTime or sth saying it's time.
2. Use Date as type.
3. Add a comment to the member.
Either is fine to me.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:104: }.bind(this));
It seems that this method doesn't return a promise synchronously. Does it work?

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:128: var
onfileFoundBound_ = this.onFileFound_.bind(this);
nit: onfile -> onFile

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:139:
promises.push(onfileFoundBound_(fileEntry));
nit: ditto

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:150: * Handles files
discovered during scanning.
nit: What does it return via promise? Event handlers usually never return
anything. Could you add a comment? Eg. "Returns a promise which settles once
handling the file is completed."

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:165:
entry.getMetadata(
We should use MetadataCache.get() to query for metadata and cache it in one go.
In general getMetadata() is expensive.

[mtomasz, 2014-12-11 05:08:44]

On 2014/12/11 05:07:22, mtomasz wrote:
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> File ui/file_manager/file_manager/background/js/media_import_handler.js
(right):
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_import_handler.js:20:
> this.scannerFactory_ = scannerFactory;
> nit: It's a method, how about renaming to createScanner?
this.scannerFactory_()
> is not as easy to understand as this.createScanner_().
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> File ui/file_manager/file_manager/background/js/media_scanner.js (right):
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
> {!Array.<!Entry>} entries Must be non-empty.
> nit: Do we need this non-empty restriction? It looks to me unnecessary. If
> empty, then we could just finish immediately with no results. As a result, we
> don't need to test and worry about this edge case. WDYT?
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:48:
> importer.ScanResult.prototype.getRequiredBytes;
> nit: How about getTotalBytes? It's simpler. Just total size in bytes of all
> scanned files.
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:81:
> this.scanStarted_ = 0;
> nit: I had to look at line #95 to understand meaning of this member. It's type
> is not DateTime, and the name doesn't say anything about time, so I didn't
know
> what's the purpose of it. I'd suggest either:
> 1. Rename to startTime/scanStartedTime or sth saying it's time.
> 2. Use Date as type.
> 3. Add a comment to the member.
> Either is fine to me.
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:104:
}.bind(this));
> It seems that this method doesn't return a promise synchronously. Does it
work?
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:128: var
> onfileFoundBound_ = this.onFileFound_.bind(this);
> nit: onfile -> onFile
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:139:
> promises.push(onfileFoundBound_(fileEntry));
> nit: ditto
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:150: * Handles
files
> discovered during scanning.
> nit: What does it return via promise? Event handlers usually never return
> anything. Could you add a comment? Eg. "Returns a promise which settles once
> handling the file is completed."
> 
>
https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
> ui/file_manager/file_manager/background/js/media_scanner.js:165:
> entry.getMetadata(
> We should use MetadataCache.get() to query for metadata and cache it in one
go.
> In general getMetadata() is expensive.

Mostly nits. lgtm, but please wait for Ben's one.

[Ben Kwa, 2014-12-11 16:20:12]

https://codereview.chromium.org/795833002/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/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler_unittest.js:84:
// Looks like we're using a "paste" operation to copy imported files.
Yes, but this is a temporary state of affairs.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array.<!Entry>} entries Must be non-empty.
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: Do we need this non-empty restriction? It looks to me unnecessary. If
> empty, then we could just finish immediately with no results. As a result, we
> don't need to test and worry about this edge case. WDYT?

+1.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:16: * @return
{!Promise.<!importer.ScanResult>} Resolves when scanning is complete.
nit: Type annotations - 
!Array<!Entry>
!Promise<!importer.ScanResult>

and so on throughout the CL.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:133: /** @type
{!DirectoryEntry} */ (entry),
I don't think this typecast is necessary.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:136: * @this
{importer.DefaultMediaScanner}
Remove @this, since the function isn't bound and doesn't reference 'this'.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:142: /** @this
{importer.DefaultMediaScanner} */
Remove @this, since the function isn't bound and doesn't reference 'this'.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:202:
importer.DefaultMediaScanner.prototype.getRequiredBytes = function() {
It's not really valid to call getRequiredBytes and getScanDurationMs until the
scan is complete either.  I find it a little odd to have DefaultMediaScanner
implement both MediaScanner and ScanResult.  If we separated them, then the
getFileEntries, getRequiredBytes and getScanDurationMs interfaces wouldn't be
available by definition until the scan completes, and we wouldn't need all this
checking and throwing logic.  Let's talk offline.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:87: try {
Use assertThrows.  But for the record I agree with Tomasz's point about empty
scans - I think they should just return an empty result.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:271:
function(result) {
Using this check, ['a', 'b', 'b', 'c'] and ['a', 'b', 'c'] would match.

I'd suggest either
a) sorting and doing exact index comparison or
b) checking for duplicates within each list

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/mock_media_scanner.js:16: this.state
= importer.ScanState.NOT_STARTED;
I think you meant to make this public (I see it being set in the test).  But,
coupled with the other point about using getters, I think what you want to do is

a) leave this private; rename accordingly
b) implement an explicit set method
c) add a getter.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:107: * @private
{importer.MediaImportHandler}
This change should already be in.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager_commands.js
(right):

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1046:
fileManager.getCurrentDirectoryEntry()));
The jsdoc for this states that it can return null.  Rather than typecast, I
think we should take corrective action (e.g. for now, just console.error and not
call importer.importMedia).

[Steve McKay, 2014-12-11 20:14:17]

Refactor media scanner to separate scanning code from results code, eliminating
the need for a factory function. Eliminate scanning state, use ScanResult as a
proxy for this for now.

[Steve McKay, 2014-12-11 20:19:05]

Uploaded a snapshot of changes discussed with Ben offline.
I haven't addressed all comments, yet, but am posting this for Ben to look at
while I'm off to my son's Christmas concert. I'll be back around 3.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler.js:20:
this.scannerFactory_ = scannerFactory;
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: It's a method, how about renaming to createScanner?
this.scannerFactory_()
> is not as easy to understand as this.createScanner_().

Better. Done.

https://codereview.chromium.org/795833002/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/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler_unittest.js:56:
return mediaScanner
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: ; missing at the end

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_import_handler_unittest.js:84:
// Looks like we're using a "paste" operation to copy imported files.
On 2014/12/11 16:20:11, Ben Kwa wrote:
> Yes, but this is a temporary state of affairs.

No worries. I'm not complaining. I was just confused at first, so I added a
little note. We can nuke it when we change the impl.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:15: * @param
{!Array.<!Entry>} entries Must be non-empty.
On 2014/12/11 16:20:11, Ben Kwa wrote:
> On 2014/12/11 05:07:21, mtomasz wrote:
> > nit: Do we need this non-empty restriction? It looks to me unnecessary. If
> > empty, then we could just finish immediately with no results. As a result,
we
> > don't need to test and worry about this edge case. WDYT?
> 
> +1.

This one I'm going to push back on. A scan should *never* be initiated with no
input. By making this class *strict* on this case, we make ongoing enforcement
of this rule super easy.

Looking at it another way.

We've all been bitten at one point or another by bugs emerging from code not
being strict with what it accepts. We might not have even written the code, but
it still affects us.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:128: var
onfileFoundBound_ = this.onFileFound_.bind(this);
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: onfile -> onFile

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:136: * @this
{importer.DefaultMediaScanner}
On 2014/12/11 16:20:11, Ben Kwa wrote:
> Remove @this, since the function isn't bound and doesn't reference 'this'.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:139:
promises.push(onfileFoundBound_(fileEntry));
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: ditto

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:142: /** @this
{importer.DefaultMediaScanner} */
On 2014/12/11 16:20:11, Ben Kwa wrote:
> Remove @this, since the function isn't bound and doesn't reference 'this'.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:202:
importer.DefaultMediaScanner.prototype.getRequiredBytes = function() {
On 2014/12/11 16:20:11, Ben Kwa wrote:
> It's not really valid to call getRequiredBytes and getScanDurationMs until the
> scan is complete either.  I find it a little odd to have DefaultMediaScanner
> implement both MediaScanner and ScanResult.  If we separated them, then the
> getFileEntries, getRequiredBytes and getScanDurationMs interfaces wouldn't be
> available by definition until the scan completes, and we wouldn't need all
this
> checking and throwing logic.  Let's talk offline. 

Per offline convo. DefaultScanResult is not a separate, but still "live",
object.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/mock_media_scanner.js:15: /**
@private {!importer.ScanState} */
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: If it's private, it should be state_. Same for other members.

Obsolete.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/mock_media_scanner.js:16: this.state
= importer.ScanState.NOT_STARTED;
On 2014/12/11 16:20:12, Ben Kwa wrote:
> I think you meant to make this public (I see it being set in the test).  But,
> coupled with the other point about using getters, I think what you want to do
is
> 
> a) leave this private; rename accordingly
> b) implement an explicit set method
> c) add a getter.

Obsolete.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:107: * @private
{importer.MediaImportHandler}
On 2014/12/11 16:20:12, Ben Kwa wrote:
> This change should already be in.

Acknowledged.

[Steve McKay, 2014-12-11 21:19:33]

Respond to review comments.

[Ben Kwa, 2014-12-11 23:12:16]

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

https://codereview.chromium.org/795833002/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:26: * @type
{?importer.ScanResult}
? is redundant here.  Declare it as {importer.ScanResult} and initialize it to
null.

[Ben Kwa, 2014-12-11 23:18:11]

lgtm

[Steve McKay, 2014-12-12 00:43:24]

Retrying publish drafts...Reitveld has been broken.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:16: * @return
{!Promise.<!importer.ScanResult>} Resolves when scanning is complete.
On 2014/12/11 16:20:11, Ben Kwa wrote:
> nit: Type annotations - 
> !Array<!Entry>
> !Promise<!importer.ScanResult>
> 
> and so on throughout the CL.

I'll let fukino make the change in one feld-swoop. Once he's done that, we can
treat dropping the dot-delimeter as a requirement.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:48:
importer.ScanResult.prototype.getRequiredBytes;
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: How about getTotalBytes? It's simpler. Just total size in bytes of all
> scanned files.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:81:
this.scanStarted_ = 0;
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: I had to look at line #95 to understand meaning of this member. It's type
> is not DateTime, and the name doesn't say anything about time, so I didn't
know
> what's the purpose of it. I'd suggest either:
> 1. Rename to startTime/scanStartedTime or sth saying it's time.
> 2. Use Date as type.
> 3. Add a comment to the member.
> Either is fine to me.

Done. Changed Type, updated comment.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:104: }.bind(this));
On 2014/12/11 05:07:21, mtomasz wrote:
> It seems that this method doesn't return a promise synchronously. Does it
work?

Fixed. Returns a ScanResult now.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:133: /** @type
{!DirectoryEntry} */ (entry),
On 2014/12/11 16:20:11, Ben Kwa wrote:
> I don't think this typecast is necessary.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:150: * Handles files
discovered during scanning.
On 2014/12/11 05:07:21, mtomasz wrote:
> nit: What does it return via promise? Event handlers usually never return
> anything. Could you add a comment? Eg. "Returns a promise which settles once
> handling the file is completed."

Updated comment to 'splain.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner.js:165:
entry.getMetadata(
On 2014/12/11 05:07:21, mtomasz wrote:
> We should use MetadataCache.get() to query for metadata and cache it in one
go.
> In general getMetadata() is expensive.

Looks like MetadataCache is a foreground object, MediaScanner is a background
object. So I don't have an instance at the time the MediaImportHandler is
instantiated.

I'm happy to work it out to make this happen, but would like to do so in a
followup CL.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager_commands.js
(right):

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1046:
fileManager.getCurrentDirectoryEntry()));
On 2014/12/11 16:20:12, Ben Kwa wrote:
> The jsdoc for this states that it can return null.  Rather than typecast, I
> think we should take corrective action (e.g. for now, just console.error and
not
> call importer.importMedia). 

We've already validated that the current directory is eligible for import...in
the canExecute method.

[mtomasz, 2014-12-12 01:00:48]

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

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:28:
this.activeScanResult_;
nit: This is undefined, not null. Can we set to null?

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

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:130: * Recursively
scans through a list of given files and directories, and creates
The description seems out of date. The result class doesn't scan.

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:205:
entry.getMetadata(
nit: Please add TODO to migrate it to the new metadata cache once ready, so we
don't forget.

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

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:98:
'happy',
nit: 2 spaces indent.

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:113:
callback);
nit: Indent looks off.

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:145:
callback);
nit: Same here? 7 spaces, should be 8.

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

Respond to review comments.

[Steve McKay, 2014-12-12 01:17:30]

Dones. Thanks!

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

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_import_handler.js:28:
this.activeScanResult_;
On 2014/12/12 01:00:47, mtomasz wrote:
> nit: This is undefined, not null. Can we set to null?

Done.

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

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:130: * Recursively
scans through a list of given files and directories, and creates
On 2014/12/12 01:00:47, mtomasz wrote:
> The description seems out of date. The result class doesn't scan.

Done.

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner.js:205:
entry.getMetadata(
On 2014/12/12 01:00:47, mtomasz wrote:
> nit: Please add TODO to migrate it to the new metadata cache once ready, so we
> don't forget.

Done. Also added an entry to the backlog.

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

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:98:
'happy',
On 2014/12/12 01:00:48, mtomasz wrote:
> nit: 2 spaces indent.

Done.

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:113:
callback);
On 2014/12/12 01:00:47, mtomasz wrote:
> nit: Indent looks off.

Done.

https://codereview.chromium.org/795833002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:145:
callback);
On 2014/12/12 01:00:48, mtomasz wrote:
> nit: Same here? 7 spaces, should be 8.

Done. But 6 spaces. Matching line 134.

[mtomasz, 2014-12-12 01:22:08]

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:275:
result.fullPath + ' is not a file');
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: . at the end of logs if they form a sentence.

Ping.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:277:
expected.indexOf(result.fullPath) != -1,
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: !== and ===. Please check other places too.

Ping.

[mtomasz, 2014-12-12 01:23:01]

lgtm, but please double check if all comments are addressed.

[Steve McKay, 2014-12-12 02:04:36]

Respond to review comments.

[Steve McKay, 2014-12-12 02:06:04]

Addressed comments I'd missed in a previous round. G'derr.

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

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:87: try {
On 2014/12/11 16:20:11, Ben Kwa wrote:
> Use assertThrows.  But for the record I agree with Tomasz's point about empty
> scans - I think they should just return an empty result.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:98:
'happy',
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: 2 spaces indent

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:237:
assertTrue(scanner.inState(importer.ScanState.NOT_STARTED));
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: I'm not convinced about this push model for checking values. I've never
> seen this pattern in Chromium code. Why not simply a getter, which is widely
> used in the entire code base?
> 
> Having a getter allows us to use the state vlaue also in switch/case, as well
as
> in ASSERT_EQ, DCHECK_EQ, DCHECK_NE (for C++). It also allows us to copy the
> value, etc, etc.

Nuked.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:239:
'foo.jpg',
On 2014/12/11 04:14:32, mtomasz wrote:
> ditto. Please check also other places.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:242:
makeTestFilesystemRoot('testScanState')
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: This may be not in this CL, but we use FileSystem naming in Chromium JS
> rather than Filesystem, as the API is called File System API.
> https://developer.mozilla.org/en-US/docs/WebGuide/API/File_System

Fixed.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:271:
function(result) {
On 2014/12/11 16:20:11, Ben Kwa wrote:
> Using this check, ['a', 'b', 'b', 'c'] and ['a', 'b', 'c'] would match.
> 
> I'd suggest either
> a) sorting and doing exact index comparison or
> b) checking for duplicates within each list

Pull MediaImportHandlerTest > assertEntryListEquals out to common location as
assertFileEntryListEquals. It checks that lists of paths are identical and that
all entries are files.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:275:
result.fullPath + ' is not a file');
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: . at the end of logs if they form a sentence.

Done.

https://codereview.chromium.org/795833002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/media_scanner_unittest.js:277:
expected.indexOf(result.fullPath) != -1,
On 2014/12/11 04:14:32, mtomasz wrote:
> nit: !== and ===. Please check other places too.

Done.

[Steve McKay, 2014-12-12 02:22:02]

pull 'n merge.

[Steve McKay, 2014-12-12 02:24:13]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:24:47]

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

[commit-bot: I haz the power, 2014-12-12 03:31:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-12 03:33:04]

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