Clean up ARC file system unit tests.

chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h

 // Copyright 2017 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_ARC_FILEAPI_ARC_DEFERRED_FILE_SYSTEM_OPERATION_RUNNER_H_
-#define CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_DEFERRED_FILE_SYSTEM_OPERATION_RUNNER_H_
+#ifndef CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_FILE_SYSTEM_OPERATION_RUNNER_H_
+#define CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_FILE_SYSTEM_OPERATION_RUNNER_H_
 
 #include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "components/arc/arc_service.h"
 #include "components/arc/common/file_system.mojom.h"
-#include "components/arc/file_system/arc_file_system_operation_runner.h"
 #include "components/arc/instance_holder.h"
 
 namespace arc {
 
-// Implements deferred ARC file system operations.
+// Runs ARC file system operations.
+//
+// This is an abstraction layer on top of mojom::FileSystemInstance. All ARC

[hidehiko, 2017/01/26 07:24:20]

I like this is explicitly documented :D. Thank you!

+// file system operations should go through this interface, rather than

[hidehiko, 2017/01/26 07:24:20]

Optional: maybe s/interface/class/ is clearer here?
In Chrome/C++ context, "interface" is often used for a class with pure-virtual
methods,
but yours is concrete one?

[Shuhei Takahashi, 2017/01/27 09:55:10]

Done.

+// invoking mojom::FileSystemInstance directly.
 //
 // When ARC is disabled or ARC has already booted, file system operations are
 // performed immediately. While ARC boot is under progress, file operations are
 // deferred until ARC boot finishes or the user disables ARC.
 //
 // This file system operation runner provides better UX when the user attempts
 // to perform file operations while ARC is booting. For example:
 //
 // - Media views are mounted in Files app soon after the user logs into
 //   the system. If the user attempts to open media views before ARC boots,
 //   a spinner is shown until file system gets ready because ReadDirectory
 //   operations are deferred.
 // - When an Android content URL is opened soon after the user logs into
 //   the system (because the user opened the tab before they logged out for
 //   instance), the tab keeps loading until ARC boot finishes, instead of
 //   failing immediately.
 //
 // All member functions must be called on the UI thread.
-class ArcDeferredFileSystemOperationRunner
-    : public ArcFileSystemOperationRunner,
+class ArcFileSystemOperationRunner
+    : public ArcService,
       public ArcSessionManager::Observer,
       public InstanceHolder<mojom::FileSystemInstance>::Observer {
  public:
-  explicit ArcDeferredFileSystemOperationRunner(
+  using GetFileSizeCallback = mojom::FileSystemInstance::GetFileSizeCallback;
+  using OpenFileToReadCallback =
+      mojom::FileSystemInstance::OpenFileToReadCallback;
+  using GetDocumentCallback = mojom::FileSystemInstance::GetDocumentCallback;
+  using GetChildDocumentsCallback =
+      mojom::FileSystemInstance::GetChildDocumentsCallback;
+
+  // For supporting ArcServiceManager::GetService<T>().
+  static const char kArcServiceName[];
+
+  // Creates an instance suitable for unit tests.
+  // This instance will run all operations immediately without deferring by
+  // default. Also, deferring can be enabled/disabled by calling
+  // SetShouldDefer() from friend classes.
+  static ArcFileSystemOperationRunner* CreateForTesting(

[hidehiko, 2017/01/26 07:24:20]

Could you return std::unique_ptr<ArcFileSystemOperationRunner> instead, to let
client manage the instance?

[Shuhei Takahashi, 2017/01/27 09:55:10]

Done.

       ArcBridgeService* bridge_service);
-  ~ArcDeferredFileSystemOperationRunner() override;
 
-  // ArcFileSystemOperationRunner overrides:
-  void GetFileSize(const GURL& url,
-                   const GetFileSizeCallback& callback) override;
-  void OpenFileToRead(const GURL& url,
-                      const OpenFileToReadCallback& callback) override;
+  // The standard constructor. A production instance should be created by
+  // this constructor.
+  explicit ArcFileSystemOperationRunner(ArcBridgeService* bridge_service);
+  ~ArcFileSystemOperationRunner() override;
+
+  // Runs file system operations. See file_system.mojom for documentation.
+  void GetFileSize(const GURL& url, const GetFileSizeCallback& callback);
+  void OpenFileToRead(const GURL& url, const OpenFileToReadCallback& callback);
   void GetDocument(const std::string& authority,
                    const std::string& document_id,
-                   const GetDocumentCallback& callback) override;
+                   const GetDocumentCallback& callback);
   void GetChildDocuments(const std::string& authority,
                          const std::string& parent_document_id,
-                         const GetChildDocumentsCallback& callback) override;
+                         const GetChildDocumentsCallback& callback);
 
   // ArcSessionManager::Observer overrides:
   void OnArcOptInChanged(bool enabled) override;
 
   // InstanceHolder<mojom::FileSystemInstance>::Observer overrides:
   void OnInstanceReady() override;
   void OnInstanceClosed() override;
 
  private:
-  friend class ArcDeferredFileSystemOperationRunnerTest;
+  friend class ArcFileSystemOperationRunnerTest;
 
-  // Constructor called from unit tests.
-  ArcDeferredFileSystemOperationRunner(ArcBridgeService* bridge_service,
-                                       bool observe_events);
+  ArcFileSystemOperationRunner(ArcBridgeService* bridge_service,
+                               bool observe_events);
 
   // Called whenever ARC states related to |should_defer_| are changed.
   void OnStateChanged();
 
   // Enables/disables deferring.
+  // Friend unit tests can call this function to simulate enabling/disabling
+  // deferring.
   void SetShouldDefer(bool should_defer);
 
   // Indicates if this instance should register observers to receive events.
   // Usually true, but set to false in unit tests.
   bool observe_events_;
 
   // Set to true if operations should be deferred at this moment.
-  bool should_defer_ = true;
+  // The default is set to false so that operations are not deferred in
+  // unit tests.
+  bool should_defer_ = false;
 
   // List of deferred operations.
   std::vector<base::Closure> deferred_operations_;
 
-  base::WeakPtrFactory<ArcDeferredFileSystemOperationRunner> weak_ptr_factory_;
+  base::WeakPtrFactory<ArcFileSystemOperationRunner> weak_ptr_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(ArcDeferredFileSystemOperationRunner);
+  DISALLOW_COPY_AND_ASSIGN(ArcFileSystemOperationRunner);
 };
 
 }  // namespace arc
 
-#endif  // CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_DEFERRED_FILE_SYSTEM_OPERATION_RUNNER_H_
+#endif  // CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_FILE_SYSTEM_OPERATION_RUNNER_H_