[fsp] Decouple file_service_provider::Service.

chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc

Index: chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
diff --git a/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc b/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d0ce1c9a3bd21f3d9979d91238fe5339ca2a9cef
--- /dev/null
+++ b/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc
@@ -0,0 +1,49 @@
+// 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.
+
+#include "chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h"
+
+#include "base/files/file.h"
+#include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "chrome/browser/profiles/profile.h"
+
+namespace chromeos {
+namespace file_system_provider {
+
+FakeProvidedFileSystem::FakeProvidedFileSystem() {}
+
+FakeProvidedFileSystem::FakeProvidedFileSystem(
+    const ProvidedFileSystemInfo& file_system_info)
+    : file_system_info_(file_system_info) {}
+
+FakeProvidedFileSystem::~FakeProvidedFileSystem() {}
+
+bool FakeProvidedFileSystem::RequestUnmount(
+    const fileapi::AsyncFileUtil::StatusCallback& callback) {
+  callback.Run(base::File::FILE_OK);

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

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();

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

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.

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

Calling MessageLoop::Run is discouraged.
Please use base::RunLoop::Run() (or RunUntilIdle() which does not require
manually calling Quit().)
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.
Please do it.
Adding additional base::RunLoop::RunUntilIdle() is much better than leaving bugs
undetected.

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

Done.

+  return true;
+}
+
+const ProvidedFileSystemInfo& FakeProvidedFileSystem::GetFileSystemInfo()
+    const {
+  return file_system_info_;
+}
+
+FakeProvidedFileSystemFactory::FakeProvidedFileSystemFactory() {}
+
+ProvidedFileSystemInterface* FakeProvidedFileSystemFactory::Create(
+    Profile* profile,
+    RequestManager* request_manager,
+    const ProvidedFileSystemInfo& file_system_info) {
+  DCHECK(profile);
+  DCHECK(request_manager);
+  // TODO(mtomasz): Create a request manager in ProvidedFileSystem, since it is
+  // only used by ProvidedFileSystem, instead of having a profile wide one.
+  // As a result, there will be no need to pass the request manager to
+  // ProvidedFileSystemInterface::Create().
+  return new FakeProvidedFileSystem(file_system_info);
+}
+
+}  // namespace file_system_provider
+}  // namespace chromeos