mediaview: Mount ARC documents provider file system volumes.

mediaview: Mount ARC documents provider file system volumes.

New media view volumes are added to chromeos::VolumeManager so that they are
available to Files.app. UI patches are coming later, so at this point it looks like
unusable unknown volumes appear in Files.app. To hide them from users for now,
the feature is guarded with base::FeatureList. Once UI is ready I will make them
enabled by default.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=Media views work with local pending UI patches

Review-Url: https://codereview.chromium.org/2580303002
Cr-Commit-Position: refs/heads/master@{#443205}
Committed: https://chromium.googlesource.com/chromium/src/+/4b8fa62ab223d4db0ea5d1a8359cbaeba3d757a6

[Shuhei Takahashi, 2016-12-19 05:56:01]

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

[commit-bot: I haz the power, 2016-12-19 05:56:17]

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

[Shuhei Takahashi, 2017-01-05 09:15:00]

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

[commit-bot: I haz the power, 2017-01-05 09:15:08]

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, 2017-01-05 10:04:54]

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

[commit-bot: I haz the power, 2017-01-05 10:04:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shuhei Takahashi, 2017-01-06 08:20:44]

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

[commit-bot: I haz the power, 2017-01-06 08:20:54]

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

[Shuhei Takahashi, 2017-01-06 08:29:39]

Description was changed from

==========
mediaview: Mount ARC documents provider file system.

BUG=chromium:671511
TEST=unit_tests
TEST=trybot
==========

to

==========
mediaview: Mount ARC documents provider file system.

New volumes are guarded with base::FeatureList so that unusable volumes
are not displayed in Files.app until UI gets ready.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=trybot
==========

[Shuhei Takahashi, 2017-01-06 08:32:13]

Description was changed from

==========
mediaview: Mount ARC documents provider file system.

New volumes are guarded with base::FeatureList so that unusable volumes
are not displayed in Files.app until UI gets ready.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=trybot
==========

to

==========
mediaview: Mount ARC documents provider file system volumes.

New volumes are guarded with base::FeatureList so that unusable volumes
are not displayed in Files.app until UI gets ready.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=trybot
==========

[commit-bot: I haz the power, 2017-01-06 09:14:50]

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

[commit-bot: I haz the power, 2017-01-06 09:14:51]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-01-06 09:29:02]

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

[commit-bot: I haz the power, 2017-01-06 09:30:19]

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

[Shuhei Takahashi, 2017-01-06 09:32:56]

Description was changed from

==========
mediaview: Mount ARC documents provider file system volumes.

New volumes are guarded with base::FeatureList so that unusable volumes
are not displayed in Files.app until UI gets ready.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=trybot
==========

to

==========
mediaview: Mount ARC documents provider file system volumes.

New media view volumes are added to chromeos::VolumeManager so that they are
available to Files.app. UI patches are coming later, so at this point it looks
like
unusable unknown volumes appear in Files.app. To hide them from users for now,
the feature is guarded with base::FeatureList. Once UI is ready I will make them
enabled by default.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=trybot
==========

[Shuhei Takahashi, 2017-01-06 09:33:35]

Description was changed from

==========
mediaview: Mount ARC documents provider file system volumes.

New media view volumes are added to chromeos::VolumeManager so that they are
available to Files.app. UI patches are coming later, so at this point it looks
like
unusable unknown volumes appear in Files.app. To hide them from users for now,
the feature is guarded with base::FeatureList. Once UI is ready I will make them
enabled by default.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=trybot
==========

to

==========
mediaview: Mount ARC documents provider file system volumes.

New media view volumes are added to chromeos::VolumeManager so that they are
available to Files.app. UI patches are coming later, so at this point it looks
like
unusable unknown volumes appear in Files.app. To hide them from users for now,
the feature is guarded with base::FeatureList. Once UI is ready I will make them
enabled by default.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=Media views work with local pending UI patches
==========

[Shuhei Takahashi, 2017-01-06 09:52:27]

nya@chromium.org changed reviewers:
+ lhchavez@lhchavez.com, michaelpg@chromium.org, mtomasz@chromium.org

[Shuhei Takahashi, 2017-01-06 09:52:29]

lhchavez:
PTAL following files:
- components/arc/...
- chrome/browser/chromeos/arc/...

mtomasz:
PTAL following files:
- chrome/browser/chromeos/extensions/file_manager/private_api_util.cc
- chrome/browser/chromeos/file_manager/volume_manager.{cc,h}
- chrome/browser/chromeos/fileapi/file_system_backend.cc
- chrome/common/extensions/api/file_manager_private.idl

michaelpg:
PTAL following file:
- third_party/closure_compiler/externs/file_manager_private.js (auto-generated)

[commit-bot: I haz the power, 2017-01-06 11:38:25]

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

[commit-bot: I haz the power, 2017-01-06 11:38:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[michaelpg, 2017-01-09 18:40:23]

third_party RS LGTM

[Shuhei Takahashi, 2017-01-10 04:08:15]

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

[commit-bot: I haz the power, 2017-01-10 04:09:00]

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

[Luis Héctor Chávez, 2017-01-10 05:49:04]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org
- lhchavez@lhchavez.com

[Luis Héctor Chávez, 2017-01-10 05:49:04]

oops, didn't see this in my review queue since the wrong lhchavez was added
(i'll add a message to select the other one to avoid this in the future). sorry
for the delay :(

https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):

https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_service_launcher.cc:106:
file_system_service->AddObserver(
This looks kind of weird. Can we either:

a) Preferred: create a small interface in components/arc that has
Add/RemoveObserver(ArcFileSystemObserver*), and have ArcServiceManager expose
that directly so that the volume_manager can subscribe to ArcFileSystemObserver
events directly instead of ArcServiceManager re-emit all the events, which does
not scale well and might get out of hand if you want to add more events. This
might become easier and less boilerplatey if we land something like
https://codereview.chromium.org/2622843002
a) Make this a Delegate instead so we can pass it to ArcFileSystemService's
constructor.

[Luis Héctor Chávez, 2017-01-10 05:52:40]

On 2017/01/10 05:49:04, Luis Héctor Chávez wrote:
> oops, didn't see this in my review queue since the wrong lhchavez was added
> (i'll add a message to select the other one to avoid this in the future).
sorry
> for the delay :(
> 
>
https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
> File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):
> 
>
https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
> chrome/browser/chromeos/arc/arc_service_launcher.cc:106:
> file_system_service->AddObserver(
> This looks kind of weird. Can we either:
> 
> a) Preferred: create a small interface in components/arc that has
> Add/RemoveObserver(ArcFileSystemObserver*),

come to think of it, that's not necessary since you'll be returning a raw
pointer: just fwd-declare the class and the compiler should be happy.

> and have ArcServiceManager expose
> that directly so that the volume_manager can subscribe to
ArcFileSystemObserver
> events directly instead of ArcServiceManager re-emit all the events, which
does
> not scale well and might get out of hand if you want to add more events. This
> might become easier and less boilerplatey if we land something like
> https://codereview.chromium.org/2622843002
> a) Make this a Delegate instead so we can pass it to ArcFileSystemService's
> constructor.

[commit-bot: I haz the power, 2017-01-10 06:19:30]

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

[commit-bot: I haz the power, 2017-01-10 06:19:31]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-01-10 14:10:33]

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

[commit-bot: I haz the power, 2017-01-10 14:10:48]

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

[Shuhei Takahashi, 2017-01-10 14:13:33]

On 2017/01/10 05:52:40, Luis Héctor Chávez wrote:
> On 2017/01/10 05:49:04, Luis Héctor Chávez wrote:
> > oops, didn't see this in my review queue since the wrong lhchavez was added
> > (i'll add a message to select the other one to avoid this in the future).
> sorry
> > for the delay :(

Oops, sorry for another mistake... Rietveld suggests a wrong address before
@chromium.org so I often choose wrong one :(


> > 
> >
>
https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
> > chrome/browser/chromeos/arc/arc_service_launcher.cc:106:
> > file_system_service->AddObserver(
> > This looks kind of weird. Can we either:
> > 
> > a) Preferred: create a small interface in components/arc that has
> > Add/RemoveObserver(ArcFileSystemObserver*),
> 
> come to think of it, that's not necessary since you'll be returning a raw
> pointer: just fwd-declare the class and the compiler should be happy.
> 
> > and have ArcServiceManager expose
> > that directly so that the volume_manager can subscribe to
> ArcFileSystemObserver
> > events directly instead of ArcServiceManager re-emit all the events, which
> does
> > not scale well and might get out of hand if you want to add more events.
This
> > might become easier and less boilerplatey if we land something like
> > https://codereview.chromium.org/2622843002
> > a) Make this a Delegate instead so we can pass it to ArcFileSystemService's
> > constructor.

I'm not very sure I correctly understand your intent, but I guess you meant
adding a getter of ArcFileSystemService to ArcServiceManager so that
VolumeManager can subscribe to ArcFileSystemService directly? I've updated the
patch accordingly, but if I'm misunderstanding please let me know.

[commit-bot: I haz the power, 2017-01-10 15:14:53]

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

[commit-bot: I haz the power, 2017-01-10 15:14:54]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2017-01-10 18:51:25]

On 2017/01/10 14:13:33, Shuhei Takahashi wrote:
> On 2017/01/10 05:52:40, Luis Héctor Chávez wrote:
> > On 2017/01/10 05:49:04, Luis Héctor Chávez wrote:
> > > oops, didn't see this in my review queue since the wrong lhchavez was
added
> > > (i'll add a message to select the other one to avoid this in the future).
> > sorry
> > > for the delay :(
> 
> Oops, sorry for another mistake... Rietveld suggests a wrong address before
> @chromium.org so I often choose wrong one :(
> 
> 
> > > 
> > >
> >
>
https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
> > > File chrome/browser/chromeos/arc/arc_service_launcher.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2580303002/diff/140001/chrome/browser/chromeo...
> > > chrome/browser/chromeos/arc/arc_service_launcher.cc:106:
> > > file_system_service->AddObserver(
> > > This looks kind of weird. Can we either:
> > > 
> > > a) Preferred: create a small interface in components/arc that has
> > > Add/RemoveObserver(ArcFileSystemObserver*),
> > 
> > come to think of it, that's not necessary since you'll be returning a raw
> > pointer: just fwd-declare the class and the compiler should be happy.
> > 
> > > and have ArcServiceManager expose
> > > that directly so that the volume_manager can subscribe to
> > ArcFileSystemObserver
> > > events directly instead of ArcServiceManager re-emit all the events, which
> > does
> > > not scale well and might get out of hand if you want to add more events.
> This
> > > might become easier and less boilerplatey if we land something like
> > > https://codereview.chromium.org/2622843002
> > > a) Make this a Delegate instead so we can pass it to
ArcFileSystemService's
> > > constructor.
> 
> I'm not very sure I correctly understand your intent, but I guess you meant
> adding a getter of ArcFileSystemService to ArcServiceManager so that
> VolumeManager can subscribe to ArcFileSystemService directly? I've updated the
> patch accordingly, but if I'm misunderstanding please let me know.

yup, that was what I had in mind, thanks!

[Luis Héctor Chávez, 2017-01-10 18:51:35]

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:751: void
VolumeManager::OnArcShutdown() {
Unfortunately this was reverted :(

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:756:
DoUnmountEvent(chromeos::MOUNT_ERROR_NONE,
What happens if OnFileSystemsReady() was never called?

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:768:
arc::ArcServiceManager::Get()->file_system_service()->RemoveObserver(this);
nit: unsubscribe in the reverse order in which you subscribed.

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:55: file_system_service_(nullptr) {
nit: can you initialize |file_system_service_ = nullptr| in the .h file to avoid
this?

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
components/arc/arc_service_manager.h:35: virtual void OnArcShutdown() {}
nit: can you leave them as they were before? we want people to try not to use
it, and having them all as = 0 introduces a desirable amount of friction.

[mtomasz, 2017-01-11 09:35:22]

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:22:
base::FilePath GetDocumentsProviderMountPath(
Is this going to be reused for any document provider? If, so then
root_document_id can be anything, and should be escaped.

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:32:
std::string* root_document_id,
nit: Is the returned root_document_id escaped or not? Can we add a comment in
the header file?

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/file_system_backend.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/file_system_backend.cc:173: root_url +=
authority + "/" + root_document_id + "/";
Does root_document_id need to be escaped?

[Shuhei Takahashi, 2017-01-11 14:20:54]

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

[commit-bot: I haz the power, 2017-01-11 14:21:19]

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, 2017-01-11 15:15:07]

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

[commit-bot: I haz the power, 2017-01-11 15:15:08]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-01-11 15:18:31]

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

[commit-bot: I haz the power, 2017-01-11 15:18:47]

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

[Shuhei Takahashi, 2017-01-11 15:20:43]

PTAL

I'll address mtomasz's comments tomorrow.

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:751: void
VolumeManager::OnArcShutdown() {
On 2017/01/10 18:51:35, Luis Héctor Chávez wrote:
> Unfortunately this was reverted :(

Thanks for heads-up. Rebased on tot.

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:756:
DoUnmountEvent(chromeos::MOUNT_ERROR_NONE,
On 2017/01/10 18:51:35, Luis Héctor Chávez wrote:
> What happens if OnFileSystemsReady() was never called?

It is okay to call DoUnmountEvent() even if mounts do not exist.

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:768:
arc::ArcServiceManager::Get()->file_system_service()->RemoveObserver(this);
On 2017/01/10 18:51:35, Luis Héctor Chávez wrote:
> nit: unsubscribe in the reverse order in which you subscribed.

Done (actually we now observe only one instance).

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
components/arc/arc_service_manager.cc:55: file_system_service_(nullptr) {
On 2017/01/10 18:51:35, Luis Héctor Chávez wrote:
> nit: can you initialize |file_system_service_ = nullptr| in the .h file to
avoid
> this?

Done.

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2580303002/diff/160001/components/arc/arc_ser...
components/arc/arc_service_manager.h:35: virtual void OnArcShutdown() {}
On 2017/01/10 18:51:35, Luis Héctor Chávez wrote:
> nit: can you leave them as they were before? we want people to try not to use
> it, and having them all as = 0 introduces a desirable amount of friction.

Done.

[commit-bot: I haz the power, 2017-01-11 16:28:13]

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

[commit-bot: I haz the power, 2017-01-11 16:28:14]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2017-01-11 16:48:45]

lgtm

https://codereview.chromium.org/2580303002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:435:
arc::ArcServiceManager::Get()->file_system_service()->RemoveObserver(this);
Should you call OnFileSystemsClosed() here?

[Luis Héctor Chávez, 2017-01-11 17:10:45]

https://codereview.chromium.org/2580303002/diff/220001/components/arc/arc_ser...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2580303002/diff/220001/components/arc/arc_ser...
components/arc/arc_service_manager.h:84: void
set_file_system_service(ArcFileSystemService* file_system_service) {
FYI the support for the generic getter has landed, so you can now remove this :)

[Shuhei Takahashi, 2017-01-12 07:12:10]

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

[commit-bot: I haz the power, 2017-01-12 07:12:25]

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, 2017-01-12 07:14:05]

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

[commit-bot: I haz the power, 2017-01-12 07:14:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Shuhei Takahashi, 2017-01-12 08:20:10]

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

[commit-bot: I haz the power, 2017-01-12 08:20:19]

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

[Shuhei Takahashi, 2017-01-12 08:20:28]

mtomasz: PTAL

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:22:
base::FilePath GetDocumentsProviderMountPath(
On 2017/01/11 09:35:22, mtomasz (OOO until Jan 5th) wrote:
> Is this going to be reused for any document provider? If, so then
> root_document_id can be anything, and should be escaped.

Right, done.

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:32:
std::string* root_document_id,
On 2017/01/11 09:35:22, mtomasz (OOO until Jan 5th) wrote:
> nit: Is the returned root_document_id escaped or not? Can we add a comment in
> the header file?

Done.

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/fileapi/file_system_backend.cc (right):

https://codereview.chromium.org/2580303002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/fileapi/file_system_backend.cc:173: root_url +=
authority + "/" + root_document_id + "/";
On 2017/01/11 09:35:22, mtomasz (OOO until Jan 5th) wrote:
> Does root_document_id need to be escaped?

You're right. Done.

[commit-bot: I haz the power, 2017-01-12 08:22:01]

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

[commit-bot: I haz the power, 2017-01-12 08:22:02]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mtomasz, 2017-01-12 08:39:20]

lgtm!

[Shuhei Takahashi, 2017-01-12 08:52:13]

Thanks for reviewing!

https://codereview.chromium.org/2580303002/diff/220001/components/arc/arc_ser...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2580303002/diff/220001/components/arc/arc_ser...
components/arc/arc_service_manager.h:84: void
set_file_system_service(ArcFileSystemService* file_system_service) {
On 2017/01/11 17:10:45, Luis Héctor Chávez wrote:
> FYI the support for the generic getter has landed, so you can now remove this
:)

Thanks, I've updated the patch to use the brand new generic getter.

[Shuhei Takahashi, 2017-01-12 08:52:25]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2017-01-12 08:52:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org, lhchavez@chromium.org, mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/2580303002/#ps320001
(title: "Use the generic service getter.")

[commit-bot: I haz the power, 2017-01-12 08:52:42]

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

[Shuhei Takahashi, 2017-01-12 08:59:59]

The CQ bit was unchecked by nya@chromium.org

[hidehiko, 2017-01-12 09:00:44]

hidehiko@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hidehiko, 2017-01-12 09:00:46]

FYI.

https://codereview.chromium.org/2580303002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:436:
arc::ArcServiceManager::Get()->GetService<arc::ArcFileSystemService>()
ArcServiceManager is destroyed before KeyedService.
So, you'd need null check here...

[Shuhei Takahashi, 2017-01-12 09:29:00]

hidehiko, thanks for catching it!

Could you please take a look if the fix is correct?

https://codereview.chromium.org/2580303002/diff/320001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/320001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:436:
arc::ArcServiceManager::Get()->GetService<arc::ArcFileSystemService>()
On 2017/01/12 09:00:46, hidehiko wrote:
> ArcServiceManager is destroyed before KeyedService.
> So, you'd need null check here...

Thanks for catching!

[Shuhei Takahashi, 2017-01-12 09:29:10]

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

[commit-bot: I haz the power, 2017-01-12 09:29:23]

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

[hidehiko, 2017-01-12 09:31:34]

LGTM with one minor comment. Thank you for quick fix!

https://codereview.chromium.org/2580303002/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:414:
->GetService<arc::ArcFileSystemService>()
arc::ArcServiceManager::GetGlobalService<...>()
for consistency?

[Shuhei Takahashi, 2017-01-12 09:34:30]

Thanks for taking a look, hidehiko.

https://codereview.chromium.org/2580303002/diff/340001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2580303002/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:414:
->GetService<arc::ArcFileSystemService>()
On 2017/01/12 09:31:34, hidehiko wrote:
> arc::ArcServiceManager::GetGlobalService<...>()
> for consistency?

Oops, I failed to replace this call...

[Shuhei Takahashi, 2017-01-12 09:35:02]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2017-01-12 09:35:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, lhchavez@chromium.org, michaelpg@chromium.org,
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2580303002/#ps360001
(title: "Consistently use GetGlobalService().")

[commit-bot: I haz the power, 2017-01-12 09:35:13]

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, 2017-01-12 10:57:30]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1484213702244190,
"parent_rev": "9b7705c2d31d7536d7fee713c24fd598a62b63c7", "commit_rev":
"4b8fa62ab223d4db0ea5d1a8359cbaeba3d757a6"}

[commit-bot: I haz the power, 2017-01-12 10:58:09]

Description was changed from

==========
mediaview: Mount ARC documents provider file system volumes.

New media view volumes are added to chromeos::VolumeManager so that they are
available to Files.app. UI patches are coming later, so at this point it looks
like
unusable unknown volumes appear in Files.app. To hide them from users for now,
the feature is guarded with base::FeatureList. Once UI is ready I will make them
enabled by default.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=Media views work with local pending UI patches
==========

to

==========
mediaview: Mount ARC documents provider file system volumes.

New media view volumes are added to chromeos::VolumeManager so that they are
available to Files.app. UI patches are coming later, so at this point it looks
like
unusable unknown volumes appear in Files.app. To hide them from users for now,
the feature is guarded with base::FeatureList. Once UI is ready I will make them
enabled by default.

BUG=chromium:671511
TEST=New (unusable) volumes are shown with --enable-features=ArcMediaView
TEST=Media views work with local pending UI patches

Review-Url: https://codereview.chromium.org/2580303002
Cr-Commit-Position: refs/heads/master@{#443205}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b8fa62ab223d4db0ea5d1a8359c...
==========

[commit-bot: I haz the power, 2017-01-12 10:58:11]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/4b8fa62ab223d4db0ea5d1a8359c...

[fukino, 2017-01-12 11:31:30]

On 2017/01/12 10:58:11, commit-bot: I haz the power wrote:
> Committed patchset #19 (id:360001) as
>
https://chromium.googlesource.com/chromium/src/+/4b8fa62ab223d4db0ea5d1a8359c...

It seems the change on file_manager_private.js broke the closure compilation.
I'll fix it.

[Shuhei Takahashi, 2017-01-12 12:46:14]

FYI, the fix by Fukino-san is:
https://codereview.chromium.org/2625233004/

On 2017/01/12 11:31:30, fukino wrote:
> On 2017/01/12 10:58:11, commit-bot: I haz the power wrote:
> > Committed patchset #19 (id:360001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/4b8fa62ab223d4db0ea5d1a8359c...
> 
> It seems the change on file_manager_private.js broke the closure compilation.
> I'll fix it.

Sorry for breakage and thanks for the fix.