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

base/system_monitor/system_monitor.h

 // Copyright (c) 2012 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 BASE_SYSTEM_MONITOR_SYSTEM_MONITOR_H_
 #define BASE_SYSTEM_MONITOR_SYSTEM_MONITOR_H_
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/base_export.h"
 #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
 // status.  This isn't true for other platforms.
 #if defined(OS_WIN)
 #define ENABLE_BATTERY_MONITORING 1
 #else
 #undef ENABLE_BATTERY_MONITORING
 #endif  // !OS_WIN
 
@@ skipping 25 matching lines @@
     RESUME_EVENT        // The system is being resumed.
   };
 
   // Type of devices whose change need to be monitored, such as add/remove.
   enum DeviceType {
     DEVTYPE_AUDIO_CAPTURE,  // Audio capture device, e.g., microphone.
     DEVTYPE_VIDEO_CAPTURE,  // Video capture device, e.g., webcam.
     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,
                          const string16& device_name,
                          const FilePath::StringType& device_location);
 
     // Unique device id - persists between device attachments.
     std::string device_id;
 
     // Human readable removable storage device name.
@@ skipping 20 matching lines @@
   static void AllocateSystemIOPorts();
 #else
   static void AllocateSystemIOPorts() {}
 #endif  // OS_IOS
 #endif  // OS_MACOSX
 
   // Returns information for attached removable storage.
   std::vector<RemovableStorageInfo> GetAttachedRemovableStorage() const;
 
   //
   // Power-related APIs

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

It looks like a mix of trying to group by API type and group according to the
style guide. Let's stick with the style guide. If you guys continue to make more
SystemMonitor changes, can you guys re-order things according to the style guide
in a followup changelist? Roughly speaking, types first, then functions, then
member variables, blah blah.

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

Added to my post M23 branch todo list.

   //
 
   // Is the computer currently on battery power.
   // Can be called on any thread.
   bool BatteryPower() const {
     // Using a lock here is not necessary for just a bool.
     return battery_in_use_;
   }
 
   // Callbacks will be called on the thread which creates the SystemMonitor.
@@ skipping 28 matching lines @@
     virtual void OnRemovableStorageAttached(
         const std::string& id,
         const string16& name,
         const FilePath::StringType& location) {}
     virtual void OnRemovableStorageDetached(const std::string& id) {}
 
    protected:
     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.
   // Embedders of this API should hook their top-level window
   // message loop and forward WM_POWERBROADCAST through this call.
   void ProcessWmPowerBroadcastMessage(int event_id);
 #endif
 
   // Cross-platform handling of a power event.
   void ProcessPowerMessage(PowerEvent event_id);
 
   // Cross-platform handling of a device change event.
   void ProcessDevicesChanged(DeviceType device_type);
   void ProcessRemovableStorageAttached(const std::string& id,
                                        const string16& name,
                                        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();
 #endif
 
   // Platform-specific method to check whether the system is currently
   // running on battery power.  Returns true if running on batteries,
   // false otherwise.
   bool IsBatteryPower();
 
   // Checks the battery status and notifies observers if the battery
   // status has changed.
   void BatteryCheck();
 
   // Functions to trigger notifications.
   void NotifyDevicesChanged(DeviceType device_type);
   void NotifyRemovableStorageAttached(const std::string& id,
                                       const string16& name,
                                       const FilePath::StringType& location);
   void NotifyRemovableStorageDetached(const std::string& id);
   void NotifyPowerStateChange();
   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_;
   bool battery_in_use_;
   bool suspended_;
 
 #if defined(ENABLE_BATTERY_MONITORING)
   base::OneShotTimer<SystemMonitor> delayed_battery_check_;
 #endif
 
 #if defined(OS_IOS)
   // Holds pointers to system event notification observers.
   std::vector<id> notification_observers_;
 #endif
 
   // 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);
 };
 
 }  // namespace base
 
 #endif  // BASE_SYSTEM_MONITOR_SYSTEM_MONITOR_H_