[fsp] Create a RequestManager per a ProvidedFileSystem instance.

[fsp] Create a RequestManager per a ProvidedFileSystem instance.

Before, there was only one single request manager per profile, and all of the
file systems were sharing it. However, it is better to create it per file
system for several reasons.

First of all, there is no need to share it, since it is lightweight, and no
data is shared between different file systems.

Secondly, it is a part of implementation of the ProvidedFileSystem. The testing
FakeProvidedFileSystem does not use a request manager at all, so there is no
need to create it for unit tests.

Also, it greatly simplifies the request manager, since there is no need to
verify the extension id and file system id every time the request manager is
used.

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

[mtomasz, 2014-04-16 03:50:29]

@hashimoto: PTAL. This is mostly a mechanical change. Thanks.

[mtomasz, 2014-04-16 03:50:57]

On 2014/04/16 03:50:29, mtomasz wrote:
> @hashimoto: PTAL. This is mostly a mechanical change. Thanks.

It depends on 210803003 (CQ).

[hashimoto, 2014-04-16 04:14:12]

lgtm

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:37: //
TODO(mtomasz): Create a request manager in ProvidedFileSystem, since it is
This comment is obsolete?

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:8: #include
"base/files/file_path.h"
nit: Is this needed?

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:34:
// Returns a request manager for the file system if exists, or NULL.
Since the real implementation always returns a non-NULL pointer, how about
removing the "if exists, or NULL" part in sake of less ambiguity?
IIUC GetRequestManager() should not be called for the fake implementation, so
that we can put NOTREACHED() in the fake's method definition.

Another solution would be to add FullfillRequest() and RejectRequest() to this
class which simply calls corresponding methods of RequestManager.
In the fake, you can leave the method body empty.

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:43: Service::~Service()
{}
Does this mean file_system_map_ will leak if Shutdown() was not called
correctly?
How about deleting values here too or having DCHECK(file_system_map_.empty())?

[mtomasz, 2014-04-16 05:08:10]

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:37: //
TODO(mtomasz): Create a request manager in ProvidedFileSystem, since it is
On 2014/04/16 04:14:13, hashimoto wrote:
> This comment is obsolete?

Done.

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:8: #include
"base/files/file_path.h"
On 2014/04/16 04:14:13, hashimoto wrote:
> nit: Is this needed?

Done.

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:34:
// Returns a request manager for the file system if exists, or NULL.
On 2014/04/16 04:14:13, hashimoto wrote:
> Since the real implementation always returns a non-NULL pointer, how about
> removing the "if exists, or NULL" part in sake of less ambiguity?
> IIUC GetRequestManager() should not be called for the fake implementation, so
> that we can put NOTREACHED() in the fake's method definition.
> 
> Another solution would be to add FullfillRequest() and RejectRequest() to this
> class which simply calls corresponding methods of RequestManager.
> In the fake, you can leave the method body empty.

NOTREACHED() sounds good to me. Done.

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/239993002/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:43: Service::~Service()
{}
On 2014/04/16 04:14:13, hashimoto wrote:
> Does this mean file_system_map_ will leak if Shutdown() was not called
> correctly?
> How about deleting values here too or having DCHECK(file_system_map_.empty())?

That's a good point. In tests, it would leak. I think we can safely move it to
the destructor. It calls ~RequestManager, which aborts requests. Aborting only
calls the error callback, which should be safe. Done.

[hashimoto, 2014-04-16 05:14:10]

lgtm

[mtomasz, 2014-04-16 05:43:03]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-16 05:43:21]

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

[commit-bot: I haz the power, 2014-04-16 06:16:16]

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

[commit-bot: I haz the power, 2014-04-16 06:16:16]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_gn_rel

[mtomasz, 2014-04-16 06:17:56]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-16 06:19:28]

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

[commit-bot: I haz the power, 2014-04-16 13:03:38]

Change committed as 264191