Add a receiver interface to RemovableStorageNotifications.

Add a receiver interface to RemovableStorageNotifications.

The receiver interface separates the API for systems that issue
notifications from the API that observes notifications.

R=vandebo@chromium.org
BUG=149059
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180881

[vandebo (ex-Chrome), 2013-01-31 23:51:29]

Not sure if all my suggestions are compatible.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_linux.cc:149:
media_transfer_protocol_device_observer_.reset(
Add a todo to make MediaTransferProtocolDeviceObserverLinux owned by
RemovableStorageNotificationsLinux

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.mm:288:
image_capture_device_manager_.reset(new chrome::ImageCaptureDeviceManager);
Add a TODO to make ImageCpatureDeviceManger owned by
RemovableDeviceNotificationsMac

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_manager.h:37: void
SetNotifications(RemovableStorageNotifications::Receiver* notifications);
Why not pass this in to the constructor?

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_manager.mm (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_manager.mm:32: // Weak
pointer
Not a weak pointer, instead say that it is guaranteed to outlive this class. And
a TODO to update when this class is owned by RemovableStorageNotifications.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc:217:
if (notifications_)
Why the condition?  Should always be true, right?  Passing in the constructor
would ensure that.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h:45:
void SetNotifications(RemovableStorageNotifications::Receiver* notifications);
Pass in constructor?

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h:72:
// The notifications object to use to signal newly attached devices.
Note lifetime and todo to update when it changes.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:42:
volume_mount_watcher_(volume_mount_watcher),
Instead, maybe have test methods that set the watchers ?

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:44: virtual
~Receiver() {}
Put the implementation in the .cc file.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:46: virtual void
ProcessAttach(const StorageInfo& info) = 0;
nit: Take the components here and make the StorageInfo internally.  That way
each caller doesn't need to do it.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:92:
scoped_ptr<Receiver> receiver_;
Make this private and add a protected getter

[Greg Billock, 2013-02-01 18:28:42]

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_linux.cc (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_linux.cc:149:
media_transfer_protocol_device_observer_.reset(
On 2013/01/31 23:51:29, vandebo wrote:
> Add a todo to make MediaTransferProtocolDeviceObserverLinux owned by
> RemovableStorageNotificationsLinux

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.mm:288:
image_capture_device_manager_.reset(new chrome::ImageCaptureDeviceManager);
On 2013/01/31 23:51:29, vandebo wrote:
> Add a TODO to make ImageCpatureDeviceManger owned by
> RemovableDeviceNotificationsMac

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_manager.h:37: void
SetNotifications(RemovableStorageNotifications::Receiver* notifications);
On 2013/01/31 23:51:29, vandebo wrote:
> Why not pass this in to the constructor?

They should be. I used these setters because of the ownership problems. What I
anticipate is that when all these are owned by the notifications class, they'll
be created that way. Currently, there's unevenness in where they're all made in
the ChromeBrowserPartsMain, and in tests, so that won't work uniformly. I hit
that early and then just copy-pasted to the rest, though. There could be a
couple where it would.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_manager.mm (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_manager.mm:32: // Weak
pointer
On 2013/01/31 23:51:29, vandebo wrote:
> Not a weak pointer, instead say that it is guaranteed to outlive this class.
And
> a TODO to update when this class is owned by RemovableStorageNotifications.

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc:217:
if (notifications_)
On 2013/01/31 23:51:29, vandebo wrote:
> Why the condition?  Should always be true, right?  Passing in the constructor
> would ensure that.

Yes. This is defensive and mostly done for uniformity -- we do this elsewhere,
for testing reasons. I'll remove if you like, though. Agreed requiring it in the
constructor (everywhere) is the way to go.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h:45:
void SetNotifications(RemovableStorageNotifications::Receiver* notifications);
see above.

On 2013/01/31 23:51:29, vandebo wrote:
> Pass in constructor?

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h:72:
// The notifications object to use to signal newly attached devices.
On 2013/01/31 23:51:29, vandebo wrote:
> Note lifetime and todo to update when it changes.

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:42:
volume_mount_watcher_(volume_mount_watcher),
On 2013/01/31 23:51:29, vandebo wrote:
> Instead, maybe have test methods that set the watchers ?

This one is the main force behind using setters. (The Linux/ChromeOS startup
timings are the other.) I think it'll work, but I figured I'd tackle it later.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:44: virtual
~Receiver() {}
On 2013/01/31 23:51:29, vandebo wrote:
> Put the implementation in the .cc file.

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:46: virtual void
ProcessAttach(const StorageInfo& info) = 0;
On 2013/01/31 23:51:29, vandebo wrote:
> nit: Take the components here and make the StorageInfo internally.  That way
> each caller doesn't need to do it.

I think we'll end up with more components -- we need to break out the type and
persistent system ID, for instance. This is mainly to preserve function
signatures as that happens.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:92:
scoped_ptr<Receiver> receiver_;
On 2013/01/31 23:51:29, vandebo wrote:
> Make this private and add a protected getter

Done.

[vandebo (ex-Chrome), 2013-02-01 18:59:55]

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_manager.h:37: void
SetNotifications(RemovableStorageNotifications::Receiver* notifications);
On 2013/02/01 18:28:42, Greg Billock wrote:
> On 2013/01/31 23:51:29, vandebo wrote:
> > Why not pass this in to the constructor?
> 
> They should be. I used these setters because of the ownership problems. What I
> anticipate is that when all these are owned by the notifications class,
they'll
> be created that way. Currently, there's unevenness in where they're all made
in
> the ChromeBrowserPartsMain, and in tests, so that won't work uniformly. I hit
> that early and then just copy-pasted to the rest, though. There could be a
> couple where it would.

Ok, can you add a TODO here then, so it's clear that's where we want to end up?

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc:217:
if (notifications_)
On 2013/02/01 18:28:42, Greg Billock wrote:
> On 2013/01/31 23:51:29, vandebo wrote:
> > Why the condition?  Should always be true, right?  Passing in the
constructor
> > would ensure that.
> 
> Yes. This is defensive and mostly done for uniformity -- we do this elsewhere,
> for testing reasons. I'll remove if you like, though. Agreed requiring it in
the
> constructor (everywhere) is the way to go.

If everything works without it, then lets go that way so that we don't have to
remember to remove it once the receiver comes in through the constructor.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:46: virtual void
ProcessAttach(const StorageInfo& info) = 0;
On 2013/02/01 18:28:43, Greg Billock wrote:
> On 2013/01/31 23:51:29, vandebo wrote:
> > nit: Take the components here and make the StorageInfo internally.  That way
> > each caller doesn't need to do it.
> 
> I think we'll end up with more components -- we need to break out the type and
> persistent system ID, for instance. This is mainly to preserve function
> signatures as that happens.

We used to have the type as a separate component and it made things more
complicated.  That's why we have MediaStorageUtil.  Are there other components
that you think we should add?  I have a strong preference for my initial
comment.

[Greg Billock, 2013-02-01 23:05:42]

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_manager.h:37: void
SetNotifications(RemovableStorageNotifications::Receiver* notifications);
On 2013/02/01 18:59:55, vandebo wrote:
> On 2013/02/01 18:28:42, Greg Billock wrote:
> > On 2013/01/31 23:51:29, vandebo wrote:
> > > Why not pass this in to the constructor?
> > 
> > They should be. I used these setters because of the ownership problems. What
I
> > anticipate is that when all these are owned by the notifications class,
> they'll
> > be created that way. Currently, there's unevenness in where they're all made
> in
> > the ChromeBrowserPartsMain, and in tests, so that won't work uniformly. I
hit
> > that early and then just copy-pasted to the rest, though. There could be a
> > couple where it would.
> 
> Ok, can you add a TODO here then, so it's clear that's where we want to end
up?

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc:217:
if (notifications_)
On 2013/02/01 18:59:55, vandebo wrote:
> On 2013/02/01 18:28:42, Greg Billock wrote:
> > On 2013/01/31 23:51:29, vandebo wrote:
> > > Why the condition?  Should always be true, right?  Passing in the
> constructor
> > > would ensure that.
> > 
> > Yes. This is defensive and mostly done for uniformity -- we do this
elsewhere,
> > for testing reasons. I'll remove if you like, though. Agreed requiring it in
> the
> > constructor (everywhere) is the way to go.
> 
> If everything works without it, then lets go that way so that we don't have to
> remember to remove it once the receiver comes in through the constructor.

Done.

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/12147002/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_storage_notifications.h:46: virtual void
ProcessAttach(const StorageInfo& info) = 0;
OK, I can put it back that way. I've been regarding that type as a useful public
format.

On 2013/02/01 18:59:55, vandebo wrote:
> On 2013/02/01 18:28:43, Greg Billock wrote:
> > On 2013/01/31 23:51:29, vandebo wrote:
> > > nit: Take the components here and make the StorageInfo internally.  That
way
> > > each caller doesn't need to do it.
> > 
> > I think we'll end up with more components -- we need to break out the type
and
> > persistent system ID, for instance. This is mainly to preserve function
> > signatures as that happens.
> 
> We used to have the type as a separate component and it made things more
> complicated.  That's why we have MediaStorageUtil.  Are there other components
> that you think we should add?  I have a strong preference for my initial
> comment.

[vandebo (ex-Chrome), 2013-02-02 01:44:30]

LGTM

[commit-bot: I haz the power, 2013-02-02 06:23:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/12147002/33002

[commit-bot: I haz the power, 2013-02-02 06:24:07]

Presubmit check for 12147002-33002 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/extensions

Presubmit checks took 2.2s to calculate.

[Greg Billock, 2013-02-04 16:09:13]

On 2013/02/02 06:24:07, I haz the power (commit-bot) wrote:
> Presubmit check for 12147002-33002 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/extensions
> 
> Presubmit checks took 2.2s to calculate.

+thestig for chrome/browser/chrome_browser_main* OWNERS
+mpcomplete for chrome/browser/extensions/api/* OWNERS

[Matt Perry, 2013-02-04 19:27:04]

lgtm

[Lei Zhang, 2013-02-04 19:31:07]

chrome/browser/chrome_browser_main* lgtm

[commit-bot: I haz the power, 2013-02-04 19:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/12147002/33002

[commit-bot: I haz the power, 2013-02-04 20:30:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-04 21:59:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/12147002/39004

[commit-bot: I haz the power, 2013-02-05 23:32:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/12147002/39004

[commit-bot: I haz the power, 2013-02-05 23:32:51]

Failed to apply patch for
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/media_gallery/media_file_system_registry_unittest.cc
  Hunk #1 FAILED at 359.
  Hunk #2 succeeded at 394 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/media_gallery/media_file_system_registry_unittest.cc.rej

Patch:       chrome/browser/media_gallery/media_file_system_registry_unittest.cc
Index: chrome/browser/media_gallery/media_file_system_registry_unittest.cc
diff --git a/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
b/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
index
947fd0f622b899b9beb847bd967dce766a3391ec..7c95b4566aefadc71339cbeeb7d682d594122d0c
100644
--- a/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
+++ b/chrome/browser/media_gallery/media_file_system_registry_unittest.cc
@@ -359,15 +359,16 @@ class MediaFileSystemRegistryTest : public
ChromeRenderViewHostTestHarness {
   std::vector<MediaFileSystemInfo> GetAutoAddedGalleries(
       ProfileState* profile_state);
 
+  // TODO(gbillock): Rework these once windows-specific code is gone.
   void ProcessAttach(const std::string& id,
                      const string16& name,
                      const FilePath::StringType& location) {
-    RemovableStorageNotifications::GetInstance()->ProcessAttach(
+    RemovableStorageNotifications::GetInstance()->receiver()->ProcessAttach(
         id, name, location);
   }
 
   void ProcessDetach(const std::string& id) {
-    RemovableStorageNotifications::GetInstance()->ProcessDetach(id);
+   
RemovableStorageNotifications::GetInstance()->receiver()->ProcessDetach(id);
   }
 
  protected:
@@ -393,6 +394,7 @@ class MediaFileSystemRegistryTest : public
ChromeRenderViewHostTestHarness {
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread file_thread_;
 
+// TODO(gbillock): Eliminate windows-specific code from this test.
 #if defined(OS_WIN)
   scoped_ptr<test::TestRemovableDeviceNotificationsWindowWin> window_;
 #else

[commit-bot: I haz the power, 2013-02-06 01:09:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/12147002/53001

[commit-bot: I haz the power, 2013-02-06 03:03:53]

Change committed as 180881