Immediately open MTP and other eligible removable devices with media directory on mount.

Immediately open MTP and other eligible removable devices with media directory on mount.

BUG=420680
TEST=browser_test: FileManagerJsTest.*

Committed: https://crrev.com/876b4210bb9ce206138f89c1721ad9a5bc8cb492
Cr-Commit-Position: refs/heads/master@{#312541}

[Steve McKay, 2015-01-16 23:35:16]

Revert unnecessary changes.

[Steve McKay, 2015-01-16 23:52:25]

Cleanup code. Add comments. Fiddle faddle.

[Steve McKay, 2015-01-16 23:56:29]

Patchset #1 (id:1) has been deleted

[Steve McKay, 2015-01-16 23:56:36]

Patchset #1 (id:20001) has been deleted

[Steve McKay, 2015-01-16 23:56:42]

Patchset #1 (id:40001) has been deleted

[Steve McKay, 2015-01-16 23:56:48]

Patchset #1 (id:60001) has been deleted

[Steve McKay, 2015-01-20 23:16:17]

Patchset #2 (id:100001) has been deleted

[Steve McKay, 2015-01-20 23:16:22]

Patchset #1 (id:80001) has been deleted

[Steve McKay, 2015-01-20 23:16:30]

Patchset #1 (id:120001) has been deleted

[Steve McKay, 2015-01-20 23:17:28]

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

[Steve McKay, 2015-01-21 23:56:23]

Refactor to lookup volume info on mount events.

[Steve McKay, 2015-01-22 00:05:29]

Minor fixups.

[Steve McKay, 2015-01-22 00:20:22]

Reinstate the multipleFail test, which was fully synchronous.

[mtomasz, 2015-01-22 00:31:14]

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:211: *     devicePath.
nit: @private missing.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:215: 
nit: Redundant \n.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:222: * @return
{!Promise<!DirectoryEntry>}
nit: Doesn't return?

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:247: })
nit: ; missing.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:259: 
nit: redundant ;

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:264: * otherwise just
nit: We can wrap it better.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:273: root,
opt_directoryPath, {create:false}));
nit: \s after :.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:285:
{currentDirectoryURL: directory.toURL()},
nit: indent

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:286: /* App ID */
undefined,
FYI: App ID argument is going to be removed in upcoming hours.
https://codereview.chromium.org/858133002/

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/device_handler.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:308: 
nit: Redundant \n.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:311: // NOTE:
device path is not set for MTP mounts (at least
Is the comment still valid?

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:404:
DeviceHandler.prototype.onMount_ = function(event) {
nit: Missing jsdoc.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:405: 
nit: redundant \n.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:406: // If this is
remounting, which happens when resuming ChromeOS, the device has
nit: ChromeOS -> Chrome OS.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:447: new Promise(
nit: Indentation in this block seems broken.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:467: if
(!!results[0] && results[0].isDirectory) {
nit: Do we need to check isDirectory? We called getDirectory() before anyway, so
it should be a dir.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/device_handler_unittest.js
(right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:143: });
nit: indent

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:415:
function ZZZZtestMulitpleFail(callback) {
nit: ZZZZ -> nothing.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:559:
assertTrue(volumeInfo != null);
nit: !==

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:559:
assertTrue(volumeInfo != null);
nit: != -> !== OR use !!.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/mock_volume_manager.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/mock_volume_manager.js:126: * Mock
class for VolumeManagerWrapper.
nit: Please add a TODO, to merge these both maps once wrapper is fixed.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/volume_manager.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_manager.js:398: var resolver =
this.volumeInfoResolvers_[volumeInfo.volumeId];
nit: Can you add a comment? It's not clear what it does, without reading
whenVolumeInfoReady.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_manager.js:493: var promise =
new Promise(
Will it work when whenVolumeInfoReady is called multiple times?

[Steve McKay, 2015-01-22 00:33:18]

Bind some unbound functions and fix a couple small bugs.

[Steve McKay, 2015-01-22 01:29:35]

Respond to review comments.

[Steve McKay, 2015-01-22 01:29:58]

PTAL.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/background.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:211: *     devicePath.
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: @private missing.

Fixed. Why doesn't compiler catch that?

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:222: * @return
{!Promise<!DirectoryEntry>}
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: Doesn't return?

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:247: })
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: ; missing.

Done. And I added a .bind.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:264: * otherwise just
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: We can wrap it better.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:273: root,
opt_directoryPath, {create:false}));
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: \s after :.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:285:
{currentDirectoryURL: directory.toURL()},
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: indent

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/background.js:286: /* App ID */
undefined,
On 2015/01/22 00:31:13, mtomasz wrote:
> FYI: App ID argument is going to be removed in upcoming hours.
> https://codereview.chromium.org/858133002/

Acknowledged.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/device_handler.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:308: 
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: Redundant \n.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:311: // NOTE:
device path is not set for MTP mounts (at least
On 2015/01/22 00:31:13, mtomasz wrote:
> Is the comment still valid?

Already nuked.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:404:
DeviceHandler.prototype.onMount_ = function(event) {
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: Missing jsdoc.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:405: 
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: redundant \n.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:447: new Promise(
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: Indentation in this block seems broken.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler.js:467: if
(!!results[0] && results[0].isDirectory) {
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: Do we need to check isDirectory? We called getDirectory() before anyway,
so
> it should be a dir.

I think so. What if there's a DCIM file. It's not likely, but it is possible.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/device_handler_unittest.js
(right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:143: });
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: indent

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:415:
function ZZZZtestMulitpleFail(callback) {
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: ZZZZ -> nothing.

Fixed. And it was fully synchronous!!!!

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:559:
assertTrue(volumeInfo != null);
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: != -> !== OR use !!.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:559:
assertTrue(volumeInfo != null);
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: !==

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/mock_volume_manager.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/mock_volume_manager.js:126: * Mock
class for VolumeManagerWrapper.
On 2015/01/22 00:31:13, mtomasz wrote:
> nit: Please add a TODO, to merge these both maps once wrapper is fixed.

Done.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/volume_manager.js (right):

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_manager.js:398: var resolver =
this.volumeInfoResolvers_[volumeInfo.volumeId];
On 2015/01/22 00:31:14, mtomasz wrote:
> nit: Can you add a comment? It's not clear what it does, without reading
> whenVolumeInfoReady.

Done. Also added cleanup in in the remove method.

https://codereview.chromium.org/858563005/diff/180001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/volume_manager.js:493: var promise =
new Promise(
On 2015/01/22 00:31:14, mtomasz wrote:
> Will it work when whenVolumeInfoReady is called multiple times?

Good catch. Fixed.

[mtomasz, 2015-01-22 01:31:40]

lgtm with a nit

https://codereview.chromium.org/858563005/diff/260001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/device_handler_unittest.js
(right):

https://codereview.chromium.org/858563005/diff/260001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:54:
function testtestGoodDevice(callback) {
nit: testest -> test, here and in other places.

[Steve McKay, 2015-01-22 01:43:02]

Respond to review comments.

[Steve McKay, 2015-01-22 01:43:39]

https://codereview.chromium.org/858563005/diff/260001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/background/js/device_handler_unittest.js
(right):

https://codereview.chromium.org/858563005/diff/260001/ui/file_manager/file_ma...
ui/file_manager/file_manager/background/js/device_handler_unittest.js:54:
function testtestGoodDevice(callback) {
On 2015/01/22 01:31:40, mtomasz wrote:
> nit: testest -> test, here and in other places.

Dones.

[Steve McKay, 2015-01-22 01:43:51]

The CQ bit was checked by smckay@chromium.org

[commit-bot: I haz the power, 2015-01-22 01:45:17]

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

[commit-bot: I haz the power, 2015-01-22 02:43:12]

Committed patchset #8 (id:280001)

[commit-bot: I haz the power, 2015-01-22 02:44:34]

Patchset 8 (id:??) landed as
https://crrev.com/876b4210bb9ce206138f89c1721ad9a5bc8cb492
Cr-Commit-Position: refs/heads/master@{#312541}