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 79be422c34d293381368a8a5948180168c4de701..01f59e27bfd2827f2a62aa7d6df5cfdbf819f33b 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"
@@ -17,6 +19,23 @@ 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;
+    default:
+      NOTREACHED();

[hidehiko, 2017/02/23 11:55:28]

Could you move this block after switch so that (some) compiler warns if some
value is missing, which helps specifically if a new value is added in
mojom::ChangeType.
cf) ARC style guide.

[Shuhei Takahashi, 2017/02/24 06:10:39]

Done.

+      return ArcFileSystemOperationRunner::ChangeType::CHANGED;
+  }
+}
+
+}  // namespace
+
 // static
 const char ArcFileSystemOperationRunner::kArcServiceName[] =
     "arc::ArcFileSystemOperationRunner";
@@ -37,26 +56,28 @@ ArcFileSystemOperationRunner::ArcFileSystemOperationRunner(
 
 ArcFileSystemOperationRunner::ArcFileSystemOperationRunner(
     ArcBridgeService* bridge_service,
-    bool observe_events)
+    bool set_should_defer_by_events)
     : ArcService(bridge_service),
-      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_) {
+  // 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.

[hidehiko, 2017/02/23 11:55:28]

auto* arc_session_manager = ArcSessionManager::Get();
if (arc_session_manager)
  arc_session_manager->AddObserver(this);

looks more fit to the comment?
If you'd like, you can DCHECK in delegating ctor, I think.

[Shuhei Takahashi, 2017/02/24 06:10:39]

Sounds good, done.

+  if (set_should_defer_by_events_)
     ArcSessionManager::Get()->AddObserver(this);
-    arc_bridge_service()->file_system()->AddObserver(this);
-    OnStateChanged();
-  }
+
+  OnStateChanged();
 }
 
 ArcFileSystemOperationRunner::~ArcFileSystemOperationRunner() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (observe_events_) {
+  if (set_should_defer_by_events_)
     ArcSessionManager::Get()->RemoveObserver(this);
-    arc_bridge_service()->file_system()->RemoveObserver(this);
-  }
+  arc_bridge_service()->file_system()->RemoveObserver(this);
   // On destruction, deferred operations are discarded.
 }
 
@@ -144,6 +165,77 @@ 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::AddWatcherDone,
+                 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_| before calling the remote method.
+  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);

[hidehiko, 2017/02/23 11:55:28]

This hides the OnDocumentChanged event between RemoveWatcher() and its callback,
which means this is slightly different from the
mojom::FileSystemInstance::RemoveWatcher behavior (in terms of timing).

Could you;
1) update the code so that the both behavior is aligned, or
2) explicitly document the difference?

+
+  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()) {
+    return;

[hidehiko, 2017/02/23 11:55:28]

Could you comment when this happens?

[Shuhei Takahashi, 2017/02/24 06:10:39]

Done.

+  }
+  WatcherCallback watcher_callback = iter->second;
+  if (type == mojom::ChangeType::DELETED) {

[hidehiko, 2017/02/23 11:55:28]

Could you elide brace of one line if stmt, for consistency?

[Shuhei Takahashi, 2017/02/24 06:10:39]

Done.

+    watcher_callbacks_.erase(watcher_id);
+  }
+  watcher_callback.Run(FromMojoChangeType(type));
+}
+
 void ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged(bool enabled) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   OnStateChanged();
@@ -151,6 +243,11 @@ 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) {

[hidehiko, 2017/02/23 11:55:28]

ditto

[Shuhei Takahashi, 2017/02/24 06:10:39]

Done.

+    file_system_instance->Init(binding_.CreateInterfacePtrAndBind());
+  }
   OnStateChanged();
 }
 
@@ -159,10 +256,25 @@ void ArcFileSystemOperationRunner::OnInstanceClosed() {
   OnStateChanged();
 }
 
+void ArcFileSystemOperationRunner::AddWatcherDone(
+    const WatcherCallback& watcher_callback,
+    const AddWatcherCallback& callback,
+    int64_t watcher_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (watcher_id < 0) {
+    callback.Run(-1);
+    return;
+  }
+  watcher_callbacks_.insert(std::make_pair(watcher_id, watcher_callback));

[hidehiko, 2017/02/23 11:55:28]

Optional: Just in case how about 

DCHECK_EQ(watcher_callbacks_.count(watcher_id), 0);

[Shuhei Takahashi, 2017/02/24 06:10:39]

Done.

+  callback.Run(watcher_id);
+}
+
 void ArcFileSystemOperationRunner::OnStateChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  SetShouldDefer(ArcSessionManager::Get()->IsArcPlayStoreEnabled() &&
-                 !arc_bridge_service()->file_system()->has_instance());
+  if (set_should_defer_by_events_) {
+    SetShouldDefer(ArcSessionManager::Get()->IsArcPlayStoreEnabled() &&
+                   !arc_bridge_service()->file_system()->has_instance());
+  }
 }
 
 void ArcFileSystemOperationRunner::SetShouldDefer(bool should_defer) {
@@ -170,8 +282,10 @@ void ArcFileSystemOperationRunner::SetShouldDefer(bool should_defer) {
 
   should_defer_ = should_defer;
 
-  if (should_defer_)
+  if (should_defer_) {
+    watcher_callbacks_.clear();

[hidehiko, 2017/02/23 11:55:28]

Could you comment why this is the timing for watcher_callbacks_ to be cleared?

[Shuhei Takahashi, 2017/02/24 06:10:39]

Hm, I thought it's better to clear it because |should_defer_| = true means the
container is booting and thus watchers have been cleared, but on a container
crash Chrome browser process should restart, so we don't need this. Let's just
remove it.

[hidehiko, 2017/02/24 16:16:17]

Sorry, I'm confused. AFAIK, even if the container is crashed, Chrome browser
process would be still alive. If the watcher ID is not persistent, it needs to
be cleared somehow?

BTW, if this is for ARC crashing case (I didn't come up with it in previous
iteration. Thank you!),
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_session_...
is the method to subscribe.
OnArcPlayStoreEnabledChanged() is called only when the preference is changed as
named.
Also, I'm interested in the expected behavior in the case. Clearing watcher
means the receivers will never receives the event. Is it acceptable practically?
Or is there any more change is needed to cover the case?

[hidehiko, 2017/02/25 14:41:30]

Oh, one thing I was wrong here. You do not actually need ArcSessionObserver,
because OnInstance{Ready,Close} can be used for that purpose. Sorry for
confusing comment.

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

Thanks for explanation. I had a misunderstanding that Chrome browser process
will restart on Android container restart, but it's obviously false.

Since watcher IDs are cleared on Android restart, we need to notify the event to
the users of this class. I introduced Observer for that purpose.

     return;
+  }
 
   // Run deferred operations.
   std::vector<base::Closure> deferred_operations;