Defer ARC file system operations while ARC is booting.

chrome/browser/chromeos/arc/fileapi/arc_deferred_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_
+
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/gtest_prod_util.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.

[hidehiko, 2017/01/20 08:01:49]

Wow, nice comment!

[Shuhei Takahashi, 2017/01/20 09:13:04]

:)

+//
+// 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 oepration runner provides better UX when the user attempts

[hidehiko, 2017/01/20 08:01:49]

s/oepration/operation/

[Shuhei Takahashi, 2017/01/20 09:13:03]

Good catch.

+// 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,
+      public ArcSessionManager::Observer,
+      public InstanceHolder<mojom::FileSystemInstance>::Observer {
+ public:
+  explicit ArcDeferredFileSystemOperationRunner(
+      ArcBridgeService* bridge_service);
+  ~ArcDeferredFileSystemOperationRunner() override;
+
+  // ArcFileSystemOperationRunner overrides:
+  void GetFileSize(const std::string& url,
+                   const GetFileSizeCallback& callback) override;
+  void OpenFileToRead(const std::string& url,
+                      const OpenFileToReadCallback& callback) override;
+  void GetDocument(const std::string& authority,
+                   const std::string& document_id,
+                   const GetDocumentCallback& callback) override;
+  void GetChildDocuments(const std::string& authority,
+                         const std::string& parent_document_id,
+                         const GetChildDocumentsCallback& callback) override;
+
+  // ArcSessionManager::Observer overrides:

[hidehiko, 2017/01/20 08:01:49]

Optional: L63 - L68 can be private?
These are for implementation purpose, than an interface this class wants to
provide?

[Shuhei Takahashi, 2017/01/20 09:13:04]

It is possible to make them private, but do we want to do so? I've never seen in
Chrome codebase interface implementations are hidden as private. Also it is
actually useless because anyone can call them by casting to interface classes.

+  void OnArcOptInChanged(bool enabled) override;
+
+  // InstanceHolder<mojom::FileSystemInstance>::Observer overrides:
+  void OnInstanceReady() override;
+  void OnInstanceClosed() override;
+
+ private:
+  friend class ArcDeferredFileSystemOperationRunnerTest;
+  FRIEND_TEST_ALL_PREFIXES(ArcDeferredFileSystemOperationRunnerTest,

[hidehiko, 2017/01/20 08:01:49]

Through my recent refactoring work, I felt maintaining FRIEND_TEST_ALL_PREFIXES
is costly, even it looked a simpler case.
So, can we avoid FRIEND_TEST_ALL_PREFIXES, e.g., as follows?

friend class ArcDeferredFileSystemOperationRunnerTest;

class ArcDeferredFileSystemOperationRunnerTest {
  ...
  void CallOnStateChanged() {
    deferred_runner_->OnStateChanged();
  }
};

TEST_F(ArcDeferredFileSystemOperationRunnerTest, ...) {
  CallOnStateChanged();
}

or,

Define OnStateChangedForTesting() function in this class?

[Shuhei Takahashi, 2017/01/20 09:13:04]

Makes sense, done.

+                           RunImmediately);
+  FRIEND_TEST_ALL_PREFIXES(ArcDeferredFileSystemOperationRunnerTest,
+                           DeferAndRun);
+  FRIEND_TEST_ALL_PREFIXES(ArcDeferredFileSystemOperationRunnerTest,
+                           DeferAndDiscard);
+
+  ArcDeferredFileSystemOperationRunner(ArcBridgeService* bridge_service,

[hidehiko, 2017/01/20 08:01:49]

IIUC, this is for testing purpose (so that public ctor just redirects to this)?
Could you comment?

[Shuhei Takahashi, 2017/01/20 09:13:04]

Sure, done.

+                                       bool observe_events);
+
+  // Called whenever ARC states related to |should_defer_| are changed.
+  void OnStateChanged();
+
+  // Enables/disables 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;
+
+  // List of deferred operations.
+  std::vector<base::Closure> deferred_operations_;
+
+  base::WeakPtrFactory<ArcDeferredFileSystemOperationRunner> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcDeferredFileSystemOperationRunner);
+};
+
+}  // namespace arc
+
+#endif  // CHROME_BROWSER_CHROMEOS_ARC_FILEAPI_ARC_DEFERRED_FILE_SYSTEM_OPERATION_RUNNER_H_