Move device notification impl out of chrome/browser/media_gallery

Move device notification impl out of chrome/browser/media_gallery


BUG=145400
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154961

[Hongbo Min, 2012-09-02 13:21:40]

As discussed in http://codereview.chromium.org/10893006/, the device
notification implementation is now moved out of chrome/browser/system_monitor. 

This patch is just using git mv and modify the header file including statement.

[Hongbo Min, 2012-09-03 06:55:10]

Hi, Reviewers,

Although the device notification implementation is moved out of
chrome/browser/media_gallery, it still has limitations if I want to extend
storage watching capacity (for free space change) into system monitor.

Since the free space change event may be fired too often, it makes more sense
only starting it when needed to avoid unnecessary overhead for watching it, and
stoping it when no listener is interested. So SystemMonitor is expected to
expose two interfaces: StartWatchingStorage and StopWatchingStorage. 

However, it is not straightforward to add these 2 interfaces, although there are
2 approaches to solve it:

1. Abstract a common class for device notification and expose
StartWatching/StopWatching. The platform specific implementations are derived
from it. Meanwhile, the client code can reference the application-wide instance
via DeviceNotification::Get() method.

The disadvantages are: 1) introduce an additional DeviceNotification class, but
its functionality is better to covered by SystemMonitor; 2) force the client
code to be aware of DeviceNotification except SysteMonitor (for the Observer
definitions).

2. Add StartWatching/StopWatching to SystemMonitor, and make SystemMonitor hold
a device notification impl. The device notification impl is encapsulated into
SystemMonitor and invisible to the client, and gets initialized when calling
SystemMonitor::Get().

The advantages are obvious: 1) SystemMonitor will have more clear responsibility
which mainly performs monitoring and notify its observers if something happens,
e.g. media device arrival/removal/free space change, power changes, and so on.
In fact, I don't know why to expose ProcessXXX method rather than implementing
device notification in SystemMonitor. The ProcessXXX series can be removed away
from SystemMonitor if the device notification impl is inlined. 2) The client
code only needs to care about SystemMonitor and its Observers.

I prefer the approach 2. What is your opinion? Is it worth refactoring
SystemMonitor in the way as described above? Thanks in advance.

[vandebo (ex-Chrome), 2012-09-04 21:47:58]

On 2012/09/03 06:55:10, Hongbo Min wrote:
> Hi, Reviewers,

Please select a minimal set of reviewers, or call out what you'd like each
reviewer to look at.  If you'd like other people to be aware of the change, just
add them to the CC line.  I've moved most of your reviewers to the CC line. And
added thestig, who can approve changes in the chrome/ directory (maybe that's
why you added Nico, but thestig is already familiar with stuff in the
media_gallery directory).

> Although the device notification implementation is moved out of
> chrome/browser/media_gallery, it still has limitations if I want to extend
> storage watching capacity (for free space change) into system monitor.
> 
> Since the free space change event may be fired too often, it makes more sense
> only starting it when needed to avoid unnecessary overhead for watching it,
and
> stoping it when no listener is interested. So SystemMonitor is expected to
> expose two interfaces: StartWatchingStorage and StopWatchingStorage. 

Since you don't plan to make this change in this CL (I assume - we prefer
simpler changes), it's probably better to have the discussion on the bug
tracking the change.

> However, it is not straightforward to add these 2 interfaces, although there
are
> 2 approaches to solve it:
> 
> 1. Abstract a common class for device notification and expose
> StartWatching/StopWatching. The platform specific implementations are derived
> from it. Meanwhile, the client code can reference the application-wide
instance
> via DeviceNotification::Get() method.
> 
> The disadvantages are: 1) introduce an additional DeviceNotification class,
but
> its functionality is better to covered by SystemMonitor; 2) force the client
> code to be aware of DeviceNotification except SysteMonitor (for the Observer
> definitions).
> 
> 2. Add StartWatching/StopWatching to SystemMonitor, and make SystemMonitor
hold
> a device notification impl. The device notification impl is encapsulated into
> SystemMonitor and invisible to the client, and gets initialized when calling
> SystemMonitor::Get().
> 
> The advantages are obvious: 1) SystemMonitor will have more clear
responsibility
> which mainly performs monitoring and notify its observers if something
happens,
> e.g. media device arrival/removal/free space change, power changes, and so on.
> In fact, I don't know why to expose ProcessXXX method rather than implementing
> device notification in SystemMonitor. The ProcessXXX series can be removed
away
> from SystemMonitor if the device notification impl is inlined. 2) The client
> code only needs to care about SystemMonitor and its Observers.
> 
> I prefer the approach 2. What is your opinion? Is it worth refactoring
> SystemMonitor in the way as described above? Thanks in advance.

I think suggest two is to make SystemMonitor more of an interface, and then have
platform specific implementations.  This won't work because the implementations
can't live in base (they need to run on different threads, or create window
objects.) and base can't depend on any other component.  Something very similar
to what I understand of your suggestion two is to create an observer interface
in SystemMonitor that has the two methods you want to add.  The implementations
can then set an observer in the SystemMonitor class to implement the desired
functions.  Aside from the interface, this doesn't require any other classes.

media_device_notification_* should get renamed to
removable_device_notification_*, but that can be done when the implementations
are changed to track all removable devices and not just media devices.

I noted a couple other renamings that should be done, but they can be done in a
separate CL.

As is, this change is a consistent first step to moving things and LGTM.

So I can better prioritize my code reviews, what time zone are you in?

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
File chrome/browser/system_monitor/media_gallery_constants.h (right):

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
This file should get renamed.  Maybe removable_device_notifications_constants.h
?

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
File chrome/browser/system_monitor/media_storage_util.h (right):

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
chrome/browser/system_monitor/media_storage_util.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
This file should get renamed.  Maybe removable_device_util.h?

[Hongbo Min, 2012-09-05 00:13:49]

On 2012/09/04 21:47:58, vandebo wrote:
> On 2012/09/03 06:55:10, Hongbo Min wrote:
> > Hi, Reviewers,
> 
> Please select a minimal set of reviewers, or call out what you'd like each
> reviewer to look at.  If you'd like other people to be aware of the change,
just
> add them to the CC line.  I've moved most of your reviewers to the CC line.
And
> added thestig, who can approve changes in the chrome/ directory (maybe that's
> why you added Nico, but thestig is already familiar with stuff in the
> media_gallery directory).
> 
> > Although the device notification implementation is moved out of
> > chrome/browser/media_gallery, it still has limitations if I want to extend
> > storage watching capacity (for free space change) into system monitor.
> > 
> > Since the free space change event may be fired too often, it makes more
sense
> > only starting it when needed to avoid unnecessary overhead for watching it,
> and
> > stoping it when no listener is interested. So SystemMonitor is expected to
> > expose two interfaces: StartWatchingStorage and StopWatchingStorage. 
> 
> Since you don't plan to make this change in this CL (I assume - we prefer
> simpler changes), it's probably better to have the discussion on the bug
> tracking the change.
> 
> > However, it is not straightforward to add these 2 interfaces, although there
> are
> > 2 approaches to solve it:
> > 
> > 1. Abstract a common class for device notification and expose
> > StartWatching/StopWatching. The platform specific implementations are
derived
> > from it. Meanwhile, the client code can reference the application-wide
> instance
> > via DeviceNotification::Get() method.
> > 
> > The disadvantages are: 1) introduce an additional DeviceNotification class,
> but
> > its functionality is better to covered by SystemMonitor; 2) force the client
> > code to be aware of DeviceNotification except SysteMonitor (for the Observer
> > definitions).
> > 
> > 2. Add StartWatching/StopWatching to SystemMonitor, and make SystemMonitor
> hold
> > a device notification impl. The device notification impl is encapsulated
into
> > SystemMonitor and invisible to the client, and gets initialized when calling
> > SystemMonitor::Get().
> > 
> > The advantages are obvious: 1) SystemMonitor will have more clear
> responsibility
> > which mainly performs monitoring and notify its observers if something
> happens,
> > e.g. media device arrival/removal/free space change, power changes, and so
on.
> > In fact, I don't know why to expose ProcessXXX method rather than
implementing
> > device notification in SystemMonitor. The ProcessXXX series can be removed
> away
> > from SystemMonitor if the device notification impl is inlined. 2) The client
> > code only needs to care about SystemMonitor and its Observers.
> > 
> > I prefer the approach 2. What is your opinion? Is it worth refactoring
> > SystemMonitor in the way as described above? Thanks in advance.
> 
> I think suggest two is to make SystemMonitor more of an interface, and then
have
> platform specific implementations.  This won't work because the
implementations
> can't live in base (they need to run on different threads, or create window
> objects.) and base can't depend on any other component.  Something very
similar
> to what I understand of your suggestion two is to create an observer interface
> in SystemMonitor that has the two methods you want to add.  The
implementations
> can then set an observer in the SystemMonitor class to implement the desired
> functions.  Aside from the interface, this doesn't require any other classes.
> 
> media_device_notification_* should get renamed to
> removable_device_notification_*, but that can be done when the implementations
> are changed to track all removable devices and not just media devices.
> 
> I noted a couple other renamings that should be done, but they can be done in
a
> separate CL.
> 
> As is, this change is a consistent first step to moving things and LGTM.
> 
> So I can better prioritize my code reviews, what time zone are you in?

> 
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> File chrome/browser/system_monitor/media_gallery_constants.h (right):
> 
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c)
2012
> The Chromium Authors. All rights reserved.
> This file should get renamed.  Maybe
removable_device_notifications_constants.h
> ?
> 
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> File chrome/browser/system_monitor/media_storage_util.h (right):
> 
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> chrome/browser/system_monitor/media_storage_util.h:1: // Copyright (c) 2012
The
> Chromium Authors. All rights reserved.
> This file should get renamed.  Maybe removable_device_util.h?

So nice you are:) I am in China, the time zone is GMT+8:00. At this moment, we
are in morning 8:00.

You understanding about suggest 2 is exactly right. Is it possible to move
SystemMonitor out of base and make it become an interface which hides the device
notification impl?

[vandebo (ex-Chrome), 2012-09-05 00:17:55]

On 2012/09/05 00:13:49, Hongbo Min wrote:
> You understanding about suggest 2 is exactly right. Is it possible to move
> SystemMonitor out of base and make it become an interface which hides the
device
> notification impl?

The removable storage part of system monitor is not the only consumer.  I am not
aware of what parts of the code the other consumers live in.  But even if that
wasn't a problem, there would still be a problem with chrome os.  We don't want
chrome/browser depending on chromeos (for the chrome os implementation) and we
can't move SystemMonitor into the chromeos directories.  I think an observer or
some other abstracting pattern is the way to go.

[Hongbo Min, 2012-09-05 05:34:01]

On 2012/09/05 00:17:55, vandebo wrote:
> On 2012/09/05 00:13:49, Hongbo Min wrote:
> > You understanding about suggest 2 is exactly right. Is it possible to move
> > SystemMonitor out of base and make it become an interface which hides the
> device
> > notification impl?
> 
> The removable storage part of system monitor is not the only consumer.  I am
not
> aware of what parts of the code the other consumers live in.  But even if that
> wasn't a problem, there would still be a problem with chrome os.  We don't
want
> chrome/browser depending on chromeos (for the chrome os implementation) and we
> can't move SystemMonitor into the chromeos directories.  I think an observer
or
> some other abstracting pattern is the way to go.

So in your mind, where is the best place to add StartWatching/StopWatching
methods?

[commit-bot: I haz the power, 2012-09-05 05:36:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10919051/1

[commit-bot: I haz the power, 2012-09-05 05:36:44]

Presubmit check for 10919051-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/intents

Presubmit checks took 2.9s to calculate.

[Hongbo Min, 2012-09-05 05:41:03]

@thestig, @jhawkins

Need your LGTM to commit. Thanks.

[Lei Zhang, 2012-09-05 05:56:50]

lgtm if you just rename the file vandebo mentioned.

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
File chrome/browser/system_monitor/media_gallery_constants.h (right):

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/04 21:47:58, vandebo wrote:
> This file should get renamed.  Maybe
removable_device_notifications_constants.h
> ?

Can you rename this file?

[Hongbo Min, 2012-09-05 08:15:12]

On 2012/09/05 05:56:50, Lei Zhang wrote:
> lgtm if you just rename the file vandebo mentioned.
> 
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> File chrome/browser/system_monitor/media_gallery_constants.h (right):
> 
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c)
2012
> The Chromium Authors. All rights reserved.
> On 2012/09/04 21:47:58, vandebo wrote:
> > This file should get renamed.  Maybe
> removable_device_notifications_constants.h
> > ?
> 
> Can you rename this file?

vandebo suggested renaming those file can be done in a separate CL.

[Lei Zhang, 2012-09-05 08:25:55]

On 2012/09/05 08:15:12, Hongbo Min wrote:
> On 2012/09/05 05:56:50, Lei Zhang wrote:
> > lgtm if you just rename the file vandebo mentioned.
> > 
> >
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> > File chrome/browser/system_monitor/media_gallery_constants.h (right):
> > 
> >
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> > chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c)
> 2012
> > The Chromium Authors. All rights reserved.
> > On 2012/09/04 21:47:58, vandebo wrote:
> > > This file should get renamed.  Maybe
> > removable_device_notifications_constants.h
> > > ?
> > 
> > Can you rename this file?
> 
> vandebo suggested renaming those file can be done in a separate CL.

It would be nice to just rename it once. OTOH, it's probably a good time for you
to commit this before more of that code change.... so go for it.

[Hongbo Min, 2012-09-05 08:29:15]

On 2012/09/05 08:25:55, Lei Zhang wrote:
> On 2012/09/05 08:15:12, Hongbo Min wrote:
> > On 2012/09/05 05:56:50, Lei Zhang wrote:
> > > lgtm if you just rename the file vandebo mentioned.
> > > 
> > >
> >
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> > > File chrome/browser/system_monitor/media_gallery_constants.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
> > > chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright
(c)
> > 2012
> > > The Chromium Authors. All rights reserved.
> > > On 2012/09/04 21:47:58, vandebo wrote:
> > > > This file should get renamed.  Maybe
> > > removable_device_notifications_constants.h
> > > > ?
> > > 
> > > Can you rename this file?
> > 
> > vandebo suggested renaming those file can be done in a separate CL.
> 
> It would be nice to just rename it once. OTOH, it's probably a good time for
you
> to commit this before more of that code change.... so go for it.

I will rename them later. Thanks.

[Hongbo Min, 2012-09-05 08:42:56]

I just renamed media_gallery_constants.* files.  See my comments.

Is it fine to commit now? Thanks.

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
File chrome/browser/system_monitor/media_gallery_constants.h (right):

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/05 05:56:50, Lei Zhang wrote:
> On 2012/09/04 21:47:58, vandebo wrote:
> > This file should get renamed.  Maybe
> removable_device_notifications_constants.h
> > ?
> 
> Can you rename this file?

Done.

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
chrome/browser/system_monitor/media_gallery_constants.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/09/04 21:47:58, vandebo wrote:
> This file should get renamed.  Maybe
removable_device_notifications_constants.h
> ?

Rename to removable_device_constants.h now.

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
File chrome/browser/system_monitor/media_storage_util.h (right):

http://codereview.chromium.org/10919051/diff/1/chrome/browser/system_monitor/...
chrome/browser/system_monitor/media_storage_util.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/09/04 21:47:58, vandebo wrote:
> This file should get renamed.  Maybe removable_device_util.h?

Seems it is better to rename this file together with
media_device_notification_*. It is left to be renamed in the next CL. 

Moreover, currently the name of this file seems not be so confused.

[commit-bot: I haz the power, 2012-09-05 09:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10919051/8002

[commit-bot: I haz the power, 2012-09-05 10:12:37]

Try job failure for 10919051-8002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-09-05 12:13:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10919051/5004

[commit-bot: I haz the power, 2012-09-05 12:40:24]

Try job failure for 10919051-5004 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-05 12:52:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10919051/13002

[commit-bot: I haz the power, 2012-09-05 16:04:50]

Change committed as 154961