Include MTP devices in Cloud Import happiness.

Include MTP devices in Cloud Import happiness.
Nuke "VolumeManager.isType".
Add test coverage of importer_common methods.
Make all "DCIM" dir-name matching case-insensitive.

BUG=420680
TEST=browser_test: FileManagerJsTest.*

Committed: https://crrev.com/15d1aae4da7783607df3fac2d38198e817cf6fa0
Cr-Commit-Position: refs/heads/master@{#308414}

[Steve McKay, 2014-12-05 22:32:31]

Nuke VolumeInfo.isType method.

[Steve McKay, 2014-12-06 00:37:47]

Add tests for importer common code.

[Steve McKay, 2014-12-06 00:38:37]

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

[mtomasz, 2014-12-08 00:41:50]

lgtm with nits

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

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:34:
entry.fullPath.toUpperCase().indexOf('/DCIM/') === 0;
AFAIR existence of a trailing slash is not well defined, and may vary. We had
problems with this in the future. Can we make sure it works with and without the
trailing slash?

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:45:
importer.ELIGIBLE_VOLUME_TYPES_.indexOf(volumeInfo.volumeType) != -1;
nit: !==

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:81:
console.assert(volumeInfoProvider != null);
nit: !==

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

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp:45:
'../../common/js/volume_manager_common.js',
What's the reason for this change? AFAIK this list should be sorted.

[Steve McKay, 2014-12-08 17:17:25]

Respond to review comments.

[Steve McKay, 2014-12-08 17:17:40]

Thanks!

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

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:34:
entry.fullPath.toUpperCase().indexOf('/DCIM/') === 0;
On 2014/12/08 00:41:50, mtomasz wrote:
> AFAIR existence of a trailing slash is not well defined, and may vary. We had
> problems with this in the future. Can we make sure it works with and without
the
> trailing slash?

isMediaEntry is only applied to files, so there has to be a separator between
DCIM and the file name. But the thought does apply to isMediaDirectory. I
updated that to check both /DCIM and /DCIM/.

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:45:
importer.ELIGIBLE_VOLUME_TYPES_.indexOf(volumeInfo.volumeType) != -1;
On 2014/12/08 00:41:50, mtomasz wrote:
> nit: !==

Darn. Sorry about that. I'm trying :)

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/importer_common.js:81:
console.assert(volumeInfoProvider != null);
On 2014/12/08 00:41:50, mtomasz wrote:
> nit: !==

Done.

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

https://codereview.chromium.org/778123006/diff/40001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp:45:
'../../common/js/volume_manager_common.js',
On 2014/12/08 00:41:50, mtomasz wrote:
> What's the reason for this change? AFAIK this list should be sorted.

importer_common came to depend on entities from volume_manager_common. The
compiler was unhappy unless it processed volume_manager_common prior to
importer_common.

[Steve McKay, 2014-12-08 17:17:45]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2014-12-08 17:18:56]

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

[Steve McKay, 2014-12-08 17:36:17]

The CQ bit was unchecked by smckay@chromium.org

[Steve McKay, 2014-12-08 17:38:09]

Add test files missing from CL.

[Steve McKay, 2014-12-08 17:38:23]

Looks like the test files were missing from the CL.
Added. Holding off on submit until you get a chance to review.

[mtomasz, 2014-12-11 01:45:15]

On 2014/12/08 17:38:23, Steve McKay wrote:
> Looks like the test files were missing from the CL.
> Added. Holding off on submit until you get a chance to review.

Sorry, I missed it. Please ping me if you need a post lgtm review. I'll take a
look in a sec.

[mtomasz, 2014-12-11 01:51:52]

lgtm with nits.

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/importer_common_unittest.html
(right):

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/importer_common_unittest.html:11:
<script src="../../../../../ui/webui/resources/js/cr/event_target.js"></script>
nit: I think some of these includes are not used. Please check and remove.

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/importer_common_unittest.js
(right):

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/importer_common_unittest.js:14: /**
@type {!VolumeInfo} */
Some variables are !Foobar, and some !Foobar|undefined. Please choose the one
which is better (I don't really remember, @fukino would know, but you may know
too).

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/importer_common_unittest.js:109:
assertTrue(importer.isMediaDirectory(mediaDirectory, volumeManager));
Can we add more edge cases: Eg. /DCIM vs. /DCIM/, as well as whether /dir/DCIM
and /DCIM/XYZ are media directories? These are edge cases we discussed during
the review.

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

Respond to review comments.

[Steve McKay, 2014-12-11 02:12:57]

Thanks. Dones. Submitting.

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/importer_common_unittest.html
(right):

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/importer_common_unittest.html:11:
<script src="../../../../../ui/webui/resources/js/cr/event_target.js"></script>
On 2014/12/11 01:51:52, mtomasz wrote:
> nit: I think some of these includes are not used. Please check and remove.

I added these individually (based on what I found in another test). Sadly, they
are all required.

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
File chrome/test/data/file_manager/unit_tests/importer_common_unittest.js
(right):

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/importer_common_unittest.js:14: /**
@type {!VolumeInfo} */
On 2014/12/11 01:51:52, mtomasz wrote:
> Some variables are !Foobar, and some !Foobar|undefined. Please choose the one
> which is better (I don't really remember, @fukino would know, but you may know
> too).

Nuked |undefined which was unnecessary. Plus we don't even compile the tests.

https://codereview.chromium.org/778123006/diff/80001/chrome/test/data/file_ma...
chrome/test/data/file_manager/unit_tests/importer_common_unittest.js:109:
assertTrue(importer.isMediaDirectory(mediaDirectory, volumeManager));
On 2014/12/11 01:51:52, mtomasz wrote:
> Can we add more edge cases: Eg. /DCIM vs. /DCIM/, as well as whether /dir/DCIM
> and /DCIM/XYZ are media directories? These are edge cases we discussed during
> the review.

Done.

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

The CQ bit was checked by smckay@chromium.org

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

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

[commit-bot: I haz the power, 2014-12-11 03:30:25]

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

[commit-bot: I haz the power, 2014-12-11 03:30:26]

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_...)

[mtomasz, 2014-12-12 03:35:12]

https://codereview.chromium.org/778123006/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/778123006/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:56:
console.assert(volumeInfoProvider != null);
nit: !==. Please git diff master | grep ==\s and !=\s or something like that.

https://codereview.chromium.org/778123006/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:78: if (fullPath !==
'/DCIM' && fullPath != '/DCIM/') {
ditto

[Steve McKay, 2014-12-12 21:25:18]

Update tests in wake of test file music chairs. Fix tests in wake of responding
to review commnents.

[Steve McKay, 2014-12-12 21:27:52]

The CQ bit was checked by smckay@chromium.org

[Steve McKay, 2014-12-12 21:28:35]

Thanks! Fixed the test issues (prob was due to test files being moved).

Submitting.

https://codereview.chromium.org/778123006/diff/100001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/importer_common.js (right):

https://codereview.chromium.org/778123006/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:56:
console.assert(volumeInfoProvider != null);
On 2014/12/12 03:35:12, mtomasz wrote:
> nit: !==. Please git diff master | grep ==\s and !=\s or something like that.

Done.

https://codereview.chromium.org/778123006/diff/100001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/importer_common.js:78: if (fullPath !==
'/DCIM' && fullPath != '/DCIM/') {
On 2014/12/12 03:35:12, mtomasz wrote:
> ditto

Done.

[commit-bot: I haz the power, 2014-12-12 21:29:48]

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

[Steve McKay, 2014-12-12 21:34:43]

The CQ bit was unchecked by smckay@chromium.org

[Steve McKay, 2014-12-12 21:36:05]

Add test files missing after move.

[Steve McKay, 2014-12-12 21:36:18]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:36:58]

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

[commit-bot: I haz the power, 2014-12-13 00:24:39]

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

[commit-bot: I haz the power, 2014-12-13 00:24:40]

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_...)

[Steve McKay, 2014-12-15 20:16:56]

Include VolumeManagerCommon in device handler test as it depends on import
common.

[Steve McKay, 2014-12-15 20:17:19]

The CQ bit was checked by smckay@chromium.org

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

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

[commit-bot: I haz the power, 2014-12-15 21:07:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-15 21:08:09]

Patchset 9 (id:??) landed as
https://crrev.com/15d1aae4da7783607df3fac2d38198e817cf6fa0
Cr-Commit-Position: refs/heads/master@{#308414}

[samuong, 2014-12-15 22:24:28]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/807583003/ by samuong@chromium.org.

The reason for reverting is: seems to be causing crashes on chrome os:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
.

[Steve McKay, 2014-12-15 22:57:37]

On 2014/12/15 22:24:28, samuong wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/807583003/ by mailto:samuong@chromium.org.
> 
> The reason for reverting is: seems to be causing crashes on chrome os:
> 
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
>
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
> .

Looks like there was a race between two of my CLs. Thanks for keeping the tree
open. Tomasz, I'll resend the CL separately with the (one-liner) fix.