Extend the capability of SystemMonitor to support watching storage free space change

base/system_monitor/system_monitor.h

Index: base/system_monitor/system_monitor.h
diff --git a/base/system_monitor/system_monitor.h b/base/system_monitor/system_monitor.h
index 48d512b708c11aa44368c05300dd9a38f29e00ed..40a1e3e8126e089dc64010e0844f8bcbb5da8e42 100644
--- a/base/system_monitor/system_monitor.h
+++ b/base/system_monitor/system_monitor.h
@@ -6,6 +6,7 @@
 #define BASE_SYSTEM_MONITOR_SYSTEM_MONITOR_H_
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -13,6 +14,7 @@
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/string16.h"
+#include "base/synchronization/lock.h"

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

Your other CL was committed, so you need to rebase this change.

 #include "build/build_config.h"
 
 // Windows HiRes timers drain the battery faster so we need to know the battery
@@ -58,6 +60,26 @@ class BASE_EXPORT SystemMonitor {
     DEVTYPE_UNKNOWN,  // Other devices.
   };
 
+  // The delegate interfaces for storage free space change notification.
+  // Since the client code may call these interfaces from any thread, the

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

I intended for you to save the thread id in delegate set and then post to that
thread in start/stop

[Hongbo Min, 2012/09/14 06:04:36]

Actually, the base code is not allowed to access BrowserThread to get the thread
ID and post a task to the specific thread.

Now since the delegate set will be moved to ctor. Nothing to do for this.

[vandebo (ex-Chrome), 2012/09/14 08:07:18]

No, but it is allowed to look at the thread ID.  Look at how
ObserverListThreadSafe is implemented.
 
I'm not sure that's the case... A consumer can still start watching free space
from any thread.  We should call the delegate from the thread that the
constructor was called on.

+  // implementation must take care to implement it, e.g. if it is not called
+  // from the thread on which the observation is working, you probably need
+  // to post a task to that thread for starting or stopping the watching
+  // operation.
+  class BASE_EXPORT StorageFreeSpaceDelegate {
+   public:
+    virtual ~StorageFreeSpaceDelegate() {}
+
+    // Start watching the storage device identified by the |path| parameter.
+    // The |path| can be a drive path on Windows, or the mount point on Posix.
+    // It can be called from any thread.
+    virtual void StartWatchingStorage(const FilePath& path) = 0;
+
+    // Stop watching the storage device identified by the |path| paramter. It
+    // can be called from any thread.
+    virtual void StopWatchingStorage(const FilePath& path) = 0;
+  };
+
   struct BASE_EXPORT RemovableStorageInfo {
     RemovableStorageInfo();
     RemovableStorageInfo(const std::string& id,
@@ -147,17 +169,33 @@ class BASE_EXPORT SystemMonitor {
     virtual ~DevicesChangedObserver() {}
   };
 
+  class BASE_EXPORT StorageFreeSpaceObserver {
+   public:
+    // Triggered when the free space of the storage |path| is changed. The
+    // |available_capacity| indicates the available capacity in bytes.
+    virtual void OnFreeSpaceChanged(const FilePath& path,
+                                    int64 available_capacity) {}

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

Shouldn't this be a uint64?

+
+    // Triggered when the free space delegate failed to start watching
+    // the given storage |path|.
+    virtual void OnFreeSpaceWatchingError(const FilePath& path) {}

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

Maybe OnFreeSpaceChangeError()?  What do you think?

+  };
+
   // Add a new observer.
   // Can be called from any thread.
   // Must not be called from within a notification callback.
   void AddPowerObserver(PowerObserver* obs);
   void AddDevicesChangedObserver(DevicesChangedObserver* obs);
+  void AddStorageFreeSpaceObserver(const FilePath& path,
+                                   StorageFreeSpaceObserver* obs);
 
   // Remove an existing observer.
   // Can be called from any thread.
   // Must not be called from within a notification callback.
   void RemovePowerObserver(PowerObserver* obs);
   void RemoveDevicesChangedObserver(DevicesChangedObserver* obs);
+  void RemoveStorageFreeSpaceObserver(const FilePath& path,
+                                      StorageFreeSpaceObserver* obs);
 
 #if defined(OS_WIN)
   // Windows-specific handling of a WM_POWERBROADCAST message.
@@ -176,10 +214,32 @@ class BASE_EXPORT SystemMonitor {
                                        const FilePath::StringType& location);
   void ProcessRemovableStorageDetached(const std::string& id);
 
+  // Cross-platform handling of free space change event. The parameter
+  // |available_capacity| is valid only if the |error| is false.
+  void ProcessStorageFreeSpaceMessage(const FilePath& path,

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

Because of the pattern for other things in the file, I think it's better to have
two different methods:

ProcessStorageFreeSpaceChanged(const FilePath& path, uint64 available_capacity);
ProcessStorageFreeSpaceError(const FilePath& path);

+                                      int64 available_capacity,
+                                      bool error);
+
+  StorageFreeSpaceDelegate* storage_free_space_delegate() const {

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

I don't think you really need this method.

+    return free_space_delegate_;
+  }
+
+  // Called on the thread that starts and stops watching storage paths.
+  void set_storage_free_space_delegate(StorageFreeSpaceDelegate* delegate);

[willchan no longer on Chromium, 2012/09/13 18:41:14]

Ah, this is interesting. Please correct me if I'm wrong, but you've got this
delegate here, since you want to implement it in Chrome, not base, which is the
right thing to do in terms of layering. The thing is, the generation of the
notifications doesn't have to be in the base interface. This interface is for
registering for and receiving notifications.

What do you think about defining some sort of virtual private interface so that
subclasses can generate notifications for the things you want delegated to a
different layer? You could just take the existing StorageFreeSpaceDelegate and
just make those member functions virtual private. Then a ChromeSystemMonitor
could implement those member functions.

[vandebo (ex-Chrome), 2012/09/13 20:08:41]

Yes, the various parts of system monitor are implemented in different layers...
some in chrome, some in content.  Also, the existing structure of SystemMonitor
was to have ProcessFoo methods that notify the observer.  It was that way the
first time I looked at SystemMonitor and I'm not sure of all the reasons for
that.
If I understand correctly, you mean that the instance returned from Get() would
be
ChromeSystemMonitor?  We'd of course have to add a method to set the global
instance.
But my deeper concern is the uniformity of it; why shouldn't all the aspect of
SystemMonitor be implemented in ChromeSystemMonitor or ContentSystemMonitor,
which
would have to layer upon each other?  It seems like a mess to orchestrate that
and
having the loosely coupled implementation classes is easier to manage.

[willchan no longer on Chromium, 2012/09/13 20:24:33]

Not needed. The setter is the constructor of SystemMonitor, which any subclass
would need to call.
Interesting, I didn't know about part of stuff being in content too. I guess I
was just paying attention to it not being in base/. I'm going to label your
concern as roughly being the fragility of implementation inheritance. I totally
agree with that. I was trying not to ask for a huge refactor of SystemMonitor,
but a more incremental change. I totally think that now that SystemMonitor's
implementation is not all in base/, it makes more sense to split out the
interface and the implementation, with implementation classes that can be
composed rather than inherited.

The main thing I object to is the setter for the delegate. This is roughly
analogous to our setter for the global SystemMonitor instance, which is the
constructor. Since you can't doubly instantiate a SystemMonitor, you can't
misconfigure it. If we add a threadunsafe setter that we can't lock down after
Chrome is up and running, then that's a weakness in the API. All the other
non-const operations are threadsafe because they just register/unregister
observers on specific threads.

+
  private:
   // Mapping of unique device id to device info tuple.
   typedef std::map<std::string, RemovableStorageInfo> RemovableStorageMap;
 
+  typedef ObserverListThreadSafe<StorageFreeSpaceObserver>
+      StorageFreeSpaceObserverList;
+  // Mapping of the storage device path to the observer list.
+  typedef std::map<FilePath, scoped_refptr<StorageFreeSpaceObserverList> >
+      StorageFreeSpaceObserverListMap;
+  // Multi-mapping of the storage path to its observer.
+  typedef std::multimap<FilePath, StorageFreeSpaceObserver*>

[vandebo (ex-Chrome), 2012/09/13 19:57:34]

Using a multimap, you've almost accomplished what I asked for, but in a much
more complicated way.  You have to iterate through all the path entries to find
the observer to see if it exists.  And you don't do this consistently in the
code.  It will be less code to use a std::set with a struct that contains both
FilePath and StorageFreeSpaceObserver*.

[Hongbo Min, 2012/09/14 06:04:36]

I don't think std::set can reduce the code complexity. Using std::set also can't
avoid iteration through all the path entries, because when a (path, observer)
pair is removed, we need to iterate the set to check whether the path is still
observed by somebody. If no, stop watching it.

[vandebo (ex-Chrome), 2012/09/14 08:07:18]

Indeed, but using std::map<FilePath, std::set<StorageSpaceObserver*> > will
avoid having to walk through stuff.  And since you already have a map with
FilePath as a key, might as well put the two values into a struct...
struct PathObserver {
  scoped_refptr<StorageFreeSpaceObserverList> observer_list;
  std::set<StorageFreeSpaceObserver*> observers;
};
std::map<FilePath, PathObserver>

+      StorageFreeSpaceObserverMap;
+
 #if defined(OS_MACOSX)
   void PlatformInit();
   void PlatformDestroy();
@@ -204,6 +264,16 @@ class BASE_EXPORT SystemMonitor {
   void NotifySuspend();
   void NotifyResume();
 
+  // Notify the |observers| observing the given |path| for free space change
+  // event.
+  void NotifyStorageFreeSpaceChanged(
+      scoped_refptr<StorageFreeSpaceObserverList> observers,
+      const FilePath& path,
+      int64 available_capacity);
+  void NotifyStorageFreeSpaceWatchingError(
+      scoped_refptr<StorageFreeSpaceObserverList> observers,
+      const FilePath& path);
+
   scoped_refptr<ObserverListThreadSafe<PowerObserver> > power_observer_list_;
   scoped_refptr<ObserverListThreadSafe<DevicesChangedObserver> >
       devices_changed_observer_list_;
@@ -222,6 +292,22 @@ class BASE_EXPORT SystemMonitor {
   // Map of all the attached removable storage devices.
   RemovableStorageMap removable_storage_map_;
 
+  // The delegate that implements free space change notification.
+  StorageFreeSpaceDelegate* free_space_delegate_;
+
+  // Used to map the storage path being watched to its observer list.
+  StorageFreeSpaceObserverListMap free_space_observer_list_map_;
+
+  // Used to map the storage path and its associated observer. We use multimap
+  // because a path may be observed by more than one observer. It helps decide
+  // when to stop watching a storage path since we can't know when the thread
+  // safe observer list becomes empty.
+  StorageFreeSpaceObserverMap free_space_observer_map_;
+
+  // A lock used to synchronize access to |free_space_observer_list_map_| and
+  // |free_space_observer_map_| data.
+  base::Lock free_space_observers_lock_;
+
   DISALLOW_COPY_AND_ASSIGN(SystemMonitor);
 };