mediaview: Support watchers in ArcFileSystemOperationRunner.

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

 // 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.
 
 #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"
 #include "base/optional.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
 #include "components/arc/arc_bridge_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
 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";
 
 // static
 std::unique_ptr<ArcFileSystemOperationRunner>
 ArcFileSystemOperationRunner::CreateForTesting(
     ArcBridgeService* bridge_service) {
   // We can't use base::MakeUnique() here because we are calling a private
   // constructor.
   return base::WrapUnique<ArcFileSystemOperationRunner>(
       new ArcFileSystemOperationRunner(bridge_service, nullptr, false));
 }
 
 ArcFileSystemOperationRunner::ArcFileSystemOperationRunner(
     ArcBridgeService* bridge_service,
     const Profile* profile)
     : ArcFileSystemOperationRunner(bridge_service, profile, true) {
   DCHECK(profile);
 }
 
 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.
 }
 
 void ArcFileSystemOperationRunner::GetFileSize(
     const GURL& url,
     const GetFileSizeCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (should_defer_) {
     deferred_operations_.emplace_back(
         base::Bind(&ArcFileSystemOperationRunner::GetFileSize,
@@ skipping 67 matching lines @@
       arc_bridge_service()->file_system(), GetChildDocuments);
   if (!file_system_instance) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, base::nullopt));
     return;
   }
   file_system_instance->GetChildDocuments(authority, parent_document_id,
                                           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();
 }
 
 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();
 }
 
 void ArcFileSystemOperationRunner::OnInstanceClosed() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   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) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   should_defer_ = should_defer;
 
   if (should_defer_)
     return;
 
   // Run deferred operations.
   std::vector<base::Closure> deferred_operations;
   deferred_operations.swap(deferred_operations_);
   for (const base::Closure& operation : deferred_operations) {
     operation.Run();
   }
 
   // No deferred operations should be left at this point.
   DCHECK(deferred_operations_.empty());
 }
 
 }  // namespace arc