Extend system monitor capacity for watching the free space changes

Extend system monitor capacity which is able to monitor the free space changes
event for storage device.

BUG=

[Hongbo Min, 2012-08-28 14:33:16]

This patch is just pseudo-code for proof-of-concept. 

You may notice that I have sent out a email to chromium-dev about proposal for
extending SystemMonitor capacity and how it should be done technically.

Some relevant code seems to be premature, such as
FilePathWatcher::PlatformDelegate. The reason why I don't use FilePathWatcher
directly is, the FilePathWatcher doesn't watch sub directory and can not be used
for watching free space change.

I am not sure if I am in the right way. Could you pls give me some hints? If I
am right, I will go ahead to implement it into SystemMonitor class.

The initial implmentation is in StorageInfoProvider class, see
http://codereview.chromium.org/10836341/patch/10002/16002. According to mihai's
suggestion, it is better to extend system monitor capacity.

Thanks very much.

[vandebo (ex-Chrome), 2012-08-28 23:56:29]

It does seem that SystemMonitor is the right place to put this functionality.

It seems that reporting small changes in the amount of free space will trigger
an event too often.  Would it make sense to set the granularity of change
notifications?  1K, 1M, 10M?

It's not clear to me if you intend to add the functionality t the SystemMonitor
class (and thus use the StorageDeviceWatcherMap) or if you intend to put the
implementations in other classes and use the Notify mechanism.

Note, we recently changed SystemMonitor to notify about all removable devices,
not just media devices.  Furthermore, requirements for the media galleries API
where updated to include some of the storage info functionality:
crbug.com/144496

http://codereview.chromium.org/10893006/diff/5001/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/10893006/diff/5001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:186: bool StartWatchStorageDevice(const
FilePath::StringType& path);
nit: Watch -> Watching

[Mihai Parparita -not on Chrome, 2012-08-29 00:25:45]

On Tue, Aug 28, 2012 at 4:56 PM, <vandebo@chromium.org> wrote:

> It's not clear to me if you intend to add the functionality t the
> SystemMonitor
> class (and thus use the StorageDeviceWatcherMap) or if you intend to put
> the
> implementations in other classes and use the Notify mechanism.
>

I was actually had a related thought about the current SystemMonitor
design. It seems a bit odd that the actual code to generate notifications
lives in chrome/browser/media_gallery. I don't think that implementation
necessarily has to be inside of SystemMonitor, but if it were 1) not in
chrome/browser 2) not called "media_gallery" then it would be more amenable
to being extended in ways that Hongo is suggesting.

Perhaps there should be a /impl subdirectory in base/system_monitor where
the current media_gallery watching code could be moved to, and Hongo's
changes go there too?

Mihai

[vandebo (ex-Chrome), 2012-08-29 00:28:50]

On 2012/08/29 00:25:45, Mihai Parparita wrote:
> On Tue, Aug 28, 2012 at 4:56 PM, <mailto:vandebo@chromium.org> wrote:
> 
> > It's not clear to me if you intend to add the functionality t the
> > SystemMonitor
> > class (and thus use the StorageDeviceWatcherMap) or if you intend to put
> > the
> > implementations in other classes and use the Notify mechanism.
> >
> 
> I was actually had a related thought about the current SystemMonitor
> design. It seems a bit odd that the actual code to generate notifications
> lives in chrome/browser/media_gallery. I don't think that implementation
> necessarily has to be inside of SystemMonitor, but if it were 1) not in
> chrome/browser 2) not called "media_gallery" then it would be more amenable
> to being extended in ways that Hongo is suggesting.
> 
> Perhaps there should be a /impl subdirectory in base/system_monitor where
> the current media_gallery watching code could be moved to, and Hongo's
> changes go there too?
> 
> Mihai

For various platform specific reasons the code can't live in base.  It isn't
needed in content.  It doesn't make sense in renderer because that kind of stuff
shouldn't be done from the sandbox.  So it should go somewhere in
chrome/browser.  It's in media gallery because it was media specific.  But I
have no problem moving it to some other chrome/browser directory. 
chrome/browser/system_monitor ?

[Mihai Parparita -not on Chrome, 2012-08-29 00:30:18]

On Tue, Aug 28, 2012 at 5:28 PM, <vandebo@chromium.org> wrote:

> chrome/browser/system_monitor ?
>

SGTM

[vandebo (ex-Chrome), 2012-08-29 00:33:35]

On 2012/08/29 00:30:18, Mihai Parparita wrote:
> On Tue, Aug 28, 2012 at 5:28 PM, <mailto:vandebo@chromium.org> wrote:
> 
> > chrome/browser/system_monitor ?
> >
> 
> SGTM

Files crbug.com/145400

[Hongbo Min, 2012-08-29 00:33:54]

On 2012/08/28 23:56:29, vandebo wrote:
> It does seem that SystemMonitor is the right place to put this functionality.
> 
> It seems that reporting small changes in the amount of free space will trigger
> an event too often.  Would it make sense to set the granularity of change
> notifications?  1K, 1M, 10M?

Yes, your concerns are valid. The granularity control is a way to reduce the
event frequency. Also, we can use pooling+watching to achieve it.

> 
> It's not clear to me if you intend to add the functionality t the
SystemMonitor
> class (and thus use the StorageDeviceWatcherMap) or if you intend to put the
> implementations in other classes and use the Notify mechanism.
> 
> Note, we recently changed SystemMonitor to notify about all removable devices,
> not just media devices.  Furthermore, requirements for the media galleries API
> where updated to include some of the storage info functionality:
> crbug.com/144496
> 

But the media_gallery component seems not be the right place to add this
functionality. 

If implementing this functionality in other place rather than SystemMonitor, the
code of similar functionalities (monitor system event) may scatter in different
place. 

>
http://codereview.chromium.org/10893006/diff/5001/base/system_monitor/system_...
> File base/system_monitor/system_monitor.h (right):
> 
>
http://codereview.chromium.org/10893006/diff/5001/base/system_monitor/system_...
> base/system_monitor/system_monitor.h:186: bool StartWatchStorageDevice(const
> FilePath::StringType& path);
> nit: Watch -> Watching

[Hongbo Min, 2012-08-29 00:41:16]

On 2012/08/29 00:33:35, vandebo wrote:
> On 2012/08/29 00:30:18, Mihai Parparita wrote:
> > On Tue, Aug 28, 2012 at 5:28 PM, <mailto:vandebo@chromium.org> wrote:
> > 
> > > chrome/browser/system_monitor ?
> > >
> > 
> > SGTM
> 
> Files crbug.com/145400

If there is no explicit owner for this bug, I would like to take it for refactor
the system monitor impl out of media_gallery. Is it OK?

[vandebo (ex-Chrome), 2012-08-29 00:44:18]

On 2012/08/29 00:41:16, Hongbo Min wrote:
> On 2012/08/29 00:33:35, vandebo wrote:
> > On 2012/08/29 00:30:18, Mihai Parparita wrote:
> > > On Tue, Aug 28, 2012 at 5:28 PM, <mailto:vandebo@chromium.org> wrote:
> > > 
> > > > chrome/browser/system_monitor ?
> > > >
> > > 
> > > SGTM
> > 
> > Files crbug.com/145400
> 
> If there is no explicit owner for this bug, I would like to take it for
refactor
> the system monitor impl out of media_gallery. Is it OK?

Feel free to move the files.  It may make sense to rename them at the same time.
 However, I have a number of changes here outstanding, so you might want to wait
a day or two:
https://chromiumcodereview.appspot.com/10882039/
https://chromiumcodereview.appspot.com/10880078/
https://chromiumcodereview.appspot.com/10898018

[Hongbo Min, 2012-08-29 14:02:42]

On 2012/08/29 00:44:18, vandebo wrote:
> On 2012/08/29 00:41:16, Hongbo Min wrote:
> > On 2012/08/29 00:33:35, vandebo wrote:
> > > On 2012/08/29 00:30:18, Mihai Parparita wrote:
> > > > On Tue, Aug 28, 2012 at 5:28 PM, <mailto:vandebo@chromium.org> wrote:
> > > > 
> > > > > chrome/browser/system_monitor ?
> > > > >
> > > > 
> > > > SGTM
> > > 
> > > Files crbug.com/145400
> > 
> > If there is no explicit owner for this bug, I would like to take it for
> refactor
> > the system monitor impl out of media_gallery. Is it OK?
> 
> Feel free to move the files.  It may make sense to rename them at the same
time.
>  However, I have a number of changes here outstanding, so you might want to
wait
> a day or two:
> https://chromiumcodereview.appspot.com/10882039/
> https://chromiumcodereview.appspot.com/10880078/
> https://chromiumcodereview.appspot.com/10898018

That's fine. Thanks.

[vandebo (ex-Chrome), 2012-08-31 17:05:55]

On 2012/08/29 14:02:42, Hongbo Min wrote:
> On 2012/08/29 00:44:18, vandebo wrote:
> > On 2012/08/29 00:41:16, Hongbo Min wrote:
> > > On 2012/08/29 00:33:35, vandebo wrote:
> > > > On 2012/08/29 00:30:18, Mihai Parparita wrote:
> > > > > On Tue, Aug 28, 2012 at 5:28 PM, <mailto:vandebo@chromium.org> wrote:
> > > > > 
> > > > > > chrome/browser/system_monitor ?
> > > > > >
> > > > > 
> > > > > SGTM
> > > > 
> > > > Files crbug.com/145400
> > > 
> > > If there is no explicit owner for this bug, I would like to take it for
> > refactor
> > > the system monitor impl out of media_gallery. Is it OK?
> > 
> > Feel free to move the files.  It may make sense to rename them at the same
> time.
> >  However, I have a number of changes here outstanding, so you might want to
> wait
> > a day or two:
> > https://chromiumcodereview.appspot.com/10882039/
> > https://chromiumcodereview.appspot.com/10880078/
> > https://chromiumcodereview.appspot.com/10898018
> 
> That's fine. Thanks.

Those CLs have landed now.  Please (try to) cc on future changes related to
system monitor.

[Hongbo Min, 2012-09-10 14:24:55]

Close it. See the new CL at http://codereview.chromium.org/10917166/