[ew] Add basic classes.

chrome/browser/file_system/entry_watcher_service.h

+// 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.
+
+#ifndef CHROME_BROWSER_FILE_SYSTEM_ENTRY_WATCHER_SERVICE_H_
+#define CHROME_BROWSER_FILE_SYSTEM_ENTRY_WATCHER_SERVICE_H_
+
+#include <map>
+#include <set>

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

Nit: <set> doesn't appear to be used.

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

Done.

+#include <string>
+#include <vector>
+
+#include "base/memory/singleton.h"

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

Super nit: you can forward declare "template <typename T> struct
DefaultSingletonTraits;" instead of including this.

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

Our style guide says that for class templates, #includes are preferred. Not sure
if struct are considered as classes though.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...

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

Ah, good to know.

+#include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "webkit/browser/fileapi/file_system_url.h"
+#include "webkit/browser/fileapi/watcher_manager.h"
+
+namespace content {
+class BrowserContext;
+}  // namespace content
+
+namespace extensions {

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

If this isn't in the extensions folder it shouldn't be in the extensions
namespace.

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

Good point. It was there in the past, but I moved it later. Removed.

+
+struct Event;
+class EventRouter;
+
+// Watches entries (files and directories) for changes. Created per profile.
+// TODO(mtomasz): Add support for watching files.
+class EntryWatcherService : public KeyedService,
+                            public fileapi::WatcherManager::Observer {
+ public:
+  typedef base::Callback<
+      bool(const std::string& extension_id, scoped_ptr<Event> event)>
+      DispatchEventImplCallback;
+
+  typedef base::Callback<fileapi::FileSystemContext*(
+      const std::string& extension_id,
+      content::BrowserContext* context)> GetFileSystemContextImplCallback;
+
+  typedef base::Callback<void(std::vector<fileapi::FileSystemURL>)>
+      GetWatchedEntriesCallback;

[tzik, 2014/08/13 09:21:49]

Not used? / Can this argument be const ref?

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

^^^

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

Done. Missed this one.

+
+  explicit EntryWatcherService(content::BrowserContext* context);
+  virtual ~EntryWatcherService();
+
+  // Watches a directory. Only one watcher can be set per the same |url| and
+  // |extension_id|.
+  void WatchDirectory(const std::string& extension_id,
+                      const fileapi::FileSystemURL& url,
+                      bool recursive,
+                      const fileapi::WatcherManager::StatusCallback& callback);
+
+  // Unwatches an entry (file or directory).
+  void UnwatchEntry(const std::string& extension_id,
+                    const fileapi::FileSystemURL& url,
+                    const fileapi::WatcherManager::StatusCallback& callback);
+
+  std::vector<fileapi::FileSystemURL> GetWatchedEntries(
+      const std::string& extension_id);
+
+  // fileapi::WatcherManager::Observer overrides.
+  virtual void OnEntryChanged(const fileapi::FileSystemURL& url) OVERRIDE;
+  virtual void OnEntryRemoved(const fileapi::FileSystemURL& url) OVERRIDE;
+
+  // Sets a custom dispatcher for tests in order to be able to verify dispatched
+  // events.
+  void SetDispatchEventImplForTesting(
+      const DispatchEventImplCallback& callback);
+
+  // Sets a custom context getter for tests in order to inject a testing
+  // file system context implementation.
+  void SetGetFileSystemContextImplForTesting(
+      const GetFileSystemContextImplCallback& callback);
+
+ private:
+  // Holds an information about an entry watcher.

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

Nit: Just "Holds information"

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

Done.

+  struct EntryWatcher {
+    EntryWatcher();
+    EntryWatcher(const fileapi::FileSystemURL& url,
+                 bool directory,
+                 bool recursive);
+    ~EntryWatcher();
+
+    fileapi::FileSystemURL url;

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

Nit: Could these fields all be const? Or do you change them after creating the
struct?

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

We don't modify them directly, but with const they can't be used in stl
containers.

+    bool directory;
+    bool recursive;
+  };
+
+  typedef std::map<GURL, std::map<std::string, EntryWatcher> > WatcherMap;

[tzik, 2014/08/13 09:21:49]

Can this be FileSystemURL with FileSystemURL::Comparator?

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

Good point. Done.

+
+  // Called when adding a directory watcher is completed with either a success
+  // or an error.
+  void OnWatchDirectoryCompleted(
+      const std::string& extension_id,
+      const fileapi::FileSystemURL& url,
+      bool recursive,
+      const fileapi::WatcherManager::StatusCallback& callback,
+      base::File::Error result);
+
+  // Called when removing a watcher is completed with either a success or an
+  // error.
+  void OnUnwatchEntryCompleted(
+      const std::string& extension_id,
+      const fileapi::FileSystemURL& url,
+      const fileapi::WatcherManager::StatusCallback& callback,
+      base::File::Error result);
+
+  content::BrowserContext* context_;
+  WatcherMap watchers_;

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

Nit: Could you comment (here or at the typedef) what the GURL and string are
for?

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

Done.

+  DispatchEventImplCallback dispatch_event_impl_;
+  GetFileSystemContextImplCallback get_file_system_context_impl_;
+  ScopedObserver<fileapi::WatcherManager, fileapi::WatcherManager::Observer>
+      observing_;
+  base::WeakPtrFactory<EntryWatcherService> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(EntryWatcherService);
+};
+
+class EntryWatcherServiceFactory : public BrowserContextKeyedServiceFactory {

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

This isn't always done, but often factories are kept in a separate file their
services. This means things that just need the factory don't also depend on the
service and visa versa, and IMHO is cleaner. Could you do that?

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

Done.

+ public:
+  // Returns a service instance singleton, after creating it (if necessary).

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

Nit: it isn't a singleton.

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

Singleton per context?

+  static EntryWatcherService* Get(content::BrowserContext* context);
+
+  // Returns a service instance for the context if exists. Otherwise, returns
+  // NULL.
+  static EntryWatcherService* FindExisting(content::BrowserContext* context);
+
+  // Gets a singleton instance of the factory.
+  static EntryWatcherServiceFactory* GetInstance();
+
+ private:
+  friend struct DefaultSingletonTraits<EntryWatcherServiceFactory>;
+
+  EntryWatcherServiceFactory();
+  virtual ~EntryWatcherServiceFactory();
+
+  // BrowserContextKeyedServiceFactory overrides:
+  virtual KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* profile) const OVERRIDE;
+  virtual bool ServiceIsCreatedWithBrowserContext() const OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(EntryWatcherServiceFactory);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_FILE_SYSTEM_ENTRY_WATCHER_SERVICE_H_