[ew] Add basic classes.

chrome/browser/file_system/entry_watcher_service.cc

+// Copyright 2014 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/file_system/entry_watcher_service.h"
+
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/api/file_system.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
+#include "extensions/browser/event_router.h"
+#include "webkit/browser/fileapi/file_system_context.h"
+
+namespace extensions {
+namespace {
+
+// Default implementation for dispatching an event. Can be replaced for unit
+// tests by EntryWatcherService::SetDispatchEventImplForTesting().
+bool DispatchEventImpl(extensions::EventRouter* event_router,
+                       const std::string& extension_id,
+                       scoped_ptr<extensions::Event> event) {
+  if (!event_router->ExtensionHasEventListener(extension_id, event->event_name))

[benwells, 2014/08/13 05:50:47]

Is it necessary to check this and return a bool?

[mtomasz, 2014/08/13 07:02:08]

Right. Removed.

+    return false;
+
+  event_router->DispatchEventToExtension(extension_id, event.Pass());
+  return true;
+}
+
+// Default implementation for acquiring a file system context for a specific
+// |extension_id| and |context|.
+fileapi::FileSystemContext* GetFileSystemContextImpl(
+    const std::string& extension_id,
+    content::BrowserContext* context) {
+  const GURL site =
+      extensions::util::GetSiteForExtensionId(extension_id, context);
+  return content::BrowserContext::GetStoragePartitionForSite(context, site)
+      ->GetFileSystemContext();
+}
+
+}  // namespace
+
+EntryWatcherService::EntryWatcherService(content::BrowserContext* context)
+    : context_(context),
+      dispatch_event_impl_(base::Bind(&DispatchEventImpl,
+                                      extensions::EventRouter::Get(context))),
+      get_file_system_context_impl_(base::Bind(&GetFileSystemContextImpl)),
+      observing_(this),
+      weak_ptr_factory_(this) {
+  // TODO(mtomasz): Restore persistent watchers.
+}
+
+EntryWatcherService::~EntryWatcherService() {
+}
+
+void EntryWatcherService::SetDispatchEventImplForTesting(
+    const DispatchEventImplCallback& callback) {
+  dispatch_event_impl_ = callback;
+}
+
+void EntryWatcherService::SetGetFileSystemContextImplForTesting(
+    const GetFileSystemContextImplCallback& callback) {
+  get_file_system_context_impl_ = callback;
+}
+
+void EntryWatcherService::WatchDirectory(
+    const std::string& extension_id,
+    const fileapi::FileSystemURL& url,
+    bool recursive,
+    const fileapi::WatcherManager::StatusCallback& callback) {
+  // TODO(mtomasz): Add support for recursive watchers.
+  if (recursive) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, base::File::FILE_ERROR_INVALID_OPERATION));
+    return;
+  }
+
+  fileapi::FileSystemContext* const context =
+      get_file_system_context_impl_.Run(extension_id, context_);
+  DCHECK(context);
+
+  fileapi::WatcherManager* const watcher_manager =
+      context->GetWatcherManager(url.type());
+  if (!watcher_manager) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[benwells, 2014/08/13 05:50:47]

Why do you post this and not just run it immediately?

[mtomasz, 2014/08/13 07:02:09]

Several times during reviews I was asked to post callbacks instead of calling
them directly with some exceptions. The reason is that a caller usually expects
it to be called asynchronously, eg.

void callback() {
  LOG(ERROR) << "Watcher creation finished.";
}

1. Call WatchDirectory(callback)
2. WatchDirectory executes PostTask(callback);
2. LOG(ERROR) << "Watcher creation started.";
3. callback is executed and prints "Watcher creation finished.".

If we called the callback directly, instead of posting it, we would get on the
screen:
Watcher creation finished.
Watcher creation started.

Another reason is that we may have different behavior depending if the callback
is called synchronously or asynchronously, hence if we always post it, then at
least we avoid one more possible bug.

[benwells, 2014/08/14 05:16:32]

OK, makes sense. Could you add a comment explaining that you're posting back to
keep the order of operations consistent in all cases?

[mtomasz, 2014/08/15 05:35:57]

Done.

+        FROM_HERE, base::Bind(callback, base::File::FILE_ERROR_SECURITY));

[benwells, 2014/08/13 05:50:47]

Why is this a security error?

[mtomasz, 2014/08/13 07:02:09]

Changed to base::File::FILE_ERROR_INVALID_OPERATION.

+    return;
+  }
+
+  watcher_manager->WatchDirectory(
+      url,
+      recursive,
+      base::Bind(&EntryWatcherService::OnWatchDirectoryCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 extension_id,
+                 url,
+                 recursive,
+                 callback));
+}
+
+void EntryWatcherService::UnwatchEntry(
+    const std::string& extension_id,
+    const fileapi::FileSystemURL& url,
+    const fileapi::WatcherManager::StatusCallback& callback) {
+  fileapi::FileSystemContext* const context =
+      get_file_system_context_impl_.Run(extension_id, context_);
+  DCHECK(context);
+
+  fileapi::WatcherManager* const watcher_manager =
+      context->GetWatcherManager(url.type());
+  if (!watcher_manager) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, base::File::FILE_ERROR_SECURITY));
+    return;
+  }
+
+  watcher_manager->UnwatchEntry(
+      url,
+      base::Bind(&EntryWatcherService::OnUnwatchEntryCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 extension_id,
+                 url,
+                 callback));
+}
+
+std::vector<fileapi::FileSystemURL> EntryWatcherService::GetWatchedEntries(
+    const std::string& extension_id) {
+  std::vector<fileapi::FileSystemURL> result;
+  for (WatcherMap::const_iterator it = watchers_.begin(); it != watchers_.end();
+       ++it) {
+    const std::map<std::string, EntryWatcher>::const_iterator watcher_it =
+        it->second.find(extension_id);
+    if (watcher_it != it->second.end())
+      result.push_back(watcher_it->second.url);
+  }
+
+  return result;
+}
+
+void EntryWatcherService::OnEntryChanged(const fileapi::FileSystemURL& url) {
+  const WatcherMap::const_iterator it = watchers_.find(url.ToGURL());
+  DCHECK(it != watchers_.end());
+  for (std::map<std::string, EntryWatcher>::const_iterator watcher_it =
+           it->second.begin();
+       watcher_it != it->second.end();
+       ++watcher_it) {
+    const std::string& extension_id = watcher_it->first;
+    api::file_system::EntryChangedEvent event;
+    dispatch_event_impl_.Run(
+        extension_id,
+        make_scoped_ptr(
+            new Event(api::file_system::OnEntryChanged::kEventName,
+                      api::file_system::OnEntryChanged::Create(event))));
+  }
+}
+
+void EntryWatcherService::OnEntryRemoved(const fileapi::FileSystemURL& url) {
+  WatcherMap::const_iterator it = watchers_.find(url.ToGURL());
+  DCHECK(it != watchers_.end());
+  for (std::map<std::string, EntryWatcher>::const_iterator watcher_it =
+           it->second.begin();
+       watcher_it != it->second.end();
+       ++watcher_it) {
+    const std::string& extension_id = watcher_it->first;
+    api::file_system::EntryRemovedEvent event;
+    dispatch_event_impl_.Run(
+        extension_id,
+        make_scoped_ptr(
+            new Event(api::file_system::OnEntryRemoved::kEventName,
+                      api::file_system::OnEntryRemoved::Create(event))));
+  }
+}
+
+void EntryWatcherService::OnWatchDirectoryCompleted(
+    const std::string& extension_id,
+    const fileapi::FileSystemURL& url,
+    bool recursive,
+    const fileapi::WatcherManager::StatusCallback& callback,
+    base::File::Error result) {
+  if (result != base::File::FILE_OK) {
+    callback.Run(result);
+    return;
+  }
+
+  DCHECK_EQ(base::File::FILE_OK, result);

[benwells, 2014/08/13 05:50:47]

Is this DCHECK necessary?

[mtomasz, 2014/08/13 07:02:09]

Removed.

+
+  fileapi::FileSystemContext* const context =
+      get_file_system_context_impl_.Run(extension_id, context_);
+  DCHECK(context);
+
+  fileapi::WatcherManager* const watcher_manager =
+      context->GetWatcherManager(url.type());
+  if (!watcher_manager) {
+    callback.Run(base::File::FILE_ERROR_SECURITY);

[benwells, 2014/08/13 05:50:47]

Given that you should have already successfully gotten the watcher manager
before calling this, is this a programming error (i.e. NOT_REACHED?).

[mtomasz, 2014/08/13 07:02:09]

GetWatcherManager() may return NULL at any point. There is no restriction on it,
so I think we can't assume it's returned value doesn't change. WDYT?

[benwells, 2014/08/14 05:16:32]

That seems odd, are you thinking about the cases where the provider is providing
the watcher, and we have no control over it?

Could we return a new error 'provider doing weird things' or something? Invalid
operation will communicate to an app that they have done something wrong, but
this seems like it would be an error on the providers part. Or maybe I'm missing
something....

[mtomasz, 2014/08/15 05:35:56]

I don't think it's weird. I can imagine a backend supporting some file system
type that is disabled while setting a watcher, and then GetWatcherManager()
returns NULL for that file type. It's unlikely to happen, but still I think any
backend can return NULL if it doesn't support a watcher anymore for any reason.

However, I tried to simplify the code and made following changes:
- Add a restriction that a pointer returned by GetWatcherManager() is valid
until shutdown.
- Bind the raw pointer to the EntryWatcher instance in WatchDirectory() to
OnWatchDirectoryCompleted.
Since UI message loop is the first thing to be stopped during shutdown, it
should safe.
Also, I believe KeyedServices are deleted *before* FileSystemContext, so it
should be all right.

WDYT?

[mtomasz, 2014/08/15 05:35:57]

I reorganized the code. How about now?

[benwells, 2014/08/18 22:58:32]

I think that is better, but the lifetime issues around shutdown are slightly
concerning. Could you add a comment explaining why it is safe?

[mtomasz, 2014/08/19 06:50:36]

Done.

+    return;
+  }
+
+  // Observe the manager if not observed yet.
+  if (!observing_.IsObserving(watcher_manager))
+    observing_.Add(watcher_manager);
+
+  const GURL gurl = url.ToGURL();
+  watchers_[gurl][extension_id] =
+      EntryWatcher(url, true /* directory */, recursive);
+
+  // TODO(mtomasz): Save in preferences.
+
+  callback.Run(base::File::FILE_OK);
+}
+
+void EntryWatcherService::OnUnwatchEntryCompleted(
+    const std::string& extension_id,
+    const fileapi::FileSystemURL& url,
+    const fileapi::WatcherManager::StatusCallback& callback,
+    base::File::Error result) {
+  if (result != base::File::FILE_OK) {
+    callback.Run(result);
+    return;
+  }
+
+  DCHECK_EQ(base::File::FILE_OK, result);
+
+  const GURL gurl = url.ToGURL();
+  if (watchers_[gurl].erase(extension_id) == 0) {
+    callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    return;
+  }
+
+  if (watchers_[gurl].empty())
+    watchers_.erase(gurl);
+
+  fileapi::FileSystemContext* const context =
+      get_file_system_context_impl_.Run(extension_id, context_);
+  DCHECK(context);
+
+  fileapi::WatcherManager* const watcher_manager =

[benwells, 2014/08/13 05:50:47]

Why do you get the watcher manager here?

[mtomasz, 2014/08/13 07:02:09]

Not needed. Removed.

+      context->GetWatcherManager(url.type());
+  if (!watcher_manager) {
+    callback.Run(base::File::FILE_ERROR_SECURITY);
+    return;
+  }
+
+  // TODO(mtomasz): Save in preferences.
+
+  callback.Run(base::File::FILE_OK);
+}
+
+EntryWatcherService::EntryWatcher::EntryWatcher()
+    : directory(false), recursive(false) {
+}
+
+EntryWatcherService::EntryWatcher::EntryWatcher(
+    const fileapi::FileSystemURL& url,
+    bool directory,
+    bool recursive)
+    : url(url), directory(directory), recursive(recursive) {
+}
+
+EntryWatcherService::EntryWatcher::~EntryWatcher() {
+}
+
+// static
+EntryWatcherService* EntryWatcherServiceFactory::Get(
+    content::BrowserContext* context) {
+  return static_cast<EntryWatcherService*>(
+      GetInstance()->GetServiceForBrowserContext(context, true));
+}
+
+// static
+EntryWatcherService* EntryWatcherServiceFactory::FindExisting(
+    content::BrowserContext* context) {
+  return static_cast<EntryWatcherService*>(
+      GetInstance()->GetServiceForBrowserContext(context, false));
+}
+
+EntryWatcherServiceFactory* EntryWatcherServiceFactory::GetInstance() {
+  return Singleton<EntryWatcherServiceFactory>::get();
+}
+
+EntryWatcherServiceFactory::EntryWatcherServiceFactory()
+    : BrowserContextKeyedServiceFactory(
+          "EntryWatcherService",
+          BrowserContextDependencyManager::GetInstance()) {
+}
+
+EntryWatcherServiceFactory::~EntryWatcherServiceFactory() {
+}
+
+KeyedService* EntryWatcherServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new EntryWatcherService(Profile::FromBrowserContext(context));
+}
+
+bool EntryWatcherServiceFactory::ServiceIsCreatedWithBrowserContext() const {
+  return true;

[benwells, 2014/08/13 05:50:47]

Could you add a comment about why this is needed? I assume it is so you can
restore persistent watchers as soon as the profile is loaded.

[mtomasz, 2014/08/13 07:02:09]

Done.

+}
+
+}  // namespace extensions