Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10917166:
Extend the capability of SystemMonitor to support watching storage free space change (Closed)
|
Created:
8 years, 3 months ago by Hongbo Min Modified:
8 years, 3 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, Mihai Parparita -not on Chrome, willchan no longer on Chromium Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionExtend the capability of SystemMonitor to support watching storage free space change.
BUG=141229
TEST=None
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 32
Patch Set 3 : Update patch according to vandebo's comments #Patch Set 4 : Rebase and add available_capacity into the observer callback #
Total comments: 24
Patch Set 5 : updated patch #
Total comments: 18
Patch Set 6 : Update and add unit test #Messages
Total messages: 40 (0 generated)
vandebo, this CL is to extend the capacity of SystemMonitor for watching storage free space change. See my embedded comments about the new interfaces. Please have a review. Thanks. BTW, where is the device notification for Mac platform? I failed to find it in the source tree. http://codereview.chromium.org/10917166/diff/1/base/system_monitor/system_mon... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/1/base/system_monitor/system_mon... base/system_monitor/system_monitor.h:204: vandebo, here I add these two methods for the client code to start and stop the free space watching process instead of implicitly start/stop when adding/removing device observers, as discuss in http://crbug.com/141229. The reason is, the SystemMonitor consumer may want to only watch a specific storage device, not all storage devices attached to the system, it is the responsibility of the client code to start/stop the watching process for the given storage. The implicit way can not satisfy this more-grained requirement.
+cc willchan@ On 2012/09/10 14:37:50, Hongbo Min wrote: > BTW, where is the device notification for Mac platform? I failed to find it in > the source tree. It's being written now: https://chromiumcodereview.appspot.com/10919185/ http://codereview.chromium.org/10917166/diff/1/base/system_monitor/system_mon... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/1/base/system_monitor/system_mon... base/system_monitor/system_monitor.h:62: // The delegate for watching the storage free space changes. If it starts This isn't for watching storage free space changes, this is for the implementation to know when to notify. Make the comment and class name more clear. http://codereview.chromium.org/10917166/diff/1/base/system_monitor/system_mon... base/system_monitor/system_monitor.h:204: On 2012/09/10 14:37:50, Hongbo Min wrote: > vandebo, here I add these two methods for the client code to start and stop the > free space watching process instead of implicitly start/stop when > adding/removing device observers, as discuss in http://crbug.com/141229. > > The reason is, the SystemMonitor consumer may want to only watch a specific > storage device, not all storage devices attached to the system, it is the > responsibility of the client code to start/stop the watching process for the > given storage. The implicit way can not satisfy this more-grained requirement. > I guess that's a fair point, but there's a lot of coordination here to make things happen correctly and it's a bit fragile.... For example, if you had two things that wanted to watch for free space, the first one to stop would call stop and the second one would stop getting notifications. Instead, lets give up on including free space changed as part of DevicesChangedObserver. So we'll have: class StorageFreeSpaceChangedObserver; with one method and two methods to register for it: AddStorageFreeSpaceChangedObserver(const FilePath::StringType& device, StorageFreeSpaceChangedObserver*); RemoveStorageFreeSpaceChangedObserver(const FilePath::StringType& device, StorageFreeSpaceChangedObserver*); This way there can be a dynamic set of observer lists and when one of them is empty, we'll tell the delegate to stop notifying about that one. http://codereview.chromium.org/10917166/diff/1/chrome/browser/system_monitor/... File chrome/browser/system_monitor/media_device_notifications_chromeos.cc (right): http://codereview.chromium.org/10917166/diff/1/chrome/browser/system_monitor/... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:165: // TODO(hongbo): Add the implementation for chromeos. Nothing will break if this isn't here, so don't add these until you have the implementation to add.
On 2012/09/10 21:52:49, vandebo wrote: > I guess that's a fair point, but there's a lot of coordination here to make > things happen correctly and it's a bit fragile.... For example, if you had two > things that wanted to watch for free space, the first one to stop would call > stop and the second one would stop getting notifications. in fact, in this case, the second one still can get notifications because the watching will not be stopped since the mulitset still contain the path string for the second one > > Instead, lets give up on including free space changed as part of > DevicesChangedObserver. So we'll have: > > class StorageFreeSpaceChangedObserver; with one method and two methods to > register for it: > AddStorageFreeSpaceChangedObserver(const FilePath::StringType& device, > StorageFreeSpaceChangedObserver*); > RemoveStorageFreeSpaceChangedObserver(const FilePath::StringType& device, > StorageFreeSpaceChangedObserver*); > > This way there can be a dynamic set of observer lists and when one of them is > empty, we'll tell the delegate to stop notifying about that one. > I do like the separate Observer for free space, it is actually a burden for the client to call start/stop, which somewhat like new/delete.
Patch is updated. Pls have a review.
http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.cc (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.cc:128: if (free_space_changed_observer_map_[path].get()) Use find(path) so that you don't add something to the map that was just deleted. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.cc:166: !free_space_notification_->StartWatchingStorage(path)) Make this two different if's to make it more readable. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.cc:174: free_space_changed_observer_map_[path]->AddObserver(obs); Once you have a lock, I think you'll want to add the observer before calling StartWatchingStorage http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:63: class BASE_EXPORT StorageFreeSpaceNotification { This isn't a notification interface so much as a delegate: -> StorageFreeSpaceDelegate http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:70: virtual bool StartWatchingStorage(const FilePath::StringType& path) = 0; Can you actually do anything if it fails to start? http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:73: virtual bool StopWatchingStorage(const FilePath::StringType& path) = 0; I doubt there's anything to do if it fails to stop: bool -> void http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:168: virtual void OnStorageFreeSpaceChanged(const FilePath::StringType& path) {} 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). http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:177: StorageFreeSpaceChangedObserver* obs); nit: Stor... should line up with const on the previous line. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:185: StorageFreeSpaceChangedObserver* obs); ditto http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:206: void set_storage_free_space_notification( notification -> delegate http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:269: StorageFreeSpaceChangedObserverMap free_space_changed_observer_map_; 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. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:271: std::multiset<FilePath::StringType> watching_storage_set_; You've be susceptible to misuse with a multiset. How about a set<pair<FilePath, StorageFreeSpaceChangedObserver*> >? But use a struct instead of a pair. http://codereview.chromium.org/10917166/diff/6001/chrome/browser/system_monit... File chrome/browser/system_monitor/media_device_notifications_chromeos.cc (right): http://codereview.chromium.org/10917166/diff/6001/chrome/browser/system_monit... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:45: MediaDeviceNotifications::MediaDeviceNotifications() { In the construction of each implementation you need to set the free space delegate in SystemMonitor
vandebo, the patch is updated. Pls review it again. Thanks. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.cc (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.cc:128: if (free_space_changed_observer_map_[path].get()) On 2012/09/11 22:49:27, vandebo wrote: > Use find(path) so that you don't add something to the map that was just deleted. Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.cc:166: !free_space_notification_->StartWatchingStorage(path)) On 2012/09/11 22:49:27, vandebo wrote: > Make this two different if's to make it more readable. Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.cc:174: free_space_changed_observer_map_[path]->AddObserver(obs); On 2012/09/11 22:49:27, vandebo wrote: > Once you have a lock, I think you'll want to add the observer before calling > StartWatchingStorage It can avoid the lock occupied too much time in case of the StartWatchingStorage is a time-consuming operation, right? For StartWatchingStorage calling, it starts out of the lock scope since IsWatchingStorage is used. But for StopWatchingStorage, the calling timing is when the observer list for a storage path becomes empty, it also implies thread synchronization arose from the observer mpa and can not move out of the lock scope to avoid multi-thread issue, e.g. in case of stop watching in one thread but it already starts watching again in another thread. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:63: class BASE_EXPORT StorageFreeSpaceNotification { On 2012/09/11 22:49:27, vandebo wrote: > This isn't a notification interface so much as a delegate: > > -> StorageFreeSpaceDelegate Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:70: virtual bool StartWatchingStorage(const FilePath::StringType& path) = 0; On 2012/09/11 22:49:27, vandebo wrote: > Can you actually do anything if it fails to start? 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. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:73: virtual bool StopWatchingStorage(const FilePath::StringType& path) = 0; On 2012/09/11 22:49:27, vandebo wrote: > I doubt there's anything to do if it fails to stop: bool -> void Done. See comment above. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:168: virtual void OnStorageFreeSpaceChanged(const FilePath::StringType& path) {} On 2012/09/11 22:49:27, vandebo wrote: > 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). 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? http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:177: StorageFreeSpaceChangedObserver* obs); On 2012/09/11 22:49:27, vandebo wrote: > nit: Stor... should line up with const on the previous line. Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:185: StorageFreeSpaceChangedObserver* obs); On 2012/09/11 22:49:27, vandebo wrote: > ditto Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:206: void set_storage_free_space_notification( On 2012/09/11 22:49:27, vandebo wrote: > notification -> delegate Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:269: StorageFreeSpaceChangedObserverMap free_space_changed_observer_map_; On 2012/09/11 22:49:27, vandebo wrote: > 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. Done. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:271: std::multiset<FilePath::StringType> watching_storage_set_; On 2012/09/11 22:49:27, vandebo wrote: > You've be susceptible to misuse with a multiset. How about a set<pair<FilePath, > StorageFreeSpaceChangedObserver*> >? But use a struct instead of a pair. 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? http://codereview.chromium.org/10917166/diff/6001/chrome/browser/system_monit... File chrome/browser/system_monitor/media_device_notifications_chromeos.cc (right): http://codereview.chromium.org/10917166/diff/6001/chrome/browser/system_monit... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:45: MediaDeviceNotifications::MediaDeviceNotifications() { On 2012/09/11 22:49:27, vandebo wrote: > In the construction of each implementation you need to set the free space > delegate in SystemMonitor Done.
If you have already fixed the thread safe issue you mentioned, please ignore it. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:269: StorageFreeSpaceChangedObserverMap free_space_changed_observer_map_; On 2012/09/11 22:49:27, vandebo wrote: > 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. I try to fix it and the patch is uploaded to http://codereview.chromium.org/10905237/ for reviewing. Pls have a review.
It seems there are a lot of details to get right. We're getting closer though. We will need to add a unit test, especially to ensure that everything happens on the right thread. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:70: virtual bool StartWatchingStorage(const FilePath::StringType& path) = 0; On 2012/09/12 09:24:31, Hongbo Min wrote: > On 2012/09/11 22:49:27, vandebo wrote: > > Can you actually do anything if it fails to start? > > 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. 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). http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:168: virtual void OnStorageFreeSpaceChanged(const FilePath::StringType& path) {} On 2012/09/12 09:24:31, Hongbo Min wrote: > On 2012/09/11 22:49:27, vandebo wrote: > > 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). > > 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? 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. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:269: StorageFreeSpaceChangedObserverMap free_space_changed_observer_map_; On 2012/09/12 14:07:32, Hongbo Min wrote: > On 2012/09/11 22:49:27, vandebo wrote: > > 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. > > I try to fix it and the patch is uploaded to > http://codereview.chromium.org/10905237/ for reviewing. Pls have a review. Thanks for doing this. http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:271: std::multiset<FilePath::StringType> watching_storage_set_; On 2012/09/12 09:24:31, Hongbo Min wrote: > On 2012/09/11 22:49:27, vandebo wrote: > > You've be susceptible to misuse with a multiset. How about a > set<pair<FilePath, > > StorageFreeSpaceChangedObserver*> >? But use a struct instead of a pair. > > 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? 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. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... File base/system_monitor/system_monitor.cc (right): http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:129: if (ContainsKey(free_space_changed_observer_map_, path) && This needs to be locked. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:129: if (ContainsKey(free_space_changed_observer_map_, path) && Use find so you only look up the path once. (elsewhere too) http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:170: new StorageFreeSpaceChangedObserverList(); nit: Omit () when using new with a constructor that has zero arguments. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:177: free_space_delegate_->StartWatchingStorage(path); Hmm, consumers can call AddStorageFreeSpaceChangedObserver from any thread. We should probably record the thread that the delegate was set on and then post a task to that thread whenever we invoke a method of the delegate. We should also document that behavior in a comment in the delegate definition. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:69: virtual bool IsWatchingStorage(const FilePath& path) = 0; Not sure you need this. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:72: // The |path| can be a drive path on Windows, or the mount point on Linux. nit: Linux -> Posix http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:215: free_space_delegate_ = delegate; Add a DCHECK that free_sapce_delegate_ is NULL. We only expect the delegate to be set once. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:271: // The delegate instance for storage free space change. nit: The delegate that implements free space-change notification. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:277: // The set of the storage path being watched. Since we can not know when nit: path -> paths, can not -> can't http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:283: // The lock for manipulating free_space_changed_observer_map_ nit: remove The: "Lock for..." http://codereview.chromium.org/10917166/diff/3023/chrome/browser/system_monit... File chrome/browser/system_monitor/media_device_notifications_chromeos.cc (right): http://codereview.chromium.org/10917166/diff/3023/chrome/browser/system_monit... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:50: base::SystemMonitor::Get()->set_storage_free_space_delegate(this); Since free space observer isn't really implemented in these classes yet. Add a comment to the delegate set code that it needs to be called on the thread that will be starting and stopping observation. http://codereview.chromium.org/10917166/diff/3023/chrome/browser/system_monit... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:54: if (base::SystemMonitor::Get()) Since we're removing the delegate here, we'll need to check that the delegate isn't null before each use in SystemMonitor.
vandebo, the updated patch has a heavily refactor now according to your comments, including documentation. Regarding to the failure of StartWatchingStorage handling, I adopt the second approach as you mentioned. I think it can make the free space delegate interface gets a good completeness. Regarding to the set of the free space watcher s, I found the std::multimap can meet our requirements very well. So I use it directly instead of introducing a new struct type to hold the (path, observer) pair. Regarding to the thread for running StartWatching/StopWatching, it may be the FILE thread, but depends on the implementation. If it is good to you, I will add unit test for it later. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... File base/system_monitor/system_monitor.cc (right): http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:129: if (ContainsKey(free_space_changed_observer_map_, path) && On 2012/09/12 17:54:47, vandebo wrote: > This needs to be locked. Done. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:129: if (ContainsKey(free_space_changed_observer_map_, path) && On 2012/09/12 17:54:47, vandebo wrote: > This needs to be locked. Done. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:170: new StorageFreeSpaceChangedObserverList(); On 2012/09/12 17:54:47, vandebo wrote: > nit: Omit () when using new with a constructor that has zero arguments. Done. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.cc:177: free_space_delegate_->StartWatchingStorage(path); On 2012/09/12 17:54:47, vandebo wrote: > Hmm, consumers can call AddStorageFreeSpaceChangedObserver from any thread. We > should probably record the thread that the delegate was set on and then post a > task to that thread whenever we invoke a method of the delegate. We should also > document that behavior in a comment in the delegate definition. Done. See the code comment of StorageFreeSpaceDelegate definition. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:69: virtual bool IsWatchingStorage(const FilePath& path) = 0; On 2012/09/12 17:54:47, vandebo wrote: > Not sure you need this. Removed it already. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:72: // The |path| can be a drive path on Windows, or the mount point on Linux. On 2012/09/12 17:54:47, vandebo wrote: > nit: Linux -> Posix Done. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:215: free_space_delegate_ = delegate; On 2012/09/12 17:54:47, vandebo wrote: > Add a DCHECK that free_sapce_delegate_ is NULL. We only expect the delegate to > be set once. Done, but allow the free_space_delegate_ to be set to NULL again. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:271: // The delegate instance for storage free space change. On 2012/09/12 17:54:47, vandebo wrote: > nit: The delegate that implements free space-change notification. Done. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:277: // The set of the storage path being watched. Since we can not know when On 2012/09/12 17:54:47, vandebo wrote: > nit: path -> paths, can not -> can't Done. http://codereview.chromium.org/10917166/diff/3023/base/system_monitor/system_... base/system_monitor/system_monitor.h:283: // The lock for manipulating free_space_changed_observer_map_ On 2012/09/12 17:54:47, vandebo wrote: > nit: remove The: "Lock for..." Done. http://codereview.chromium.org/10917166/diff/3023/chrome/browser/system_monit... File chrome/browser/system_monitor/media_device_notifications_chromeos.cc (right): http://codereview.chromium.org/10917166/diff/3023/chrome/browser/system_monit... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:50: base::SystemMonitor::Get()->set_storage_free_space_delegate(this); On 2012/09/12 17:54:47, vandebo wrote: > Since free space observer isn't really implemented in these classes yet. Add a > comment to the delegate set code that it needs to be called on the thread that > will be starting and stopping observation. Done. http://codereview.chromium.org/10917166/diff/3023/chrome/browser/system_monit... chrome/browser/system_monitor/media_device_notifications_chromeos.cc:54: if (base::SystemMonitor::Get()) On 2012/09/12 17:54:47, vandebo wrote: > Since we're removing the delegate here, we'll need to check that the delegate > isn't null before each use in SystemMonitor. Done.
http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... base/system_monitor/system_monitor.h:168: virtual void OnStorageFreeSpaceChanged(const FilePath::StringType& path) {} On 2012/09/12 17:54:47, vandebo wrote: > On 2012/09/12 09:24:31, Hongbo Min wrote: > > On 2012/09/11 22:49:27, vandebo wrote: > > > 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). > > > > 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? > > 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. 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.
http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:123: // Power-related APIs 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. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:228: void set_storage_free_space_delegate(StorageFreeSpaceDelegate* delegate); 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.
On 2012/09/13 13:40:53, Hongbo Min wrote: > If it is good to you, I will add unit test for it later. I would rather the unit test come in at the same time to ensure that gets done in a timely manner. The unit test is not just about making sure you're code is right, it's just as much about making sure someone doesn't break your code in the future. And right now is when you best understand and can test your own code. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.cc (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.cc:185: free_space_delegate_->StartWatchingStorage(path); Can you put this on line 191? http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.cc:212: DCHECK(free_space_observer_map_.find(path) == This case shouldn't happen, right? http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:17: #include "base/synchronization/lock.h" Your other CL was committed, so you need to rebase this change. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:64: // Since the client code may call these interfaces from any thread, the I intended for you to save the thread id in delegate set and then post to that thread in start/stop http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:177: int64 available_capacity) {} Shouldn't this be a uint64? http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:181: virtual void OnFreeSpaceWatchingError(const FilePath& path) {} Maybe OnFreeSpaceChangeError()? What do you think? http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:219: void ProcessStorageFreeSpaceMessage(const FilePath& path, 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); http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:223: StorageFreeSpaceDelegate* storage_free_space_delegate() const { I don't think you really need this method. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:240: typedef std::multimap<FilePath, StorageFreeSpaceObserver*> 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*.
http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:123: // Power-related APIs Added to my post M23 branch todo list. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:228: void set_storage_free_space_delegate(StorageFreeSpaceDelegate* delegate); On 2012/09/13 18:41:14, willchan wrote: > 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 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. > 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. 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.
http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:228: void set_storage_free_space_delegate(StorageFreeSpaceDelegate* delegate); On 2012/09/13 20:08:41, vandebo wrote: > On 2012/09/13 18:41:14, willchan wrote: > > 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 > > 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. > > > 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. > > 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. Not needed. The setter is the constructor of SystemMonitor, which any subclass would need to call. > 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. 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. >
Hongbo, I spoke with willchan on chat. One result was issue 149059, which this change is exempt from. The other is that we can't have a method to set the free space watcher delegate, it needs to be set in the constructor. This will mean moving the construction of SystemMonitor out of content/ and into the various embedders (i.e. chrome/ and content shell). See BrowserMainLoop::MainMessageLoopStart() and ChromeBrowserMainParts.
+jam Since that would require content/ changes, I went ahead and talked with jam@ offline. Since content/ uses it, he'd rather have everything constructed in content/. We didn't resolve that issue, because he followed up asking me a good question which I didn't know how to answer - why are we changing SystemMonitor to support this? If this is all strictly within content & chrome code, why do we need to go through SystemMonitor?
On 2012/09/13 22:10:47, willchan wrote: > +jam > > Since that would require content/ changes, I went ahead and talked with jam@ > offline. Since content/ uses it, he'd rather have everything constructed in > content/. We didn't resolve that issue, because he followed up asking me a good > question which I didn't know how to answer - why are we changing SystemMonitor > to support this? If this is all strictly within content & chrome code, why do we > need to go through SystemMonitor? You could ask that about SystemMonitor in general, or anything in base... I think there's two questions to answer. Should all this stuff be bundled into a class? If so, where should that class live? The thing that ties all the pieces together here is that they are properties of the devices on the computer that we're running. It makes sense to put them all in the same place. Before I added the removable device notifications, I asked if they should go into SystemMonitor and Will said yes. Should all of this be in content instead of base? Maybe, but jam already made us move some stuff up from content into chrome because content doesn't actually need it.
On 2012/09/13 22:41:48, vandebo wrote: > On 2012/09/13 22:10:47, willchan wrote: > > +jam > > > > Since that would require content/ changes, I went ahead and talked with jam@ > > offline. Since content/ uses it, he'd rather have everything constructed in > > content/. We didn't resolve that issue, because he followed up asking me a > good > > question which I didn't know how to answer - why are we changing SystemMonitor > > to support this? If this is all strictly within content & chrome code, why do > we > > need to go through SystemMonitor? > > You could ask that about SystemMonitor in general, or anything in base... I > think there's two questions to answer. Should all this stuff be bundled into a > class? If so, where should that class live? The thing that ties all the pieces > together here is that they are properties of the devices on the computer that > we're running. It makes sense to put them all in the same place. Before I > added the removable device notifications, I asked if they should go into > SystemMonitor and Will said yes. Should all of this be in content instead of > base? Maybe, but jam already made us move some stuff up from content into > chrome because content doesn't actually need it. (Note I didn't read all the above comments or the code) From my discussion with Will, I was under the impression that this is all code that runs in chrome (i.e. the code that triggers _and_ listens to the notification). If that's true, then base is not really contributing anything other than holding on to a list of observers. It seems that this code belongs in src/chrome then. base is for two things in general: generic utility style code like string/file manipulations, and classes that are shared across multiple module (i.e. src/content and src/chrome). However in this case, it seems that this functionality is all within src/chrome.
On 2012/09/13 23:36:39, John Abd-El-Malek wrote: > On 2012/09/13 22:41:48, vandebo wrote: > > On 2012/09/13 22:10:47, willchan wrote: > > > +jam > > > > > > Since that would require content/ changes, I went ahead and talked with jam@ > > > offline. Since content/ uses it, he'd rather have everything constructed in > > > content/. We didn't resolve that issue, because he followed up asking me a > > good > > > question which I didn't know how to answer - why are we changing > SystemMonitor > > > to support this? If this is all strictly within content & chrome code, why > do > > we > > > need to go through SystemMonitor? > > > > You could ask that about SystemMonitor in general, or anything in base... I > > think there's two questions to answer. Should all this stuff be bundled into > a > > class? If so, where should that class live? The thing that ties all the pieces > > together here is that they are properties of the devices on the computer that > > we're running. It makes sense to put them all in the same place. Before I > > added the removable device notifications, I asked if they should go into > > SystemMonitor and Will said yes. Should all of this be in content instead of > > base? Maybe, but jam already made us move some stuff up from content into > > chrome because content doesn't actually need it. > > (Note I didn't read all the above comments or the code) > From my discussion with Will, I was under the impression that this is all code > that runs in chrome (i.e. the code that triggers _and_ listens to the > notification). If that's true, then base is not really contributing anything > other than holding on to a list of observers. It seems that this code belongs in > src/chrome then. > > base is for two things in general: generic utility style code like string/file > manipulations, and classes that are shared across multiple module (i.e. > src/content and src/chrome). However in this case, it seems that this > functionality is all within src/chrome. There are consumers of the SystemMonitor interface in both content and chrome. There are providers of SystemMonitor implementation in both content and chrome.
Yeah, I think this all snuck into SystemMonitor, because that's where the PowerObserver was. And it grew organically, and we never really revisited the decision. That's my fault as the base/ reviewer here, and jam@ was right to call this out. Note that PowerObserver (which is part of SystemMonitor), is used by base/ and net/ itself, so that's why it made sense for PowerObserver in SystemMonitor to be part of base/. I think that there are enough of these observers now, which are all in content / chrome, that they should be taken out of SystemMonitor. Note though, that this would remove the base/ dependency, but it still doesn't solve the other issue that originally prompted us to discuss this. That some of the providers of the SystemMonitor implementation live in chrome, so in order to delegate from content to chrome, we need a delegate set in either the constructor or a setter. And my argument that a delegate setter for a multithreaded object is undesirable. On Thu, Sep 13, 2012 at 4:40 PM, <vandebo@chromium.org> wrote: > On 2012/09/13 23:36:39, John Abd-El-Malek wrote: > >> On 2012/09/13 22:41:48, vandebo wrote: >> > On 2012/09/13 22:10:47, willchan wrote: >> > > +jam >> > > >> > > Since that would require content/ changes, I went ahead and talked >> with >> > jam@ > >> > > offline. Since content/ uses it, he'd rather have everything >> constructed >> > in > >> > > content/. We didn't resolve that issue, because he followed up asking >> me a >> > good >> > > question which I didn't know how to answer - why are we changing >> SystemMonitor >> > > to support this? If this is all strictly within content & chrome >> code, why >> do >> > we >> > > need to go through SystemMonitor? >> > >> > You could ask that about SystemMonitor in general, or anything in >> base... I >> > think there's two questions to answer. Should all this stuff be bundled >> > into > >> a >> > class? If so, where should that class live? The thing that ties all the >> > pieces > >> > together here is that they are properties of the devices on the computer >> > that > >> > we're running. It makes sense to put them all in the same place. >> Before I >> > added the removable device notifications, I asked if they should go into >> > SystemMonitor and Will said yes. Should all of this be in content >> instead >> > of > >> > base? Maybe, but jam already made us move some stuff up from content >> into >> > chrome because content doesn't actually need it. >> > > (Note I didn't read all the above comments or the code) >> From my discussion with Will, I was under the impression that this is >> all code >> that runs in chrome (i.e. the code that triggers _and_ listens to the >> notification). If that's true, then base is not really contributing >> anything >> other than holding on to a list of observers. It seems that this code >> belongs >> > in > >> src/chrome then. >> > > base is for two things in general: generic utility style code like >> string/file >> manipulations, and classes that are shared across multiple module (i.e. >> src/content and src/chrome). However in this case, it seems that this >> functionality is all within src/chrome. >> > > There are consumers of the SystemMonitor interface in both content and > chrome. > There are providers of SystemMonitor implementation in both content and > chrome. > > https://codereview.chromium.**org/10917166/<https://codereview.chromium.org/1... >
On 2012/09/13 23:48:24, willchan wrote: > Yeah, I think this all snuck into SystemMonitor, because that's where the > PowerObserver was. And it grew organically, and we never really revisited > the decision. That's my fault as the base/ reviewer here, and jam@ was > right to call this out. > > Note that PowerObserver (which is part of SystemMonitor), is used by base/ > and net/ itself, so that's why it made sense for PowerObserver in > SystemMonitor to be part of base/. > > I think that there are enough of these observers now, which are all in > content / chrome, that they should be taken out of SystemMonitor. I'm not sure there's benefit in separating PowerObserver from the other observers, but your wisdom will likely enlighten me. However lets figure out what the right design is on the refactor bug and not in this CL.
On Thu, Sep 13, 2012 at 4:48 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > Yeah, I think this all snuck into SystemMonitor, because that's where the > PowerObserver was. And it grew organically, and we never really revisited > the decision. That's my fault as the base/ reviewer here, and jam@ was > right to call this out. > > Note that PowerObserver (which is part of SystemMonitor), is used by base/ > and net/ itself, so that's why it made sense for PowerObserver in > SystemMonitor to be part of base/. > > I think that there are enough of these observers now, which are all in > content / chrome, that they should be taken out of SystemMonitor. > > Note though, that this would remove the base/ dependency, but it still > doesn't solve the other issue that originally prompted us to discuss this. > That some of the providers of the SystemMonitor implementation live in > chrome, so in order to delegate from content to chrome, we need a delegate > set in either the constructor or a setter. And my argument that a delegate > setter for a multithreaded object is undesirable. > I don't see why a delegate would be needed if an event that chrome fires and listens for stays in chrome. i.e. it seems that the reason this change adds a delegate is so that chrome can tell base that an event fired. but this shouldn't be in base at all. the whole _tell me when disk space frees up_ doesn't belong in base since the implementation is really in chrome.
On Thu, Sep 13, 2012 at 4:55 PM, <vandebo@chromium.org> wrote: > On 2012/09/13 23:48:24, willchan wrote: > >> Yeah, I think this all snuck into SystemMonitor, because that's where the >> PowerObserver was. And it grew organically, and we never really revisited >> the decision. That's my fault as the base/ reviewer here, and jam@ was >> right to call this out. >> > > Note that PowerObserver (which is part of SystemMonitor), is used by base/ >> and net/ itself, so that's why it made sense for PowerObserver in >> SystemMonitor to be part of base/. >> > > I think that there are enough of these observers now, which are all in >> content / chrome, that they should be taken out of SystemMonitor. >> > > I'm not sure there's benefit in separating PowerObserver from the other > observers, but your wisdom will likely enlighten me. However lets figure > out > what the right design is on the refactor bug and not in this CL. > Sorry if I'm being exasperating for having expressed a different opinion now than I did originally. Let me reflect on this a bit more. But I think my other comment stands, that regardless of whether these chrome/content provided notifications move out of SystemMonitor (and out of base), we'll still have a situation where chrome is providing some of these notifications, that may be consumed by content. So we need a method for chrome to pass in the delegate to content, either via the constructor, or via a setter. And I will defer to John on that. > > https://codereview.chromium.**org/10917166/<https://codereview.chromium.org/1... >
On Thu, Sep 13, 2012 at 4:40 PM, <vandebo@chromium.org> wrote: > On 2012/09/13 23:36:39, John Abd-El-Malek wrote: > >> On 2012/09/13 22:41:48, vandebo wrote: >> > On 2012/09/13 22:10:47, willchan wrote: >> > > +jam >> > > >> > > Since that would require content/ changes, I went ahead and talked >> with >> > jam@ > >> > > offline. Since content/ uses it, he'd rather have everything >> constructed >> > in > >> > > content/. We didn't resolve that issue, because he followed up asking >> me a >> > good >> > > question which I didn't know how to answer - why are we changing >> SystemMonitor >> > > to support this? If this is all strictly within content & chrome >> code, why >> do >> > we >> > > need to go through SystemMonitor? >> > >> > You could ask that about SystemMonitor in general, or anything in >> base... I >> > think there's two questions to answer. Should all this stuff be bundled >> > into > >> a >> > class? If so, where should that class live? The thing that ties all the >> > pieces > >> > together here is that they are properties of the devices on the computer >> > that > >> > we're running. It makes sense to put them all in the same place. >> Before I >> > added the removable device notifications, I asked if they should go into >> > SystemMonitor and Will said yes. Should all of this be in content >> instead >> > of > >> > base? Maybe, but jam already made us move some stuff up from content >> into >> > chrome because content doesn't actually need it. >> > > (Note I didn't read all the above comments or the code) >> From my discussion with Will, I was under the impression that this is >> all code >> that runs in chrome (i.e. the code that triggers _and_ listens to the >> notification). If that's true, then base is not really contributing >> anything >> other than holding on to a list of observers. It seems that this code >> belongs >> > in > >> src/chrome then. >> > > base is for two things in general: generic utility style code like >> string/file >> manipulations, and classes that are shared across multiple module (i.e. >> src/content and src/chrome). However in this case, it seems that this >> functionality is all within src/chrome. >> > > There are consumers of the SystemMonitor interface in both content and > chrome. > There are providers of SystemMonitor implementation in both content and > chrome. > This is too high level. I was referring to individual notifications, i.e. like disk space ran out. Let me give you an analogy: NotificationService is in the content layer, but chrome's notification (i.e. EXTENSION_INSTALLED) is in src/chrome since chrome is what fires and listens for this event. Same for PathService, which is in base, but DIR_USER_DATA is in chrome/common/chrome_paths.h > > https://codereview.chromium.**org/10917166/<https://codereview.chromium.org/1... >
On Thu, Sep 13, 2012 at 5:05 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > On Thu, Sep 13, 2012 at 4:55 PM, <vandebo@chromium.org> wrote: > >> On 2012/09/13 23:48:24, willchan wrote: >> >>> Yeah, I think this all snuck into SystemMonitor, because that's where the >>> PowerObserver was. And it grew organically, and we never really revisited >>> the decision. That's my fault as the base/ reviewer here, and jam@ was >>> right to call this out. >>> >> >> Note that PowerObserver (which is part of SystemMonitor), is used by >>> base/ >>> and net/ itself, so that's why it made sense for PowerObserver in >>> SystemMonitor to be part of base/. >>> >> >> I think that there are enough of these observers now, which are all in >>> content / chrome, that they should be taken out of SystemMonitor. >>> >> >> I'm not sure there's benefit in separating PowerObserver from the other >> observers, but your wisdom will likely enlighten me. However lets figure >> out >> what the right design is on the refactor bug and not in this CL. >> > > Sorry if I'm being exasperating for having expressed a different opinion > now than I did originally. Let me reflect on this a bit more. But I think > my other comment stands, that regardless of whether these chrome/content > provided notifications move out of SystemMonitor (and out of base), we'll > still have a situation where chrome is providing some of these > notifications, that may be consumed by content. > Do we really have this case? what notifications are fired by chrome and used by content? > So we need a method for chrome to pass in the delegate to content, either > via the constructor, or via a setter. And I will defer to John on that. > > >> >> https://codereview.chromium.**org/10917166/<https://codereview.chromium.org/1... >> > >
On 2012/09/14 00:08:19, John Abd-El-Malek wrote: > On Thu, Sep 13, 2012 at 5:05 PM, William Chan (陈智昌) > <willchan@chromium.org>wrote: > > > On Thu, Sep 13, 2012 at 4:55 PM, <mailto:vandebo@chromium.org> wrote: > > > >> On 2012/09/13 23:48:24, willchan wrote: > >> > >>> Yeah, I think this all snuck into SystemMonitor, because that's where the > >>> PowerObserver was. And it grew organically, and we never really revisited > >>> the decision. That's my fault as the base/ reviewer here, and jam@ was > >>> right to call this out. > >>> > >> > >> Note that PowerObserver (which is part of SystemMonitor), is used by > >>> base/ > >>> and net/ itself, so that's why it made sense for PowerObserver in > >>> SystemMonitor to be part of base/. > >>> > >> > >> I think that there are enough of these observers now, which are all in > >>> content / chrome, that they should be taken out of SystemMonitor. > >>> > >> > >> I'm not sure there's benefit in separating PowerObserver from the other > >> observers, but your wisdom will likely enlighten me. However lets figure > >> out > >> what the right design is on the refactor bug and not in this CL. > >> > > > > Sorry if I'm being exasperating for having expressed a different opinion > > now than I did originally. Let me reflect on this a bit more. But I think > > my other comment stands, that regardless of whether these chrome/content > > provided notifications move out of SystemMonitor (and out of base), we'll > > still have a situation where chrome is providing some of these > > notifications, that may be consumed by content. > > > > Do we really have this case? what notifications are fired by chrome and > used by content? I don't think we have this case, no. > > > > > So we need a method for chrome to pass in the delegate to content, either > > via the constructor, or via a setter. And I will defer to John on that. > > > > > >> > >> > https://codereview.chromium.**org/10917166/%3Chttps://codereview.chromium.org...> > >> > > > >
On Thu, Sep 13, 2012 at 5:09 PM, <vandebo@chromium.org> wrote: > On 2012/09/14 00:08:19, John Abd-El-Malek wrote: > >> On Thu, Sep 13, 2012 at 5:05 PM, William Chan (陈智昌) >> <willchan@chromium.org>wrote: >> > > > On Thu, Sep 13, 2012 at 4:55 PM, <mailto:vandebo@chromium.org> wrote: >> > >> >> On 2012/09/13 23:48:24, willchan wrote: >> >> >> >>> Yeah, I think this all snuck into SystemMonitor, because that's where >> the >> >>> PowerObserver was. And it grew organically, and we never really >> revisited >> >>> the decision. That's my fault as the base/ reviewer here, and jam@was >> >>> right to call this out. >> >>> >> >> >> >> Note that PowerObserver (which is part of SystemMonitor), is used by >> >>> base/ >> >>> and net/ itself, so that's why it made sense for PowerObserver in >> >>> SystemMonitor to be part of base/. >> >>> >> >> >> >> I think that there are enough of these observers now, which are all in >> >>> content / chrome, that they should be taken out of SystemMonitor. >> >>> >> >> >> >> I'm not sure there's benefit in separating PowerObserver from the other >> >> observers, but your wisdom will likely enlighten me. However lets >> figure >> >> out >> >> what the right design is on the refactor bug and not in this CL. >> >> >> > >> > Sorry if I'm being exasperating for having expressed a different opinion >> > now than I did originally. Let me reflect on this a bit more. But I >> think >> > my other comment stands, that regardless of whether these chrome/content >> > provided notifications move out of SystemMonitor (and out of base), >> we'll >> > still have a situation where chrome is providing some of these >> > notifications, that may be consumed by content. >> > >> > > Do we really have this case? what notifications are fired by chrome and >> used by content? >> > > I don't think we have this case, no. > > > Oops > > > > > > So we need a method for chrome to pass in the delegate to content, >> either >> > via the constructor, or via a setter. And I will defer to John on that. >> > >> > >> >> >> >> >> > > https://codereview.chromium.****org/10917166/%3Chttps://codere** > view.chromium.org/10917166/ <http://codereview.chromium.org/10917166/>> > >> >> >> > >> > >> > > > > https://codereview.chromium.**org/10917166/<https://codereview.chromium.org/1... >
so, briefly looking at the new code being added to base, it seems that this code can just be put in a separate class in src/chrome/browser? it's not really sharing implementation with SystemMonitor. There could be a StorageFreeSpaceWatcher class, and a StorageFreeSpaceWatcherDelegate (preferably in a separate file, that way an implementor doesn't need to include the header of StorageFreeSpaceWatcher in its header). On Thu, Sep 13, 2012 at 5:10 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > On Thu, Sep 13, 2012 at 5:09 PM, <vandebo@chromium.org> wrote: > >> On 2012/09/14 00:08:19, John Abd-El-Malek wrote: >> >>> On Thu, Sep 13, 2012 at 5:05 PM, William Chan (陈智昌) >>> <willchan@chromium.org>wrote: >>> >> >> > On Thu, Sep 13, 2012 at 4:55 PM, <mailto:vandebo@chromium.org> wrote: >>> > >>> >> On 2012/09/13 23:48:24, willchan wrote: >>> >> >>> >>> Yeah, I think this all snuck into SystemMonitor, because that's >>> where the >>> >>> PowerObserver was. And it grew organically, and we never really >>> revisited >>> >>> the decision. That's my fault as the base/ reviewer here, and jam@was >>> >>> right to call this out. >>> >>> >>> >> >>> >> Note that PowerObserver (which is part of SystemMonitor), is used by >>> >>> base/ >>> >>> and net/ itself, so that's why it made sense for PowerObserver in >>> >>> SystemMonitor to be part of base/. >>> >>> >>> >> >>> >> I think that there are enough of these observers now, which are all >>> in >>> >>> content / chrome, that they should be taken out of SystemMonitor. >>> >>> >>> >> >>> >> I'm not sure there's benefit in separating PowerObserver from the >>> other >>> >> observers, but your wisdom will likely enlighten me. However lets >>> figure >>> >> out >>> >> what the right design is on the refactor bug and not in this CL. >>> >> >>> > >>> > Sorry if I'm being exasperating for having expressed a different >>> opinion >>> > now than I did originally. Let me reflect on this a bit more. But I >>> think >>> > my other comment stands, that regardless of whether these >>> chrome/content >>> > provided notifications move out of SystemMonitor (and out of base), >>> we'll >>> > still have a situation where chrome is providing some of these >>> > notifications, that may be consumed by content. >>> > >>> >> >> Do we really have this case? what notifications are fired by chrome and >>> used by content? >>> >> >> I don't think we have this case, no. >> >> >> > Oops > > >> >> >> >> >> > So we need a method for chrome to pass in the delegate to content, >>> either >>> > via the constructor, or via a setter. And I will defer to John on that. >>> > >>> > >>> >> >>> >> >>> >> >> https://codereview.chromium.****org/10917166/%3Chttps://codere** >> view.chromium.org/10917166/ <http://codereview.chromium.org/10917166/>> >> >>> >> >>> > >>> > >>> >> >> >> >> https://codereview.chromium.**org/10917166/<https://codereview.chromium.org/1... >> > >
On 2012/09/13 13:48:14, Hongbo Min wrote: > http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... > File base/system_monitor/system_monitor.h (right): > > http://codereview.chromium.org/10917166/diff/6001/base/system_monitor/system_... > base/system_monitor/system_monitor.h:168: virtual void > OnStorageFreeSpaceChanged(const FilePath::StringType& path) {} > On 2012/09/12 17:54:47, vandebo wrote: > > On 2012/09/12 09:24:31, Hongbo Min wrote: > > > On 2012/09/11 22:49:27, vandebo wrote: > > > > 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). > > > > > > 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? > > > > 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. > > 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. vandebo, what is your opinion about this question? You may miss it due to the flooding message.
On 2012/09/14 03:20:54, Hongbo Min wrote: > On 2012/09/13 13:48:14, Hongbo Min wrote: > > 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. > > vandebo, what is your opinion about this question? You may miss it due to the > flooding message. Sorry, I don't understand what the question about is. Can you ask it again?
On 2012/09/14 05:10:33, vandebo wrote: > On 2012/09/14 03:20:54, Hongbo Min wrote: > > On 2012/09/13 13:48:14, Hongbo Min wrote: > > > 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. > > > > vandebo, what is your opinion about this question? You may miss it due to the > > flooding message. > > Sorry, I don't understand what the question about is. Can you ask it again? The question is, I think using FilePath::StringType is more reasonable than using FilePath for storage |path|. The reason is, as you mentioned, for MTP/PTP device, its path actually is not a regular file path, and FilePath::StringType is just a string type which can be used to represent any device identifier.
http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:64: // Since the client code may call these interfaces from any thread, the On 2012/09/13 19:57:34, vandebo wrote: > I intended for you to save the thread id in delegate set and then post to that > thread in start/stop 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. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:240: typedef std::multimap<FilePath, StorageFreeSpaceObserver*> On 2012/09/13 19:57:34, vandebo wrote: > 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*. 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.
On 2012/09/14 05:58:46, Hongbo Min wrote: > On 2012/09/14 05:10:33, vandebo wrote: > > On 2012/09/14 03:20:54, Hongbo Min wrote: > > > On 2012/09/13 13:48:14, Hongbo Min wrote: > > > > 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. > > > > > > vandebo, what is your opinion about this question? You may miss it due to > the > > > flooding message. > > > > Sorry, I don't understand what the question about is. Can you ask it again? > > The question is, I think using FilePath::StringType is more reasonable than > using FilePath for storage |path|. > > The reason is, as you mentioned, for MTP/PTP device, its path actually is not a > regular file path, and FilePath::StringType is just a string type which can be > used to represent any device identifier. You can't monitor free space on an MTP device as far as I know, so using FilePath is best.
http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... File base/system_monitor/system_monitor.h (right): http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:64: // Since the client code may call these interfaces from any thread, the On 2012/09/14 06:04:36, Hongbo Min wrote: > On 2012/09/13 19:57:34, vandebo wrote: > > I intended for you to save the thread id in delegate set and then post to that > > thread in start/stop > > Actually, the base code is not allowed to access BrowserThread to get the thread > ID and post a task to the specific thread. No, but it is allowed to look at the thread ID. Look at how ObserverListThreadSafe is implemented. > Now since the delegate set will be moved to ctor. Nothing to do for this. 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. http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... base/system_monitor/system_monitor.h:240: typedef std::multimap<FilePath, StorageFreeSpaceObserver*> On 2012/09/14 06:04:36, Hongbo Min wrote: > On 2012/09/13 19:57:34, vandebo wrote: > > 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*. > > 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. 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>
On 2012/09/14 08:07:17, vandebo wrote: > http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... > File base/system_monitor/system_monitor.h (right): > > http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... > base/system_monitor/system_monitor.h:64: // Since the client code may call these > interfaces from any thread, the > On 2012/09/14 06:04:36, Hongbo Min wrote: > > On 2012/09/13 19:57:34, vandebo wrote: > > > I intended for you to save the thread id in delegate set and then post to > that > > > thread in start/stop > > > > Actually, the base code is not allowed to access BrowserThread to get the > thread > > ID and post a task to the specific thread. > > No, but it is allowed to look at the thread ID. Look at how > ObserverListThreadSafe is implemented. Thanks for your hint. It exactly what I want to know. > > > Now since the delegate set will be moved to ctor. Nothing to do for this. > > 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. But we may not know on which thread the start/stop watching is called, says, although we save the thread id in SystemMonitor ctor, can we post a task for start/stop watching to it? I doubt. For example, on Linux, the SystemMonitor is constructed on UI thread, but the watching operation is posted to FILE thread (which is not visible for SystemMonitor).
On 2012/09/14 08:00:16, vandebo wrote: > On 2012/09/14 05:58:46, Hongbo Min wrote: > > On 2012/09/14 05:10:33, vandebo wrote: > > > On 2012/09/14 03:20:54, Hongbo Min wrote: > > > > On 2012/09/13 13:48:14, Hongbo Min wrote: > > > > > 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. > > > > > > > > vandebo, what is your opinion about this question? You may miss it due to > > the > > > > flooding message. > > > > > > Sorry, I don't understand what the question about is. Can you ask it again? > > > > The question is, I think using FilePath::StringType is more reasonable than > > using FilePath for storage |path|. > > > > The reason is, as you mentioned, for MTP/PTP device, its path actually is not > a > > regular file path, and FilePath::StringType is just a string type which can be > > used to represent any device identifier. > > You can't monitor free space on an MTP device as far as I know, so using > FilePath is best. That makes more sense.
On 2012/09/14 08:56:58, Hongbo Min wrote: > On 2012/09/14 08:07:17, vandebo wrote: > > > http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... > > File base/system_monitor/system_monitor.h (right): > > > > > http://codereview.chromium.org/10917166/diff/8016/base/system_monitor/system_... > > base/system_monitor/system_monitor.h:64: // Since the client code may call > these > > interfaces from any thread, the > > On 2012/09/14 06:04:36, Hongbo Min wrote: > > > On 2012/09/13 19:57:34, vandebo wrote: > > > > I intended for you to save the thread id in delegate set and then post to > > that > > > > thread in start/stop > > > > > > Actually, the base code is not allowed to access BrowserThread to get the > > thread > > > ID and post a task to the specific thread. > > > > No, but it is allowed to look at the thread ID. Look at how > > ObserverListThreadSafe is implemented. > > Thanks for your hint. It exactly what I want to know. > > > > > > Now since the delegate set will be moved to ctor. Nothing to do for this. > > > > 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. > > But we may not know on which thread the start/stop watching is called, says, > although we save the thread id in SystemMonitor ctor, can we post a task for > start/stop watching to it? I doubt. For example, on Linux, the SystemMonitor is > constructed on UI thread, but the watching operation is posted to FILE thread > (which is not visible for SystemMonitor). Ok, you convinced me. We have no good way of knowing what thread the implementation wants the start stop methods called. Posting to the construction thread and then having the implementation post to a different thread would be a waste, so lets just note in the documentation of the functions that they may be called on any thread and the implementation/delegate should post to a specific thread if it needs to.
vandebo, the patch is updated now. Pls review it. The free space delegate is now set in SystemMonitor ctor, and the value for delegate parameter accepted in ctor is NULL as default. But I didn't change the SystemMonitor construction in the SystemMonitor consumer, e.g. chrome, content_shell etc. , since this changes may touch many source files, and I think it is meaningless currently because the functionality for watching free space is indeed not ready. It makes more sense to change them when the functionality is implemented in future or the SystemMonitor is refactored. The unit test is added for SystemMonitor, but it only covers the case for Process* and Observer interfaces, it is actually not enough for mulitiple thread environment. The testing for MT env is expected to be covered by the unittest for implementation.
On 2012/09/15 14:45:58, Hongbo Min wrote: > vandebo, the patch is updated now. Pls review it. > > The free space delegate is now set in SystemMonitor ctor, and the value for > delegate parameter accepted in ctor is NULL as default. But I didn't change the > SystemMonitor construction in the SystemMonitor consumer, e.g. chrome, > content_shell etc. , since this changes may touch many source files, and I think > it is meaningless currently because the functionality for watching free space is > indeed not ready. It makes more sense to change them when the functionality is > implemented in future or the SystemMonitor is refactored. > > The unit test is added for SystemMonitor, but it only covers the case for > Process* and Observer interfaces, it is actually not enough for mulitiple thread > environment. The testing for MT env is expected to be covered by the unittest > for implementation. I am considering whether it is worth adding free space watching capability into SystemMonitor in this way. Now I prefer to close this CL because of the SystemMonitor will be refactored and it may introduce additional effort on refactoring work. Moreover, the current design needs to be polished again. The way of exposing StorageFreeSpaceDelegate in SystemMonitor seems to not be sound enough. Let's see SystemMonitor class responsibility again, it is designed to monitor the system device changes, e.g. power status, device notification, free space watcher, and may be more capabilities in future, each capability should be a separate implementation. I like the jam@ suggestion for implementing StorageFreeSpaceWatcher and StorageFreeSpaceDelegate in a separate file located in chrome/browser. |
