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..12f6f5ed4b8102883acc0ac82bb134d418891bf4 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>
 
@@ -58,6 +59,20 @@ class BASE_EXPORT SystemMonitor {
     DEVTYPE_UNKNOWN,  // Other devices.
   };
 
+  // The notification interfaces for storage free space.
+  class BASE_EXPORT StorageFreeSpaceNotification {

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

This isn't a notification interface so much as a delegate:

-> StorageFreeSpaceDelegate

[Hongbo Min, 2012/09/12 09:24:31]

Done.

+   public:
+    virtual ~StorageFreeSpaceNotification() {}
+
+    // Start watching the storage device identified by the |path| parameter.
+    // Return true if it succeeds to start, otherwise, false is returned.
+    // The |path| can be a drive path on Windows, or the mount point on Linux.
+    virtual bool StartWatchingStorage(const FilePath::StringType& path) = 0;

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

Can you actually do anything if it fails to start?

[Hongbo Min, 2012/09/12 09:24:31]

My initial thought is, if it failed to start watching when the first observer is
going to be added, for example, the given path is not a valid file path, the
observer won't be added into the observer list, and we try to start it again
when another observer is going to be added next time.

But such a design may have a problem if the start watching is posted to another
thread, e.g. FILE thread, it is an async calling and should no return value.

To solve this problem, 

1) bool->void for Start/Stop method;
1) Add a new API called IsWatchingStorage. The delegate implementation allows
its consumer to query whether the given path is in watching state.

[vandebo (ex-Chrome), 2012/09/12 17:54:47]

I can think of two approaches to dealing with errors:
1) Simply document that invalid paths will never receive notifications and add a
DCHECK that it never fails in the implementation.
2) Plumb failures all the way through.  That means that
StorageFreeSpaceChangedObserver should have a method to indicate that the
implementation failed to start watching. and you'll need a method
ProcessStorageFreeSpaceStartFailed.  With these two methods, you start assuming
that it will work and then if it fails, the implementation notifies
SystemMonitor, which cleans up state and notifies the consumer(s).

+
+    // Stop watching the storage device identified by the |path| paramter.
+    virtual bool StopWatchingStorage(const FilePath::StringType& path) = 0;

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

I doubt there's anything to do if it fails to stop: bool -> void

[Hongbo Min, 2012/09/12 09:24:31]

Done. See comment above.

+  };
+
   struct BASE_EXPORT RemovableStorageInfo {
     RemovableStorageInfo();
     RemovableStorageInfo(const std::string& id,
@@ -147,17 +162,27 @@ class BASE_EXPORT SystemMonitor {
     virtual ~DevicesChangedObserver() {}
   };
 
+  class BASE_EXPORT StorageFreeSpaceChangedObserver {
+   public:
+    // Triggered when the free space of the storage |path| is changed.
+    virtual void OnStorageFreeSpaceChanged(const FilePath::StringType& path) {}

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

You've used FilePath::StringType throughout your change in this file, it should
just be FilePath.  Removable storage location is a special case -- we're going
to have non VFS devices (MTP/PTP).

[Hongbo Min, 2012/09/12 09:24:31]

Now I use FilePath instead, but actually a FilePath is always constructed from a
FilePath::StringType object, I am wondering why the MTP/PTP device
representation can benefit from it?

[vandebo (ex-Chrome), 2012/09/12 17:54:47]

An MTP device may not be a valid file path to read from like /dev/usb5 or
\\unc\foobar so we don't want people to think they can use it like a regular
file path.

[Hongbo Min, 2012/09/13 13:48:14]

One more question, if we don't want to represent the MTP device like a regular
file path, FilePath::StringType seems to be more reasonable than FilePath,
because the FilePath::StringType is just a string type (std::string or
std::wstring) which can also be used to represent a MTP device, however, the
FilePath implies it is a regular file path logically, even for MTP/PTP device.

+  };
+
   // 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 AddStorageFreeSpaceChangedObserver(const FilePath::StringType& path,
+                                     StorageFreeSpaceChangedObserver* obs);

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

nit: Stor... should line up with const on the previous line.

[Hongbo Min, 2012/09/12 09:24:31]

Done.

 
   // 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 RemoveStorageFreeSpaceChangedObserver(const FilePath::StringType& path,
+                                        StorageFreeSpaceChangedObserver* obs);

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

ditto

[Hongbo Min, 2012/09/12 09:24:31]

Done.

 
 #if defined(OS_WIN)
   // Windows-specific handling of a WM_POWERBROADCAST message.
@@ -176,10 +201,26 @@ class BASE_EXPORT SystemMonitor {
                                        const FilePath::StringType& location);
   void ProcessRemovableStorageDetached(const std::string& id);
 
+  void ProcessStorageFreeSpaceChanged(const FilePath::StringType& path);
+
+  void set_storage_free_space_notification(

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

notification -> delegate

[Hongbo Min, 2012/09/12 09:24:31]

Done.

+      StorageFreeSpaceNotification* notification) {
+    free_space_notification_ = notification;
+  }
+
  private:
   // Mapping of unique device id to device info tuple.
   typedef std::map<std::string, RemovableStorageInfo> RemovableStorageMap;
 
+  typedef ObserverListThreadSafe<StorageFreeSpaceChangedObserver>
+      StorageFreeSpaceChangedObserverList;
+
+  // Mapping of the storage device path to the observer list.
+  typedef std::map<FilePath::StringType,
+      scoped_refptr<StorageFreeSpaceChangedObserverList> >
+          StorageFreeSpaceChangedObserverMap;
+
+
 #if defined(OS_MACOSX)
   void PlatformInit();
   void PlatformDestroy();
@@ -222,6 +263,13 @@ class BASE_EXPORT SystemMonitor {
   // Map of all the attached removable storage devices.
   RemovableStorageMap removable_storage_map_;
 
+  // The notification of storage free space change.
+  StorageFreeSpaceNotification* free_space_notification_;
+  // Map of the storage being watched to the observer list.
+  StorageFreeSpaceChangedObserverMap free_space_changed_observer_map_;

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

Editing a std::map is not thread safe.  This is actually a bug with
removable_stroage_map_ as well, but I just noticed it yesterday and haven't
fixed it yet.  You'll need need to add a lock to guard access to the map.

[Hongbo Min, 2012/09/12 09:24:31]

Done.

[Hongbo Min, 2012/09/12 14:07:32]

I try to fix it and the patch is uploaded to
http://codereview.chromium.org/10905237/ for reviewing. Pls have a review.

[vandebo (ex-Chrome), 2012/09/12 17:54:47]

Thanks for doing this.

+  // The set of the storage path being watched.
+  std::multiset<FilePath::StringType> watching_storage_set_;

[vandebo (ex-Chrome), 2012/09/11 22:49:27]

You've be susceptible to misuse with a multiset.  How about a set<pair<FilePath,
 StorageFreeSpaceChangedObserver*> >?  But use a struct instead of a pair.

[Hongbo Min, 2012/09/12 09:24:31]

The multiset usage is to work around the limitation that ObserverListThreadSafe
doesn't provide an ability to allow its consumer to check if it becomes empty.
We need to stop watching a storage path if its observer list becomes empty, so
the multiset usage is coming in the way.

Is there any better approach for it?

[vandebo (ex-Chrome), 2012/09/12 17:54:47]

Yea, I see why you're using the multiset. What I was saying is that if there's a
bug in one of the consumers it can mess up the other consumers.  Like if it
calls stop twice and there were two consumers, notificiations would stop for the
other consumer.

struct FreeSpaceWatcher {
  FilePath path;
  StorageFreeSpaceChangedObserver* observer;
};

std::set<FreSpaceWatcher> watched_storage_;

This data structure requires a consumer to pass the same observer for start and
stop, but makes it so that a double stop (or double start) doesn't through the
counting off.

+
   DISALLOW_COPY_AND_ASSIGN(SystemMonitor);
 };