[fsp] Decouple file_service_provider::Service.

[fsp] Decouple file_service_provider::Service.

This patch splits Service into thinner Service and ProvidedFileSystem. Also
now, provided file system information instances are renamed to
ProvidedFileSystemInfo.

Such separation makes it easy to test provided file systems independently, from
the service. Also, after the introduced interface will allow to create a
FakeProvidedFileSystem, which will be used to test upcoming AsyncFileUtil
without need of using an extension.

TEST=New unit_tests:*FileSystemProviderProvidedFileSystemTest* and previous:
     *FileSystemProvider* (unit_tests + browser_tests).
BUG=248427
R=hashimoto@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264106

[mtomasz, 2014-03-25 08:54:21]

This CL depends on: 194693002.

[mtomasz, 2014-03-25 08:56:32]

@hashimoto: PTAL. If you have any questions, let me know. This change is almost
entirely mechanical (refactoring).

[hashimoto, 2014-03-26 03:03:00]

Please rebase onto the updated patch set

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:28: //
since unmount reqiest does not provide arguments.
nit: s/reqiest/request/?

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:13: #include
"chrome/browser/chromeos/file_system_provider/request_manager.h"
nit: No need to include this? Forward declaration isn't enough?

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:38:
extensions::EventRouter* event_router_;  // Not owned.
nit: No need to have "Not owned" annotation.
If it's owned, it should be scoped_ptr anyways.
(http://www.chromium.org/developers/smart-pointer-guidelines)

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:25:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
nit: "static" is not needed.
nit: use char[]

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:43:
std::string extension_id() const { return extension_id_; }
nit: const std::string&?

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:117:
ASSERT_EQ(2u, event_args->GetSize());
nit: EXPECT instead of ASSERT?

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:119:
event_args->GetInteger(0, &file_system_id);
EXPECT_TRUE?

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:123:
int request_id;
nit: Please don't leave this variable uninitialized.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:124:
event_args->GetInteger(0, &request_id);
Why are you calling GetInteger for the index=0 twice?

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:38: int
CreateRequest(const std::string extension_id,
nit: const std::string&

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:61: const
ProvidedFileSystemInfo& file_system_info,
Why don't these methods take (extension_id, file_system_id) pair instead of
file_system_info?

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:163: const
ProvidedFileSystemInfo file_system_info =
nit: const ProvidedFileSystemInfo&?

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:195:
file_system_it->second->GetFileSystemInfo().extension_id() !=
Why do you need to check extension ID?
There can be multiple filesystems with the same filesystem ID?

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:245:
FOR_EACH_OBSERVER(Observer,
nit: Here what you want is to notify observers about errors which occur before
the request is sent to the provider extension, right?
Then, I think it would be easier to understand by reversing the logic like:
if (error != base::File::FILE_OK) {
  FOR_EACH_OBSERVER(...)
}
WDYT?

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.h:89: // Values owned by
above ScopedVector.
Managing two containers is difficult.
How about owning pointers with the map?
You can use STLDeleteValues to delete all pointers in the dtor.
(https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=S...)

[mtomasz, 2014-03-26 05:45:13]

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:28: //
since unmount reqiest does not provide arguments.
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: s/reqiest/request/?

Done.

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:13: #include
"chrome/browser/chromeos/file_system_provider/request_manager.h"
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: No need to include this? Forward declaration isn't enough?

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:38:
extensions::EventRouter* event_router_;  // Not owned.
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: No need to have "Not owned" annotation.
> If it's owned, it should be scoped_ptr anyways.
> (http://www.chromium.org/developers/smart-pointer-guidelines)

Done.

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:25:
static const std::string kExtensionId = "mbflcebpggnecokmikipoihdbecnjfoj";
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: "static" is not needed.
> nit: use char[]

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:43:
std::string extension_id() const { return extension_id_; }
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: const std::string&?

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:117:
ASSERT_EQ(2u, event_args->GetSize());
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: EXPECT instead of ASSERT?

Good point. Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:119:
event_args->GetInteger(0, &file_system_id);
On 2014/03/26 03:03:01, hashimoto wrote:
> EXPECT_TRUE?

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:123:
int request_id;
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: Please don't leave this variable uninitialized.

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:124:
event_args->GetInteger(0, &request_id);
On 2014/03/26 03:03:01, hashimoto wrote:
> Why are you calling GetInteger for the index=0 twice?

Should be index=1 for the second one. Coincidentally, value for both was same. I
just changed kFilesystemId to a different value that request_id. Done.

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:38: int
CreateRequest(const std::string extension_id,
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: const std::string&

Oops. Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:61: const
ProvidedFileSystemInfo& file_system_info,
On 2014/03/26 03:03:01, hashimoto wrote:
> Why don't these methods take (extension_id, file_system_id) pair instead of
> file_system_info?

Just to match the observer's interface.

As for CreateRequest, FulfillRequest and RejectRequest, they accept
(extension_id, file_system_id), because the RequestManager is called from
fileBrowserPrivateAPI, which doesn't have a reference to ProvidedFileSystemInfo.
We could get it from the service, but it would just add extra overhead and
dependency.

Note, that per #30, RequestManager is going to be created per file system in the
future. As a result:
1. It will stop observing the service.
2. extension_id and file_system_id arguments will be removed from all above
methods.

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:163: const
ProvidedFileSystemInfo file_system_info =
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: const ProvidedFileSystemInfo&?

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:163: const
ProvidedFileSystemInfo file_system_info =
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: const ProvidedFileSystemInfo&?

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:195:
file_system_it->second->GetFileSystemInfo().extension_id() !=
On 2014/03/26 03:03:01, hashimoto wrote:
> Why do you need to check extension ID?
> There can be multiple filesystems with the same filesystem ID?

You're right, this is wrong. I was comparing extension of the caller with the
provider extension. There should be no such check. It surprisingly passed tests,
because the provider is implemented in the same extension as the unmount caller.
I have plans to add tests with separated provider extensions from testing
extensions. 

Done.

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:245:
FOR_EACH_OBSERVER(Observer,
On 2014/03/26 03:03:01, hashimoto wrote:
> nit: Here what you want is to notify observers about errors which occur before
> the request is sent to the provider extension, right?
> Then, I think it would be easier to understand by reversing the logic like:
> if (error != base::File::FILE_OK) {
>   FOR_EACH_OBSERVER(...)
> }
> WDYT?

Almost. I want to notify observers about failure in unmounting after provider
extension calls onError, or if ProvidedFileSystem failed to create a request. I
agree with the cleanup. Done.

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.h:89: // Values owned by
above ScopedVector.
On 2014/03/26 03:03:01, hashimoto wrote:
> Managing two containers is difficult.
> How about owning pointers with the map?
> You can use STLDeleteValues to delete all pointers in the dtor.
>
(https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=S...)

Good point. Done.

[hashimoto, 2014-03-26 06:53:03]

lgtm

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

https://codereview.chromium.org/210803003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/request_manager.h:61: const
ProvidedFileSystemInfo& file_system_info,
On 2014/03/26 05:45:13, mtomasz OOO until 21.03 wrote:
> On 2014/03/26 03:03:01, hashimoto wrote:
> > Why don't these methods take (extension_id, file_system_id) pair instead of
> > file_system_info?
> 
> Just to match the observer's interface.
> 
> As for CreateRequest, FulfillRequest and RejectRequest, they accept
> (extension_id, file_system_id), because the RequestManager is called from
> fileBrowserPrivateAPI, which doesn't have a reference to
ProvidedFileSystemInfo.
> We could get it from the service, but it would just add extra overhead and
> dependency.
> 
> Note, that per #30, RequestManager is going to be created per file system in
the
> future. As a result:
> 1. It will stop observing the service.
> 2. extension_id and file_system_id arguments will be removed from all above
> methods.

Thank you for clearly explaining about your plan.
Makes sense.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:174:
SendResponse(true);
nit: Just returning false isn't enough?

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:125:
EXPECT_TRUE(event_args->GetInteger(kExpectedRequestId, &request_id));
1 instead of kExpectedRequestId?

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:138:
ASSERT_TRUE(reply_result);
nit: I guess this can be EXPECT.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:175:
ASSERT_TRUE(reply_result);
ditto.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.cc:106: it++;
nit: ++it is better.

[mtomasz, 2014-03-26 08:47:31]

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:174:
SendResponse(true);
On 2014/03/26 06:53:04, hashimoto wrote:
> nit: Just returning false isn't enough?

If we return false, then we would signal an error. However, this is not an
error, we want to finish with a success.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:125:
EXPECT_TRUE(event_args->GetInteger(kExpectedRequestId, &request_id));
On 2014/03/26 06:53:04, hashimoto wrote:
> 1 instead of kExpectedRequestId?

Woops! Done.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:138:
ASSERT_TRUE(reply_result);
On 2014/03/26 06:53:04, hashimoto wrote:
> nit: I guess this can be EXPECT.

Done.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:175:
ASSERT_TRUE(reply_result);
On 2014/03/26 06:53:04, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.cc:106: it++;
On 2014/03/26 06:53:04, hashimoto wrote:
> nit: ++it is better.

Done.

[hashimoto, 2014-03-26 09:04:26]

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:174:
SendResponse(true);
On 2014/03/26 08:47:31, mtomasz OOO until 21.03 wrote:
> On 2014/03/26 06:53:04, hashimoto wrote:
> > nit: Just returning false isn't enough?
> 
> If we return false, then we would signal an error. However, this is not an
> error, we want to finish with a success.

oops, I was confusing this with a SyncExtensionFunction's behavior to return the
result without manually calling SendResponse().

BTW, why all these functions are implemented as async functions?
It seems all of them can be sync functions. (=SyncExtensionFunction or
ChromeSyncExtensionFunction)

[mtomasz, 2014-03-26 10:53:57]

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/210803003/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:174:
SendResponse(true);
On 2014/03/26 09:04:26, hashimoto wrote:
> On 2014/03/26 08:47:31, mtomasz OOO until 21.03 wrote:
> > On 2014/03/26 06:53:04, hashimoto wrote:
> > > nit: Just returning false isn't enough?
> > 
> > If we return false, then we would signal an error. However, this is not an
> > error, we want to finish with a success.
> 
> oops, I was confusing this with a SyncExtensionFunction's behavior to return
the
> result without manually calling SendResponse().
> 
> BTW, why all these functions are implemented as async functions?
> It seems all of them can be sync functions. (=SyncExtensionFunction or
> ChromeSyncExtensionFunction)

Good catch. Converted to SyncExtensionFunctions! Done.

[hashimoto, 2014-03-27 09:21:17]

Is the current patch set based on the newest patch set of
https://codereview.chromium.org/194693002/?

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

https://codereview.chromium.org/210803003/diff/280019/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:191:
"/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-testing_profile-hash",
This EXPECT looks fragile for changes in UserManager/Profile implementation.
Could you make this EXPECT more robust? (e.g. instead of writing this cryptic
hash down directly in this test, get the hash from the profile or somewhere
appropriate)

[mtomasz, 2014-03-28 09:42:56]

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

https://codereview.chromium.org/210803003/diff/280019/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:191:
"/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-testing_profile-hash",
On 2014/03/27 09:21:18, hashimoto wrote:
> This EXPECT looks fragile for changes in UserManager/Profile implementation.
> Could you make this EXPECT more robust? (e.g. instead of writing this cryptic
> hash down directly in this test, get the hash from the profile or somewhere
> appropriate)

Per offline discussion. SGTM. I'll prepare a fix.

[mtomasz, 2014-03-31 01:45:47]

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

https://codereview.chromium.org/210803003/diff/280019/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:191:
"/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-testing_profile-hash",
On 2014/03/28 09:42:57, mtomasz wrote:
> On 2014/03/27 09:21:18, hashimoto wrote:
> > This EXPECT looks fragile for changes in UserManager/Profile implementation.
> > Could you make this EXPECT more robust? (e.g. instead of writing this
cryptic
> > hash down directly in this test, get the hash from the profile or somewhere
> > appropriate)
> 
> Per offline discussion. SGTM. I'll prepare a fix.

Since it is not critical, how about fixing it in a separate patch? I'd like to
avoid crazy rebasing. In an upcoming patch GetMountPointPath() is moved from 
the anonymous namespace to a separate util.cc, so it would be much easier to
refactor it then, and get rid of hard-coded mount paths. WDYT?

[mtomasz, 2014-04-09 00:16:31]

@hashimoto: I've moved GetMountPointPath to util.cc and simplified tests. PTAL.
Thanks.

[hashimoto, 2014-04-09 06:18:22]

The number of files added by the newest patch set doesn't match the change in
GYP files.
Please fix it.

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

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:9: #include
"base/strings/string_number_conversions.h"
nit: No need to include this?

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

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:170:
ASSERT_EQ(kMaxFileSystems, file_system_info_list.size());
One of l. 164 or l. 170 can be removed?

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/util.h (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/util.h:6: #define
CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_UTIL_H_
I'm a bit worried about the file name "util.h" because it's too generic so that
this file will soon be as messy as a trash bin.
How about using a slightly clearer name like "path_util.h" or "mount_util.h"?

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/util_unittest.cc (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/util_unittest.cc:38:
GetMountPointPath(profile_.get(), "mbflcebpggnecokmikipoihdbecnjfoj", 1);
Please make the string "mbflce..." constant to make it clear that the string is
an extension ID.
The same goes for the number "1".

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:377:
'browser/chromeos/file_system_provider/provided_file_system_interface.h',
I don't see these files in the newest patch set.

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:726:
'browser/chromeos/file_system_provider/provided_file_system_unittest.cc',
I don't see this file in the newest patch set.

[mtomasz, 2014-04-09 19:51:14]

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

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:9: #include
"base/strings/string_number_conversions.h"
On 2014/04/09 06:18:23, hashimoto wrote:
> nit: No need to include this?

Done.

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

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:170:
ASSERT_EQ(kMaxFileSystems, file_system_info_list.size());
On 2014/04/09 06:18:23, hashimoto wrote:
> One of l. 164 or l. 170 can be removed?

Good catch. Done.

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/util.h (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/util.h:6: #define
CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_UTIL_H_
On 2014/04/09 06:18:23, hashimoto wrote:
> I'm a bit worried about the file name "util.h" because it's too generic so
that
> this file will soon be as messy as a trash bin.
> How about using a slightly clearer name like "path_util.h" or "mount_util.h"?

Done.

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/util_unittest.cc (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/util_unittest.cc:38:
GetMountPointPath(profile_.get(), "mbflcebpggnecokmikipoihdbecnjfoj", 1);
On 2014/04/09 06:18:23, hashimoto wrote:
> Please make the string "mbflce..." constant to make it clear that the string
is
> an extension ID.
> The same goes for the number "1".

Done.

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:377:
'browser/chromeos/file_system_provider/provided_file_system_interface.h',
On 2014/04/09 06:18:23, hashimoto wrote:
> I don't see these files in the newest patch set.

Done.

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:726:
'browser/chromeos/file_system_provider/provided_file_system_unittest.cc',
On 2014/04/09 06:18:23, hashimoto wrote:
> I don't see this file in the newest patch set.

Done.

[mtomasz, 2014-04-10 05:20:23]

On 2014/04/09 19:51:14, mtomasz wrote:
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> File chrome/browser/chromeos/file_system_provider/service.cc (right):
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> chrome/browser/chromeos/file_system_provider/service.cc:9: #include
> "base/strings/string_number_conversions.h"
> On 2014/04/09 06:18:23, hashimoto wrote:
> > nit: No need to include this?
> 
> Done.
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> chrome/browser/chromeos/file_system_provider/service_unittest.cc:170:
> ASSERT_EQ(kMaxFileSystems, file_system_info_list.size());
> On 2014/04/09 06:18:23, hashimoto wrote:
> > One of l. 164 or l. 170 can be removed?
> 
> Good catch. Done.
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> File chrome/browser/chromeos/file_system_provider/util.h (right):
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> chrome/browser/chromeos/file_system_provider/util.h:6: #define
> CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_UTIL_H_
> On 2014/04/09 06:18:23, hashimoto wrote:
> > I'm a bit worried about the file name "util.h" because it's too generic so
> that
> > this file will soon be as messy as a trash bin.
> > How about using a slightly clearer name like "path_util.h" or
"mount_util.h"?
> 
> Done.
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> File chrome/browser/chromeos/file_system_provider/util_unittest.cc (right):
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/browser/chromeos...
> chrome/browser/chromeos/file_system_provider/util_unittest.cc:38:
> GetMountPointPath(profile_.get(), "mbflcebpggnecokmikipoihdbecnjfoj", 1);
> On 2014/04/09 06:18:23, hashimoto wrote:
> > Please make the string "mbflce..." constant to make it clear that the string
> is
> > an extension ID.
> > The same goes for the number "1".
> 
> Done.
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_browser_c...
> File chrome/chrome_browser_chromeos.gypi (right):
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_browser_c...
> chrome/chrome_browser_chromeos.gypi:377:
> 'browser/chromeos/file_system_provider/provided_file_system_interface.h',
> On 2014/04/09 06:18:23, hashimoto wrote:
> > I don't see these files in the newest patch set.
> 
> Done.
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_tests_uni...
> File chrome/chrome_tests_unit.gypi (right):
> 
>
https://codereview.chromium.org/210803003/diff/410001/chrome/chrome_tests_uni...
> chrome/chrome_tests_unit.gypi:726:
> 'browser/chromeos/file_system_provider/provided_file_system_unittest.cc',
> On 2014/04/09 06:18:23, hashimoto wrote:
> > I don't see this file in the newest patch set.
> 
> Done.

Please hold on with reviewing this. I'm going to fix the tests tomorrow, which
will change the code significantly.

[mtomasz, 2014-04-10 21:22:55]

@hashimoto: I've fixed the tests. PTAL. Thanks.

[hashimoto, 2014-04-11 06:01:50]

When you made a large change to the patch set, could you describe what you
changed?
It's very difficult for me to follow what happened to this change because of the
files missing in the PS #7, multiples rebases, and the hugeness of this patch
(it's almost reaching 1000 lines!).

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:44: //
of a profile wide. As a result, this dependency will not be needed in a
nit: What does "this dependency" mean?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:42: :
public ProvidedFileSystemFactoryInterface {
Do we need an interface for this?
Can't this be replaced with a callback?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:84:
Profile* profile,
Why doesn't this method take event_router instead of profile?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:49: virtual
~ProvidedFileSystemFactory();
Are these empty ctor and dtor needed?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.cc:111:
RequestManager::Request::Request() {}
Please don't leave file_system_id uninitialized.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.h:38: int
CreateRequest(const std::string& extension_id,
Perhaps I might have asked this previously, but just in case.
Why do we need to pass extension_id?
There can be multiple filesystems with the same file_system_id, but provided by
different extensions?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:18: #include
"chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
nit: Is this include needed?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:93: Service(Profile*
profile,
Why don't you simply replace the 1-argument version ctor with this?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:94:
ProvidedFileSystemFactoryInterface* file_system_factory);
You should use scoped_ptr if this method takes ownership of the pointer.

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

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:180:
base::FilePath expected_mount_path =
nit: Why this is declared here?
Our style guide says the declaration should be "as close to the first use as
possible".
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...)

https://codereview.chromium.org/210803003/diff/470001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:373:
'browser/chromeos/file_system_provider/fake_provided_file_system.h',
No need to include this class in the product binary and increase the binary
size.
This should be moved to a test binary.

[mtomasz, 2014-04-11 07:11:55]

Sorry for the big patch. As I mentioned in
https://codereview.chromium.org/210803003/#msg11
I wanted to do further updates in separate patches.

To sum up, the recent patch introduces a ProvidedFileSystemFactory. The reason
for that is that I want to test as much as possible as unit tests for speed
(browser tests are slow). However, event router is not available in unit tests.
Injecting a FakeProvidedFileSystemFactory cuts off the part depending on the
event router.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:42: :
public ProvidedFileSystemFactoryInterface {
On 2014/04/11 06:01:50, hashimoto wrote:
> Do we need an interface for this?
> Can't this be replaced with a callback?

I just made it for consistency with other factories. In contrast to this common
pattern, a factory callback sounds quite awkward to me. WDYT?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:84:
Profile* profile,
On 2014/04/11 06:01:50, hashimoto wrote:
> Why doesn't this method take event_router instead of profile?

I tried it, but it would be difficult to test. Provided_file_system_unittest.cc
creates an instance with a fake event router.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:49: virtual
~ProvidedFileSystemFactory();
On 2014/04/11 06:01:50, hashimoto wrote:
> Are these empty ctor and dtor needed?

dtor yes, ctor seems redundant. I'll remove tmr.
Per:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/n5IAT9wywPo

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.h:38: int
CreateRequest(const std::string& extension_id,
On 2014/04/11 06:01:50, hashimoto wrote:
> Perhaps I might have asked this previously, but just in case.
> Why do we need to pass extension_id?
> There can be multiple filesystems with the same file_system_id, but provided
by
> different extensions?

Basically, I wanted to avoid situation, when an malicious or incorrectly written
providing extension uses wront request id, and as a result accesses a request of
another extension. By passing an extension id, we can check, if the source
extension id matches the extension id used for the request.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:93: Service(Profile*
profile,
On 2014/04/11 06:01:50, hashimoto wrote:
> Why don't you simply replace the 1-argument version ctor with this?

We would have to pass the factory in many places, eg. in the VolumeManager, and
in any browser test (not unit test). I think that since the factory is default,
it is nice, if it is created inside Service, not outside. WDYT?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:94:
ProvidedFileSystemFactoryInterface* file_system_factory);
On 2014/04/11 06:01:50, hashimoto wrote:
> You should use scoped_ptr if this method takes ownership of the pointer.

Good idea.

https://codereview.chromium.org/210803003/diff/470001/chrome/chrome_browser_c...
File chrome/chrome_browser_chromeos.gypi (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/chrome_browser_c...
chrome/chrome_browser_chromeos.gypi:373:
'browser/chromeos/file_system_provider/fake_provided_file_system.h',
On 2014/04/11 06:01:50, hashimoto wrote:
> No need to include this class in the product binary and increase the binary
> size.
> This should be moved to a test binary.

Very good idea.

[hashimoto, 2014-04-11 09:09:03]

Did you forget to upload a new patch set?

[mtomasz, 2014-04-11 19:54:12]

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:44: //
of a profile wide. As a result, this dependency will not be needed in a
On 2014/04/11 06:01:50, hashimoto wrote:
> nit: What does "this dependency" mean?

Dependency of the fake factory on the request manager. If the request manager
was created in ProvidedFileSystem, then it wouldn't have to be passed to
ProvidedFileSystemFactoryInterface::Create(). Since the request manager is used
only by ProvidedFileSystem, not by FakeProvidedFileSystem, the second one
shouldn't depend on it. I simplified the comment. Done.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:49: virtual
~ProvidedFileSystemFactory();
On 2014/04/11 07:11:56, mtomasz wrote:
> On 2014/04/11 06:01:50, hashimoto wrote:
> > Are these empty ctor and dtor needed?
> 
> dtor yes, ctor seems redundant. I'll remove tmr.
> Per:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/n5IAT9wywPo

Opposite way. Dtor is redundant. Ctor is necessary. Otherwise, we would not be
able to create the factory (because of DISALLOW_COPY_ASSIGN prevents creating
the default constructor).

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:18: #include
"chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
On 2014/04/11 06:01:50, hashimoto wrote:
> nit: Is this include needed?

Done.

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

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:180:
base::FilePath expected_mount_path =
On 2014/04/11 06:01:50, hashimoto wrote:
> nit: Why this is declared here?
> Our style guide says the declaration should be "as close to the first use as
> possible".
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...)

Done.

[hashimoto, 2014-04-14 10:43:41]

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:42: :
public ProvidedFileSystemFactoryInterface {
On 2014/04/11 07:11:56, mtomasz wrote:
> On 2014/04/11 06:01:50, hashimoto wrote:
> > Do we need an interface for this?
> > Can't this be replaced with a callback?
> 
> I just made it for consistency with other factories. In contrast to this
common
> pattern, a factory callback sounds quite awkward to me. WDYT?
How common do you think this pattern is?
There are many files named *_factory.h in chrome/browser and many of them are
just using a static function to create instances.
BrowserContextKeyedService is even using a raw function pointer.
Why did you choose this pattern, which requires a derived class for one "new"
sentence, and prefer it to callbacks, which requires only one function?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:84:
Profile* profile,
On 2014/04/11 07:11:56, mtomasz wrote:
> On 2014/04/11 06:01:50, hashimoto wrote:
> > Why doesn't this method take event_router instead of profile?
> 
> I tried it, but it would be difficult to test.
Provided_file_system_unittest.cc
> creates an instance with a fake event router.

Sorry, I still don't understand why changing this method's argument can cause
problems in provided_file_system_unittest.cc
Can you elaborate on this?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.cc (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.cc:111:
RequestManager::Request::Request() {}
On 2014/04/11 06:01:50, hashimoto wrote:
> Please don't leave file_system_id uninitialized.

ping.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/request_manager.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/request_manager.h:38: int
CreateRequest(const std::string& extension_id,
On 2014/04/11 07:11:56, mtomasz wrote:
> On 2014/04/11 06:01:50, hashimoto wrote:
> > Perhaps I might have asked this previously, but just in case.
> > Why do we need to pass extension_id?
> > There can be multiple filesystems with the same file_system_id, but provided
> by
> > different extensions?
> 
> Basically, I wanted to avoid situation, when an malicious or incorrectly
written
> providing extension uses wront request id, and as a result accesses a request
of
> another extension. By passing an extension id, we can check, if the source
> extension id matches the extension id used for the request.
Makes sense.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:93: Service(Profile*
profile,
On 2014/04/11 07:11:56, mtomasz wrote:
> On 2014/04/11 06:01:50, hashimoto wrote:
> > Why don't you simply replace the 1-argument version ctor with this?
> 
> We would have to pass the factory in many places, eg. in the VolumeManager,
and
> in any browser test (not unit test). I think that since the factory is
default,
> it is nice, if it is created inside Service, not outside. WDYT?

Do you think this merit is worth for the cost required to maintain two versions
of constructors?

Also, while this trick makes writing browser_tests handy (I guess we can ignore
production code because it's only one place), it makes unit_tests verbose.

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:24:
callback.Run(base::File::FILE_OK);
How about doing something like
MessageLoopProxy::current()->PostTask(base::Bind(callback,
base::File::FILE_OK))?
Without this, execution order changes in the production and tests.

Example: The code below results in calling function like A()->B()->C() with the
production code, but A()->C()->B() in tests.

A();
fake_provided_file_system->RequestUnmount(Bind(&C))
B();

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:25:
FakeProvidedFileSystem();
Why is this ctor there?

[mtomasz, 2014-04-15 02:46:40]

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:42: :
public ProvidedFileSystemFactoryInterface {
On 2014/04/14 10:43:42, hashimoto wrote:
> On 2014/04/11 07:11:56, mtomasz wrote:
> > On 2014/04/11 06:01:50, hashimoto wrote:
> > > Do we need an interface for this?
> > > Can't this be replaced with a callback?
> > 
> > I just made it for consistency with other factories. In contrast to this
> common
> > pattern, a factory callback sounds quite awkward to me. WDYT?
> How common do you think this pattern is?
> There are many files named *_factory.h in chrome/browser and many of them are
> just using a static function to create instances.
> BrowserContextKeyedService is even using a raw function pointer.
> Why did you choose this pattern, which requires a derived class for one "new"
> sentence, and prefer it to callbacks, which requires only one function?

I just took a look, and there is a lot of single-method factory classes in
Chromium code base.

I believe this is just a matter of preference. Having a class interface for a
factory is just a super simple and clean design. 
Callbacks are IMHO always tricky (not friendly for static analysis, so not
friendly for cs.chromium.org, produce unfriendly compiling 
errors, difficult to understand by external developers, etc). It is also
difficult to maintain them, since we can't easily grep to find which
functions/methods are "implementing" a callback for some typedefed callback.

A class interface is more verbose, but I don't find a big issue in having a
verbose code. It is also self-documenting.

As for passing pointer to BrowserContextKeyedService, this sounds like a very
bug-prone design.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:84:
Profile* profile,
On 2014/04/14 10:43:42, hashimoto wrote:
> On 2014/04/11 07:11:56, mtomasz wrote:
> > On 2014/04/11 06:01:50, hashimoto wrote:
> > > Why doesn't this method take event_router instead of profile?
> > 
> > I tried it, but it would be difficult to test.
> Provided_file_system_unittest.cc
> > creates an instance with a fake event router.
> 
> Sorry, I still don't understand why changing this method's argument can cause
> problems in provided_file_system_unittest.cc
> Can you elaborate on this?

Yep, I must have been tired writing that comment :p.

The problem is that extensions::ExtensionSystem::Get(profile)->event_router()
returns NULL for a testing profile. So if we pass TestingProfile in unit tests,
ProvidedFileSystem will fail because the event router is NULL. Also,
ProvidedFileSystem doesn't use Profile, so there seem to be no need to add this
extra dependency. WDYT?

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:93: Service(Profile*
profile,
On 2014/04/14 10:43:42, hashimoto wrote:
> On 2014/04/11 07:11:56, mtomasz wrote:
> > On 2014/04/11 06:01:50, hashimoto wrote:
> > > Why don't you simply replace the 1-argument version ctor with this?
> > 
> > We would have to pass the factory in many places, eg. in the VolumeManager,
> and
> > in any browser test (not unit test). I think that since the factory is
> default,
> > it is nice, if it is created inside Service, not outside. WDYT?
> 
> Do you think this merit is worth for the cost required to maintain two
versions
> of constructors?
> 
> Also, while this trick makes writing browser_tests handy (I guess we can
ignore
> production code because it's only one place), it makes unit_tests verbose.

I think that having multiple constructors is useful if it makes it easier to
instantiate the class later. Why not taking an advantage of that?

If we remove this second constructor, then unit tests *will not* be less
verbose, because if I understand correctly, we are talking about removing the
Service(Profile) constructor not Service(Profile, FactoryClass/FactoryClosure).
Do I understand correctly?

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:24:
callback.Run(base::File::FILE_OK);
On 2014/04/14 10:43:42, hashimoto wrote:
> How about doing something like
> MessageLoopProxy::current()->PostTask(base::Bind(callback,
> base::File::FILE_OK))?
> Without this, execution order changes in the production and tests.
> 
> Example: The code below results in calling function like A()->B()->C() with
the
> production code, but A()->C()->B() in tests.
> 
> A();
> fake_provided_file_system->RequestUnmount(Bind(&C))
> B();

Hm. That's an interesting idea. However, calling callbacks synchronously makes
unit testing easier, since we don't need to call
base::MessageLoop::current()->Run() before validating. Also, since there are no
requirements in ProvidedFileSystemInterface on way of calling callbacks, then by
calling them synchronously, unit tests may catch some implementation errors (eg.
relying on order of calls). Note, that the real ProvidedFileSystem from the
production code is tested by api tests (browser test).

Anyway, I don't have hard feelings here. If you prefer PostTask, I'll do a
PostTask and add MessageLoop::current()->Run() to unit tests. Let me know.

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:25:
FakeProvidedFileSystem();
On 2014/04/14 10:43:42, hashimoto wrote:
> Why is this ctor there?

Not necessary. Done.

[hashimoto, 2014-04-15 07:22:44]

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:42: :
public ProvidedFileSystemFactoryInterface {
On 2014/04/15 02:46:40, mtomasz wrote:
> On 2014/04/14 10:43:42, hashimoto wrote:
> > On 2014/04/11 07:11:56, mtomasz wrote:
> > > On 2014/04/11 06:01:50, hashimoto wrote:
> > > > Do we need an interface for this?
> > > > Can't this be replaced with a callback?
> > > 
> > > I just made it for consistency with other factories. In contrast to this
> > common
> > > pattern, a factory callback sounds quite awkward to me. WDYT?
> > How common do you think this pattern is?
> > There are many files named *_factory.h in chrome/browser and many of them
are
> > just using a static function to create instances.
> > BrowserContextKeyedService is even using a raw function pointer.
> > Why did you choose this pattern, which requires a derived class for one
"new"
> > sentence, and prefer it to callbacks, which requires only one function?
> 
> I just took a look, and there is a lot of single-method factory classes in
> Chromium code base.
I know, as I said, what I wanted to know is the rationale behind your design
decision.

For example, there is a factory interface net::URLRequestJobFactory in
net/url_request.
This factory interface serves to dynamically decide which class to instantiate
for each URLRequest.
OTOH, the factory interface here is only beneficial for tests.
If you don't want to instantiate many kinds of classes dynamically, there are
many other handy ways. (e.g. callbacks, adding a factory setter method for
tests)

Since you introduced this interface suddenly midst the review to fix the broken
tests and ended up having duplicated ctors,
I got an implication that you didn't spend much time to design this code so I
wanted to suggest alternative solutions.
Though, I don't have a strong feeling about callbacks.
It should be OK as long as there is no duplicated ctors.

> 
> I believe this is just a matter of preference. Having a class interface for a
> factory is just a super simple and clean design. 
> Callbacks are IMHO always tricky (not friendly for static analysis, so not
> friendly for http://cs.chromium.org, produce unfriendly compiling 
> errors, difficult to understand by external developers, etc). It is also
base::Callback is one of the most fundamental classes of Chromium.
Every Chromium developer is expected to be familiar with it.
> difficult to maintain them, since we can't easily grep to find which
> functions/methods are "implementing" a callback for some typedefed callback.
You can use the code search for callbacks too.
When changing the signature of the callback, the compiler will tell you which
code you should fix.
> 
> A class interface is more verbose, but I don't find a big issue in having a
> verbose code. It is also self-documenting.
> 
> As for passing pointer to BrowserContextKeyedService, this sounds like a very
> bug-prone design.
I agree, this is the reason why DriveIntegrationService is using base::Callback
instead.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:84:
Profile* profile,
On 2014/04/15 02:46:40, mtomasz wrote:
> On 2014/04/14 10:43:42, hashimoto wrote:
> > On 2014/04/11 07:11:56, mtomasz wrote:
> > > On 2014/04/11 06:01:50, hashimoto wrote:
> > > > Why doesn't this method take event_router instead of profile?
> > > 
> > > I tried it, but it would be difficult to test.
> > Provided_file_system_unittest.cc
> > > creates an instance with a fake event router.
> > 
> > Sorry, I still don't understand why changing this method's argument can
cause
> > problems in provided_file_system_unittest.cc
> > Can you elaborate on this?
> 
> Yep, I must have been tired writing that comment :p.
> 
> The problem is that extensions::ExtensionSystem::Get(profile)->event_router()
> returns NULL for a testing profile. So if we pass TestingProfile in unit
tests,
> ProvidedFileSystem will fail because the event router is NULL. Also,
Sorry, I still don't understand why changing this method results in breaking
provided_file_system_unittest.cc.
AFAIK this method is only used by Service, but provided_file_system_unittest.cc
does not instantiate a Service.
> ProvidedFileSystem doesn't use Profile, so there seem to be no need to add
this
> extra dependency. WDYT?
Replacing |profile| here with an EventRouter does not mean adding an extra
dependency.
It's narrowing dependency from the entire Profile to EventRouter only.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:93: Service(Profile*
profile,
On 2014/04/15 02:46:40, mtomasz wrote:
> On 2014/04/14 10:43:42, hashimoto wrote:
> > On 2014/04/11 07:11:56, mtomasz wrote:
> > > On 2014/04/11 06:01:50, hashimoto wrote:
> > > > Why don't you simply replace the 1-argument version ctor with this?
> > > 
> > > We would have to pass the factory in many places, eg. in the
VolumeManager,
> > and
> > > in any browser test (not unit test). I think that since the factory is
> > default,
> > > it is nice, if it is created inside Service, not outside. WDYT?
> > 
> > Do you think this merit is worth for the cost required to maintain two
> versions
> > of constructors?
> > 
> > Also, while this trick makes writing browser_tests handy (I guess we can
> ignore
> > production code because it's only one place), it makes unit_tests verbose.
> 
> I think that having multiple constructors is useful if it makes it easier to
> instantiate the class later. Why not taking an advantage of that?
I don't think we should pay the cost for maintaining an additional version of
ctor used only by tests, just to save some key strokes when adding new tests.
> 
> If we remove this second constructor, then unit tests *will not* be less
> verbose, because if I understand correctly, we are talking about removing the
> Service(Profile) constructor not Service(Profile,
FactoryClass/FactoryClosure).
> Do I understand correctly?
If this ctor is public, you don't have to type "::CreateForTest".

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:24:
callback.Run(base::File::FILE_OK);
On 2014/04/15 02:46:40, mtomasz wrote:
> On 2014/04/14 10:43:42, hashimoto wrote:
> > How about doing something like
> > MessageLoopProxy::current()->PostTask(base::Bind(callback,
> > base::File::FILE_OK))?
> > Without this, execution order changes in the production and tests.
> > 
> > Example: The code below results in calling function like A()->B()->C() with
> the
> > production code, but A()->C()->B() in tests.
> > 
> > A();
> > fake_provided_file_system->RequestUnmount(Bind(&C))
> > B();
> 
> Hm. That's an interesting idea. However, calling callbacks synchronously makes
> unit testing easier, since we don't need to call
> base::MessageLoop::current()->Run() before validating. Also, since there are
no
Calling MessageLoop::Run is discouraged.
Please use base::RunLoop::Run() (or RunUntilIdle() which does not require
manually calling Quit().)
> requirements in ProvidedFileSystemInterface on way of calling callbacks, then
by
> calling them synchronously, unit tests may catch some implementation errors
(eg.
> relying on order of calls). Note, that the real ProvidedFileSystem from the
> production code is tested by api tests (browser test).
I guess browser_tests is going to have smaller test coverage and many parts like
corner-case error handling will be only covered by unit_tests.
This means that, if you or someone mistakenly add a corner-case handling code
which relies on callbacks called synchronously, automated tests cannot find it
automatically.
> 
> Anyway, I don't have hard feelings here. If you prefer PostTask, I'll do a
> PostTask and add MessageLoop::current()->Run() to unit tests. Let me know.
Please do it.
Adding additional base::RunLoop::RunUntilIdle() is much better than leaving bugs
undetected.

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager_unittest.cc (right):

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager_unittest.cc:152:
ProvidedFileSystemFactoryInterface>(
You can use make_scoped_ptr

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:167:
scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
It seems this variable is not needed.

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:44: //
|file_system_factory| instance.
scoped_ptr always takes the ownership.
No need to say it here.

[mtomasz, 2014-04-15 09:42:11]

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h:42: :
public ProvidedFileSystemFactoryInterface {
On 2014/04/15 07:22:45, hashimoto wrote:
> On 2014/04/15 02:46:40, mtomasz wrote:
> > On 2014/04/14 10:43:42, hashimoto wrote:
> > > On 2014/04/11 07:11:56, mtomasz wrote:
> > > > On 2014/04/11 06:01:50, hashimoto wrote:
> > > > > Do we need an interface for this?
> > > > > Can't this be replaced with a callback?
> > > > 
> > > > I just made it for consistency with other factories. In contrast to this
> > > common
> > > > pattern, a factory callback sounds quite awkward to me. WDYT?
> > > How common do you think this pattern is?
> > > There are many files named *_factory.h in chrome/browser and many of them
> are
> > > just using a static function to create instances.
> > > BrowserContextKeyedService is even using a raw function pointer.
> > > Why did you choose this pattern, which requires a derived class for one
> "new"
> > > sentence, and prefer it to callbacks, which requires only one function?
> > 
> > I just took a look, and there is a lot of single-method factory classes in
> > Chromium code base.
> I know, as I said, what I wanted to know is the rationale behind your design
> decision.
> 
> For example, there is a factory interface net::URLRequestJobFactory in
> net/url_request.
> This factory interface serves to dynamically decide which class to instantiate
> for each URLRequest.
> OTOH, the factory interface here is only beneficial for tests.
> If you don't want to instantiate many kinds of classes dynamically, there are
> many other handy ways. (e.g. callbacks, adding a factory setter method for
> tests)
> 
> Since you introduced this interface suddenly midst the review to fix the
broken
> tests and ended up having duplicated ctors,
> I got an implication that you didn't spend much time to design this code so I
> wanted to suggest alternative solutions.
> Though, I don't have a strong feeling about callbacks.
> It should be OK as long as there is no duplicated ctors.
> 
> > 
> > I believe this is just a matter of preference. Having a class interface for
a
> > factory is just a super simple and clean design. 
> > Callbacks are IMHO always tricky (not friendly for static analysis, so not
> > friendly for http://cs.chromium.org, produce unfriendly compiling 
> > errors, difficult to understand by external developers, etc). It is also
> base::Callback is one of the most fundamental classes of Chromium.
> Every Chromium developer is expected to be familiar with it.
> > difficult to maintain them, since we can't easily grep to find which
> > functions/methods are "implementing" a callback for some typedefed callback.
> You can use the code search for callbacks too.
> When changing the signature of the callback, the compiler will tell you which
> code you should fix.
> > 
> > A class interface is more verbose, but I don't find a big issue in having a
> > verbose code. It is also self-documenting.
> > 
> > As for passing pointer to BrowserContextKeyedService, this sounds like a
very
> > bug-prone design.
> I agree, this is the reason why DriveIntegrationService is using
base::Callback
> instead.
> 

I changed my mind. Let's go with SetFactoryForTest() and use a callback.
After removing the simplified constructor, this way is indeed cleaner. Done.

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/470001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:84:
Profile* profile,
On 2014/04/15 07:22:45, hashimoto wrote:
> On 2014/04/15 02:46:40, mtomasz wrote:
> > On 2014/04/14 10:43:42, hashimoto wrote:
> > > On 2014/04/11 07:11:56, mtomasz wrote:
> > > > On 2014/04/11 06:01:50, hashimoto wrote:
> > > > > Why doesn't this method take event_router instead of profile?
> > > > 
> > > > I tried it, but it would be difficult to test.
> > > Provided_file_system_unittest.cc
> > > > creates an instance with a fake event router.
> > > 
> > > Sorry, I still don't understand why changing this method's argument can
> cause
> > > problems in provided_file_system_unittest.cc
> > > Can you elaborate on this?
> > 
> > Yep, I must have been tired writing that comment :p.
> > 
> > The problem is that
extensions::ExtensionSystem::Get(profile)->event_router()
> > returns NULL for a testing profile. So if we pass TestingProfile in unit
> tests,
> > ProvidedFileSystem will fail because the event router is NULL. Also,
> Sorry, I still don't understand why changing this method results in breaking
> provided_file_system_unittest.cc.
> AFAIK this method is only used by Service, but
provided_file_system_unittest.cc
> does not instantiate a Service.
> > ProvidedFileSystem doesn't use Profile, so there seem to be no need to add
> this
> > extra dependency. WDYT?
> Replacing |profile| here with an EventRouter does not mean adding an extra
> dependency.
> It's narrowing dependency from the entire Profile to EventRouter only.

Per offline talk. I misread your first comment. Sorry! Of course we can change
the Profile with an EventRouter here.

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
(right):

https://codereview.chromium.org/210803003/diff/510001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc:24:
callback.Run(base::File::FILE_OK);
On 2014/04/15 07:22:45, hashimoto wrote:
> On 2014/04/15 02:46:40, mtomasz wrote:
> > On 2014/04/14 10:43:42, hashimoto wrote:
> > > How about doing something like
> > > MessageLoopProxy::current()->PostTask(base::Bind(callback,
> > > base::File::FILE_OK))?
> > > Without this, execution order changes in the production and tests.
> > > 
> > > Example: The code below results in calling function like A()->B()->C()
with
> > the
> > > production code, but A()->C()->B() in tests.
> > > 
> > > A();
> > > fake_provided_file_system->RequestUnmount(Bind(&C))
> > > B();
> > 
> > Hm. That's an interesting idea. However, calling callbacks synchronously
makes
> > unit testing easier, since we don't need to call
> > base::MessageLoop::current()->Run() before validating. Also, since there are
> no
> Calling MessageLoop::Run is discouraged.
> Please use base::RunLoop::Run() (or RunUntilIdle() which does not require
> manually calling Quit().)
> > requirements in ProvidedFileSystemInterface on way of calling callbacks,
then
> by
> > calling them synchronously, unit tests may catch some implementation errors
> (eg.
> > relying on order of calls). Note, that the real ProvidedFileSystem from the
> > production code is tested by api tests (browser test).
> I guess browser_tests is going to have smaller test coverage and many parts
like
> corner-case error handling will be only covered by unit_tests.
> This means that, if you or someone mistakenly add a corner-case handling code
> which relies on callbacks called synchronously, automated tests cannot find it
> automatically.
> > 
> > Anyway, I don't have hard feelings here. If you prefer PostTask, I'll do a
> > PostTask and add MessageLoop::current()->Run() to unit tests. Let me know.
> Please do it.
> Adding additional base::RunLoop::RunUntilIdle() is much better than leaving
bugs
> undetected.

Done.

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager_unittest.cc (right):

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager_unittest.cc:152:
ProvidedFileSystemFactoryInterface>(
On 2014/04/15 07:22:45, hashimoto wrote:
> You can use make_scoped_ptr

I didn't know about it. This is great!

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc
(right):

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc:167:
scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
On 2014/04/15 07:22:45, hashimoto wrote:
> It seems this variable is not needed.

Done.

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service.h (right):

https://codereview.chromium.org/210803003/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.h:44: //
|file_system_factory| instance.
On 2014/04/15 07:22:45, hashimoto wrote:
> scoped_ptr always takes the ownership.
> No need to say it here.

Done.

[hashimoto, 2014-04-15 13:42:00]

lgtm

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager_unittest.cc (right):

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager_unittest.cc:157:
chromeos::file_system_provider::FakeProvidedFileSystem::Create));
nit: '&' can be omitted here, but we usually add '&'.

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:43: class
ProvidedFileSystemFactory : public ProvidedFileSystemFactoryInterface {
This class is unneeded?

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:36:
class ProvidedFileSystemFactoryInterface {
This interface is unneeded?

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

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:16: #include
"extensions/browser/event_router.h"
nit: No need to include this as we are just passing a pointer of EventRouter?

[mtomasz, 2014-04-16 01:04:03]

Thanks for the detailed review!

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/volume_manager_unittest.cc (right):

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/volume_manager_unittest.cc:157:
chromeos::file_system_provider::FakeProvidedFileSystem::Create));
On 2014/04/15 13:42:00, hashimoto wrote:
> nit: '&' can be omitted here, but we usually add '&'.

Done.

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:43: class
ProvidedFileSystemFactory : public ProvidedFileSystemFactoryInterface {
On 2014/04/15 13:42:00, hashimoto wrote:
> This class is unneeded?

Done.

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:36:
class ProvidedFileSystemFactoryInterface {
On 2014/04/15 13:42:00, hashimoto wrote:
> This interface is unneeded?

Done.

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

https://codereview.chromium.org/210803003/diff/550001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service.cc:16: #include
"extensions/browser/event_router.h"
On 2014/04/15 13:42:00, hashimoto wrote:
> nit: No need to include this as we are just passing a pointer of EventRouter? 

Done.

[mtomasz, 2014-04-16 01:04:11]

The CQ bit was checked by mtomasz@chromium.org

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

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

[commit-bot: I haz the power, 2014-04-16 01:38:33]

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

[mtomasz, 2014-04-16 04:03:41]

Committed patchset #15 manually as r264106 (presubmit successful).