mediaview: Support watchers in ArcFileSystemOperationRunner.

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

Index: chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc
diff --git a/chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc b/chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc
index 9e914c16f9b9ebc06ccca3ee45277ad827f8a7a1..161989d61b5d052c6bcd094bb28cc415edad721e 100644
--- a/chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc
+++ b/chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
@@ -18,6 +20,22 @@ using content::BrowserThread;
 
 namespace arc {
 
+namespace {
+
+ArcFileSystemOperationRunner::ChangeType FromMojoChangeType(
+    mojom::ChangeType type) {
+  switch (type) {
+    case mojom::ChangeType::CHANGED:
+      return ArcFileSystemOperationRunner::ChangeType::CHANGED;
+    case mojom::ChangeType::DELETED:
+      return ArcFileSystemOperationRunner::ChangeType::DELETED;

[dcheng, 2017/02/28 06:56:32]

Let's add a typemap for this.

[Shuhei Takahashi, 2017/03/01 09:10:08]

Typemap sounds good and I tried adding one, but I could not resolve GN errors
possibly due to weird configs in //storage/browser. Please let me address this
later.

[dcheng, 2017/03/01 21:47:57]

What's the error you're seeing?

[Shuhei Takahashi, 2017/03/02 02:05:01]

Here's a WIP patch:
https://codereview.chromium.org/2727113002/

I'm glad if you could help resolving the GN error. Anyway, the change to add
typemaps will be not trivial, so I want to keep it a separate patch.

+  }
+  NOTREACHED();

[Luis Héctor Chávez, 2017/03/06 17:10:48]

nit: mention that the above switch is exhaustive, but some compilers (old gcc?)
still complain about it :/

+  return ArcFileSystemOperationRunner::ChangeType::CHANGED;
+}
+
+}  // namespace
+
 // static
 const char ArcFileSystemOperationRunner::kArcServiceName[] =
     "arc::ArcFileSystemOperationRunner";
@@ -42,27 +60,33 @@ ArcFileSystemOperationRunner::ArcFileSystemOperationRunner(
 ArcFileSystemOperationRunner::ArcFileSystemOperationRunner(
     ArcBridgeService* bridge_service,
     const Profile* profile,
-    bool observe_events)
+    bool set_should_defer_by_events)
     : ArcService(bridge_service),
       profile_(profile),
-      observe_events_(observe_events),
+      set_should_defer_by_events_(set_should_defer_by_events),
+      binding_(this),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (observe_events_) {
-    ArcSessionManager::Get()->AddObserver(this);
-    arc_bridge_service()->file_system()->AddObserver(this);
-    OnStateChanged();
-  }
+  // We need to observe FileSystemInstance even in unit tests to call Init().
+  arc_bridge_service()->file_system()->AddObserver(this);
+
+  // ArcSessionManager may not exist in unit tests.

[dcheng, 2017/02/28 06:56:32]

Nit: in many other parts of Chrome, null for testing has made code harder to
understand. If possible, let's make it so test code and real code don't have to
differentiate on this.

[Shuhei Takahashi, 2017/03/01 09:10:08]

I totally agree checking null is not good, but we don't have working fakes of
ArcSessionManager. Good news is that some folks are working on large refactoring
around ArcSessionManager, so I hope we're in better situation after it is
finished.

+  auto* arc_session_manager = ArcSessionManager::Get();
+  if (arc_session_manager)
+    arc_session_manager->AddObserver(this);
+
+  OnStateChanged();
 }
 
 ArcFileSystemOperationRunner::~ArcFileSystemOperationRunner() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (observe_events_) {
-    ArcSessionManager::Get()->RemoveObserver(this);
-    arc_bridge_service()->file_system()->RemoveObserver(this);
-  }
+  auto* arc_session_manager = ArcSessionManager::Get();
+  if (arc_session_manager)
+    arc_session_manager->AddObserver(this);

[dcheng, 2017/02/28 06:56:32]

RemoveObserver?

[Shuhei Takahashi, 2017/03/01 09:10:08]

Oops! Thanks for catching.

+
+  arc_bridge_service()->file_system()->RemoveObserver(this);
   // On destruction, deferred operations are discarded.
 }
 
@@ -150,6 +174,80 @@ void ArcFileSystemOperationRunner::GetChildDocuments(
                                           callback);
 }
 
+void ArcFileSystemOperationRunner::AddWatcher(
+    const std::string& authority,
+    const std::string& document_id,
+    const WatcherCallback& watcher_callback,
+    const AddWatcherCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (should_defer_) {
+    deferred_operations_.emplace_back(
+        base::Bind(&ArcFileSystemOperationRunner::AddWatcher,
+                   weak_ptr_factory_.GetWeakPtr(), authority, document_id,
+                   watcher_callback, callback));
+    return;
+  }
+  auto* file_system_instance = ARC_GET_INSTANCE_FOR_METHOD(
+      arc_bridge_service()->file_system(), AddWatcher);
+  if (!file_system_instance) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, -1));
+    return;
+  }
+  file_system_instance->AddWatcher(
+      authority, document_id,
+      base::Bind(&ArcFileSystemOperationRunner::OnWatcherAdded,
+                 weak_ptr_factory_.GetWeakPtr(), watcher_callback, callback));
+}
+
+void ArcFileSystemOperationRunner::RemoveWatcher(
+    int64_t watcher_id,
+    const RemoveWatcherCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // RemoveWatcher() is never deferred since watchers do not persist across
+  // container reboots.
+  if (should_defer_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, false));
+    return;
+  }
+
+  // Unregister from |watcher_callbacks_| now because we will do it even if
+  // the remote method fails anyway. This is an implementation detail, so
+  // users must not assume registered callbacks are immediately invalidated.
+  auto iter = watcher_callbacks_.find(watcher_id);
+  if (iter == watcher_callbacks_.end()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, false));
+    return;
+  }
+  watcher_callbacks_.erase(iter);
+
+  auto* file_system_instance = ARC_GET_INSTANCE_FOR_METHOD(
+      arc_bridge_service()->file_system(), AddWatcher);
+  if (!file_system_instance) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, false));
+    return;
+  }
+  file_system_instance->RemoveWatcher(watcher_id, callback);
+}
+
+void ArcFileSystemOperationRunner::OnDocumentChanged(int64_t watcher_id,
+                                                     mojom::ChangeType type) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto iter = watcher_callbacks_.find(watcher_id);
+  if (iter == watcher_callbacks_.end()) {
+    // This may happen in a race condition with documents changes and
+    // RemoveWatcher().
+    return;
+  }
+  WatcherCallback watcher_callback = iter->second;
+  if (type == mojom::ChangeType::DELETED)
+    watcher_callbacks_.erase(watcher_id);
+  watcher_callback.Run(FromMojoChangeType(type));
+}
+
 void ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged(bool enabled) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   OnStateChanged();
@@ -157,6 +255,10 @@ void ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged(bool enabled) {
 
 void ArcFileSystemOperationRunner::OnInstanceReady() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto* file_system_instance =
+      ARC_GET_INSTANCE_FOR_METHOD(arc_bridge_service()->file_system(), Init);
+  if (file_system_instance)
+    file_system_instance->Init(binding_.CreateInterfacePtrAndBind());
   OnStateChanged();
 }
 
@@ -165,10 +267,26 @@ void ArcFileSystemOperationRunner::OnInstanceClosed() {
   OnStateChanged();
 }
 
+void ArcFileSystemOperationRunner::OnWatcherAdded(
+    const WatcherCallback& watcher_callback,
+    const AddWatcherCallback& callback,
+    int64_t watcher_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (watcher_id < 0) {
+    callback.Run(-1);
+    return;
+  }
+  DCHECK_EQ(0u, watcher_callbacks_.count(watcher_id));

[dcheng, 2017/02/28 06:56:32]

As this is coming from a less-trusted container, we can't make any assumptions.
If it's important that watcher_id is not already in the container, we need to
explicitly handle that.

[Shuhei Takahashi, 2017/03/01 09:10:08]

Makes sense, done.

+  watcher_callbacks_.insert(std::make_pair(watcher_id, watcher_callback));
+  callback.Run(watcher_id);
+}
+
 void ArcFileSystemOperationRunner::OnStateChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  SetShouldDefer(IsArcPlayStoreEnabledForProfile(profile_) &&
-                 !arc_bridge_service()->file_system()->has_instance());
+  if (set_should_defer_by_events_) {
+    SetShouldDefer(IsArcPlayStoreEnabledForProfile(profile_) &&
+                   !arc_bridge_service()->file_system()->has_instance());
+  }
 }
 
 void ArcFileSystemOperationRunner::SetShouldDefer(bool should_defer) {