Divide volume_manager.js into files for each classes and extract interfaces from them.

Divide volume_manager.js into files for each classes and extract interfaces from them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts can refer to the interface definitions without depending on their implementation classes.

BUG=636289
TEST=run browser_tests

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

[fukino, 2016-08-30 13:51:18]

Description was changed from

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

BUG=636289
TEST=run browser_tests
==========

to

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on implementations.

BUG=636289
TEST=run browser_tests
==========

[fukino, 2016-08-30 13:51:45]

Description was changed from

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on implementations.

BUG=636289
TEST=run browser_tests
==========

to

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on their implementation
classes.

BUG=636289
TEST=run browser_tests
==========

[fukino, 2016-08-30 13:52:01]

fukino@chromium.org changed reviewers:
+ oka@chromium.org

[fukino, 2016-08-30 13:52:02]

PTAL. Thanks!

[fukino, 2016-08-30 13:52:09]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 13:52:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 14:42:32]

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

[commit-bot: I haz the power, 2016-08-30 14:42:33]

Dry run: Exceeded global retry quota

[oka, 2016-09-01 09:05:24]

Why don't you update the targets corresponding to the split files in
compiled_resources2.gyp ?

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/bac...
File ui/file_manager/externs/background_window_common.js (right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/bac...
ui/file_manager/externs/background_window_common.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
You may want to extract interface for volume_manager_wrapper.js instead of
volumeManagerFactory to speed up compile speed and simplify the dependency of
audio_player, gallery, video_player and main (file_manager).

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/bac...
ui/file_manager/externs/background_window_common.js:8: * Returns the
VolumeManager instance asynchronously. If it is not created or
The comment is duplicated with js/volume_manager_factory.js .
Why don't you remove the comments?

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/ent...
File ui/file_manager/externs/entry_location.js (right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/ent...
ui/file_manager/externs/entry_location.js:31: * Whether the location obtained
from the fake entry correspond to special
nit: corresponds

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/volume_manager_factory.js
(right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_factory.js:19: *
Returns the VolumeManager instance asynchronously. If it is not created or
Nit: If it has not been created or is under initialization  ?

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_factory.js:26:
function getInstance(opt_callback) {
I think you can remove opt_callback.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/volume_manager_util.js (right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_util.js:12: *
mount/unmount received the request supposed failed.
Add annotation.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_util.js:219:
volumeManagerUtil.volumeListOrder_ = [
capitalize this constant value.

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

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/volume_manager_wrapper.js:163: event
= /** @type {!ExternallyUnmountedEvent} */ (event);
Is this an intended change?

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/volume_manager_wrapper.js:168: if
(eventVolumeType && this.isAllowedVolume_(eventVolumeType))
ditto

[fukino, 2016-09-01 12:04:25]

Thanks!
I'll update compiled_resources2.gyp on top of this CL in a separate CL, as I
need to modify outside of file_manager and this CL is already big enough.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/bac...
File ui/file_manager/externs/background_window_common.js (right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/bac...
ui/file_manager/externs/background_window_common.js:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/09/01 09:05:23, oka wrote:
> You may want to extract interface for volume_manager_wrapper.js instead of
> volumeManagerFactory to speed up compile speed and simplify the dependency of
> audio_player, gallery, video_player and main (file_manager).
> 

I added this extern file for foreground scripts to be able to know the exported
namespace/objects from background script.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/bac...
ui/file_manager/externs/background_window_common.js:8: * Returns the
VolumeManager instance asynchronously. If it is not created or
On 2016/09/01 09:05:23, oka wrote:
> The comment is duplicated with js/volume_manager_factory.js .
> Why don't you remove the comments?

Removed redundant comments from this file.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/ent...
File ui/file_manager/externs/entry_location.js (right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/externs/ent...
ui/file_manager/externs/entry_location.js:31: * Whether the location obtained
from the fake entry correspond to special
On 2016/09/01 09:05:23, oka wrote:
> nit: corresponds

Done.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/volume_manager_factory.js
(right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_factory.js:19: *
Returns the VolumeManager instance asynchronously. If it is not created or
On 2016/09/01 09:05:23, oka wrote:
> Nit: If it has not been created or is under initialization  ?

Done.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_factory.js:26:
function getInstance(opt_callback) {
On 2016/09/01 09:05:23, oka wrote:
> I think you can remove opt_callback.

The callback is use in some places
(e.g.https://codesearch.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/volume_manager_wrapper.js?q=VolumeManager.getInstance&sq=package:chromium&l=51&dr=C)
Making them use returned promise is out of this CL's scope.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/volume_manager_util.js (right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_util.js:12: *
mount/unmount received the request supposed failed.
On 2016/09/01 09:05:24, oka wrote:
> Add annotation.

Done.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_util.js:219:
volumeManagerUtil.volumeListOrder_ = [
On 2016/09/01 09:05:23, oka wrote:
> capitalize this constant value.

Done.
Additionally, as this is used only in volume_info_list_impl.js, I moved the
definition to the file.

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

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/volume_manager_wrapper.js:163: event
= /** @type {!ExternallyUnmountedEvent} */ (event);
On 2016/09/01 09:05:24, oka wrote:
> Is this an intended change?

Yes.
After I reorganized the files, closure compiler newly detected that
event.volumeInfo.volumeType is not safe.

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/volume_manager_wrapper.js:168: if
(eventVolumeType && this.isAllowedVolume_(eventVolumeType))
On 2016/09/01 09:05:24, oka wrote:
> ditto

Yes.
Somehow closure compiler detected new suspicious code.

[fukino, 2016-09-01 13:18:59]

Oops, I've just uploaded the new patch set. (I forgot to type git cl upload..)

PTAL

[fukino, 2016-09-02 05:36:05]

Ping > oka@ san

[oka, 2016-09-02 06:01:52]

lgtm

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/background/js/volume_manager_factory.js
(right):

https://codereview.chromium.org/2292873003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/background/js/volume_manager_factory.js:26:
function getInstance(opt_callback) {
On 2016/09/01 12:04:25, fukino wrote:
> On 2016/09/01 09:05:23, oka wrote:
> > I think you can remove opt_callback.
> 
> The callback is use in some places
>
(e.g.https://codesearch.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/volume_manager_wrapper.js?q=VolumeManager.getInstance&sq=package:chromium&l=51&dr=C)
> Making them use returned promise is out of this CL's scope.

Oops. Sorry for the mistake.

[fukino, 2016-09-02 06:04:00]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 06:04:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 06:45:59]

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

[commit-bot: I haz the power, 2016-09-02 06:46:00]

Dry run: This issue passed the CQ dry run.

[fukino, 2016-09-02 06:55:14]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2016-09-02 06:55:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 07:00:12]

Description was changed from

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on their implementation
classes.

BUG=636289
TEST=run browser_tests
==========

to

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on their implementation
classes.

BUG=636289
TEST=run browser_tests
==========

[commit-bot: I haz the power, 2016-09-02 07:00:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-02 07:03:06]

Description was changed from

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on their implementation
classes.

BUG=636289
TEST=run browser_tests
==========

to

==========
Divide volume_manager.js into files for each classes and extract interfaces from
them.

This CL divides classes in volume_manager.js into separate files.
In addition, it extracts interfaces from each classes so that foreground scripts
can refer to the interface definitions without depending on their implementation
classes.

BUG=636289
TEST=run browser_tests

Committed: https://crrev.com/a5abef97afdb57b24e5bce63fb10054ca0266b91
Cr-Commit-Position: refs/heads/master@{#416205}
==========

[commit-bot: I haz the power, 2016-09-02 07:03:07]

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