[fsp] Decouple file_service_provider::Service.

chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_FAKE_PROVIDED_FILE_SYSTEM_H_
+#define CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_FAKE_PROVIDED_FILE_SYSTEM_H_
+
+#include <string>
+
+#include "base/files/file_path.h"
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
+
+class Profile;
+
+namespace chromeos {
+namespace file_system_provider {
+
+class RequestManager;
+
+// Fake provided file system implementation. Does not communicate with target
+// extensions. Used for unit tests.
+class FakeProvidedFileSystem : public ProvidedFileSystemInterface {
+ public:
+  FakeProvidedFileSystem();
+  explicit FakeProvidedFileSystem(
+      const ProvidedFileSystemInfo& file_system_info);
+  virtual ~FakeProvidedFileSystem();
+
+  // ProvidedFileSystemInterface overrides.
+  virtual bool RequestUnmount(
+      const fileapi::AsyncFileUtil::StatusCallback& callback) OVERRIDE;
+  virtual const ProvidedFileSystemInfo& GetFileSystemInfo() const OVERRIDE;
+
+ private:
+  ProvidedFileSystemInfo file_system_info_;
+  DISALLOW_COPY_AND_ASSIGN(FakeProvidedFileSystem);
+};
+
+// Factory creating fake provided file system instances.
+class FakeProvidedFileSystemFactory
+    : public ProvidedFileSystemFactoryInterface {

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

Do we need an interface for this?
Can't this be replaced with a callback?

[mtomasz, 2014/04/11 07:11:56]

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

[hashimoto, 2014/04/14 10:43:42]

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?

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

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.

[hashimoto, 2014/04/15 07:22:45]

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.
base::Callback is one of the most fundamental classes of Chromium.
Every Chromium developer is expected to be familiar with it.
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.
I agree, this is the reason why DriveIntegrationService is using base::Callback
instead.

[mtomasz, 2014/04/15 09:42:12]

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

+ public:
+  FakeProvidedFileSystemFactory();
+  virtual ~FakeProvidedFileSystemFactory();
+
+  // ProvidedFileSystemFactoryInterface overrides.
+  virtual ProvidedFileSystemInterface* Create(
+      Profile* profile,
+      RequestManager* request_manager,
+      const ProvidedFileSystemInfo& file_system_info) OVERRIDE;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FakeProvidedFileSystemFactory);
+};
+
+}  // namespace file_system_provider
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_FAKE_PROVIDED_FILE_SYSTEM_H_