[fsp] Introduce file_system_provider::Service class for the FileSystemProvider API.

[fsp] Introduce file_system_provider::Service class for the FileSystemProvider API.

This patch introduces a service which manages file systems provided by third party extensions.

The class has two methods: registerFileSystem() and unregisterFileSystem() which are supposed to be called from
chrome.fileSystemProvider.* api methods. The service stores a map of the registered file systems, mounts them, and
notifies observers (VolumeManager) about the fact.

The file system backend does not handle the new file system type yet, this will be done separately.

TEST=browser_test, unit_tests: *FileSystemProvider*
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259820

[mtomasz, 2014-03-10 09:01:49]

@satorux, @kinaba: PTAL. Thanks.

[kinaba, 2014-03-12 01:12:36]

lgtm

https://codereview.chromium.org/192573002/diff/30023/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/observer.h (right):

https://codereview.chromium.org/192573002/diff/30023/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/observer.h:18: const
ProvidedFileSystem& file_system);
= 0?

https://codereview.chromium.org/192573002/diff/30023/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/observer.h:20: const
ProvidedFileSystem& file_system);
ditto

[mtomasz, 2014-03-12 03:06:18]

@kinuko: PTAL at fileapi::*. Thanks.

https://codereview.chromium.org/192573002/diff/30023/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/observer.h (right):

https://codereview.chromium.org/192573002/diff/30023/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/observer.h:18: const
ProvidedFileSystem& file_system);
On 2014/03/12 01:12:36, kinaba wrote:
> = 0?

Done.

https://codereview.chromium.org/192573002/diff/30023/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/observer.h:20: const
ProvidedFileSystem& file_system);
On 2014/03/12 01:12:36, kinaba wrote:
> ditto

Done.

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

@benwells: PTAL at the *.idl change. Thanks.

[kinuko, 2014-03-12 05:09:14]

fileapi changes lgtm

How much does this code depend on chromeos-specific code (just asking)? I'm
curious how easy/tough it'd be to support this in non-cros world.

[mtomasz, 2014-03-12 05:19:31]

On 2014/03/12 05:09:14, kinuko wrote:
> fileapi changes lgtm
> 
> How much does this code depend on chromeos-specific code (just asking)? I'm
> curious how easy/tough it'd be to support this in non-cros world.

@kinuko: As for now, there should be no dependencies on chromeos code at all.
However, I planned to use the chromeos backend for simplicity (not done yet).

We could however quite easily create a new standalone backend. This shouldn't be
much work, since all what the chromeos backend would do is to return the 
AsyncFileUtil, which is not going to depend on chromeos.

However, as for now, for simplicity I'd like to go with the chromeos backend,
but I have in mind supporting other platforms soon. WDYT?

[kinuko, 2014-03-12 05:29:22]

On 2014/03/12 05:19:31, mtomasz wrote:
> On 2014/03/12 05:09:14, kinuko wrote:
> > fileapi changes lgtm
> > 
> > How much does this code depend on chromeos-specific code (just asking)? I'm
> > curious how easy/tough it'd be to support this in non-cros world.
> 
> @kinuko: As for now, there should be no dependencies on chromeos code at all.
> However, I planned to use the chromeos backend for simplicity (not done yet).
> 
> We could however quite easily create a new standalone backend. This shouldn't
be
> much work, since all what the chromeos backend would do is to return the 
> AsyncFileUtil, which is not going to depend on chromeos.
> 
> However, as for now, for simplicity I'd like to go with the chromeos backend,
> but I have in mind supporting other platforms soon. WDYT?

Thanks for the brief plan & design context. SGTM

[benwells, 2014-03-12 09:15:49]

idl lgtm

[mtomasz, 2014-03-26 05:52:50]

@satorux: Ping.

[satorux1, 2014-03-26 07:55:36]

The code looks pretty good and is nicely tested. great work!

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:248: static const bool
kNotRemounting = false;
don't need static. IIRC, static like this generates complex code to make it
thread-safe.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:51: VOLUME_TYPE_PROVIDED,
could you add a comment about the new enum? this one looks rather cryptic
compared to others.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:198:
chromeos::file_system_provider::Service* file_system_provider_service_;
Please add a comment about who owns this object.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/observer.h (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/observer.h:15: class Observer {
Class comment is missing.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/observer.h:17: virtual void
OnProvidedFileSystemRegistered(
function comments are missing.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:28: base::FilePath
GetMountPointPath(Profile* profile,
function comment is missing. What's file_system_handle?

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:70: return
std::string();
std::string() is more correct, but I think "" is used in other places in this
directory. maybe "" for consistency.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:47: class
FileSystemProviderServiceTest : public testing::Test {
Thank you for writing the unit tests!

https://codereview.chromium.org/192573002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/mount/test.js
(right):

https://codereview.chromium.org/192573002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/mount/test.js:51:
function stressMountTest() {
nice test!

[satorux1, 2014-03-26 08:07:33]

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:25: class
LoggingObserver : public Observer {
class comment is missing.

[mtomasz, 2014-03-26 09:21:43]

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.cc:248: static const bool
kNotRemounting = false;
On 2014/03/26 07:55:37, satorux1 wrote:
> don't need static. IIRC, static like this generates complex code to make it
> thread-safe.

Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:51: VOLUME_TYPE_PROVIDED,
On 2014/03/26 07:55:37, satorux1 wrote:
> could you add a comment about the new enum? this one looks rather cryptic
> compared to others.

Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:198:
chromeos::file_system_provider::Service* file_system_provider_service_;
On 2014/03/26 07:55:37, satorux1 wrote:
> Please add a comment about who owns this object.

It is owned very deeply by BrowserContextKeyedServiceFactory, which is a
singleton freed on process termination. Same for #192. Added some comments.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/observer.h (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/observer.h:15: class Observer {
On 2014/03/26 07:55:37, satorux1 wrote:
> Class comment is missing.

Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/observer.h:17: virtual void
OnProvidedFileSystemRegistered(
On 2014/03/26 07:55:37, satorux1 wrote:
> function comments are missing.

Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:28: base::FilePath
GetMountPointPath(Profile* profile,
On 2014/03/26 07:55:37, satorux1 wrote:
> function comment is missing. What's file_system_handle?

This is simplified in https://codereview.chromium.org/209663006/ CL.
Added a comment as for now. Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:70: return
std::string();
On 2014/03/26 07:55:37, satorux1 wrote:
> std::string() is more correct, but I think "" is used in other places in this
> directory. maybe "" for consistency.

Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:25: class
LoggingObserver : public Observer {
On 2014/03/26 08:07:33, satorux1 wrote:
> class comment is missing.

Done.

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:47: class
FileSystemProviderServiceTest : public testing::Test {
On 2014/03/26 07:55:37, satorux1 wrote:
> Thank you for writing the unit tests!

Thx!

https://codereview.chromium.org/192573002/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/mount/test.js
(right):

https://codereview.chromium.org/192573002/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/mount/test.js:51:
function stressMountTest() {
On 2014/03/26 07:55:37, satorux1 wrote:
> nice test!

Thx!

[satorux1, 2014-03-27 04:38:55]

LGTM

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:198:
chromeos::file_system_provider::Service* file_system_provider_service_;
On 2014/03/26 09:21:43, mtomasz OOO until 21.03 wrote:
> On 2014/03/26 07:55:37, satorux1 wrote:
> > Please add a comment about who owns this object.
> 
> It is owned very deeply by BrowserContextKeyedServiceFactory, which is a
> singleton freed on process termination. Same for #192. Added some comments.

Thanks. I was interested in whether or not if this object is owned by this
class. Maybe:

// Not owned by this class.

was sufficient.

[mtomasz, 2014-03-27 04:43:32]

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager.h (right):

https://codereview.chromium.org/192573002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager.h:198:
chromeos::file_system_provider::Service* file_system_provider_service_;
On 2014/03/27 04:38:55, satorux1 wrote:
> On 2014/03/26 09:21:43, mtomasz OOO until 21.03 wrote:
> > On 2014/03/26 07:55:37, satorux1 wrote:
> > > Please add a comment about who owns this object.
> > 
> > It is owned very deeply by BrowserContextKeyedServiceFactory, which is a
> > singleton freed on process termination. Same for #192. Added some comments.
> 
> Thanks. I was interested in whether or not if this object is owned by this
> class. Maybe:
> 
> // Not owned by this class.
> 
> was sufficient.

Done.

[mtomasz, 2014-03-27 04:43:38]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-03-27 04:44:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/192573002/270001

[commit-bot: I haz the power, 2014-03-27 07:56:35]

Change committed as 259820