[Media Galleries] Move RemovableStorageInfo notifications to chrome namespace (part 2)

[Media Galleries] Move RemovableStorageInfo notifications to chrome namespace (part 2)

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

[Lei Zhang, 2012-12-17 20:10:39]

Steve can tell you if this is where we want to move stuff as part of the
SystemMonitor refactoring:
https://code.google.com/p/chromium/issues/detail?id=149059

[vandebo (ex-Chrome), 2013-01-04 19:58:38]

A couple structural comments to get your started.

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_storage_notifications.h:56: class
BASE_EXPORT RemovableStorageObserver {
The Observer should not be an inner class and should be in its own header file.

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_storage_notifications.h:70: void
AddRemovableStorageObserver(RemovableStorageObserver* obs);
Since there's only one observer in this class, these can be just AddObserver and
RemoveObserver

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_storage_notifications.h:73: void
ProcessRemovableStorageAttached(const std::string& id,
The process methods should not be public.  I was planning to make them protected
and make the implementations inherit from this class.

[vandebo (ex-Chrome), 2013-01-04 21:06:03]

I had initially picked "StorageMonitor" for the name of the class, but
"RemovableStorageNotifications" seems fine too.  Not necessarily in this CL, but
we should also rename the directory.

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:217:

Why move this below the implementation set up?  The observer won't get
notifications about the already connected devices here.

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm
(right):

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:50:
class MockStorageObserver
Duplicate... pull this into a test util file.

[Greg Billock, 2013-01-07 21:55:18]

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:217:

On 2013/01/04 21:06:03, vandebo wrote:
> Why move this below the implementation set up?  The observer won't get
> notifications about the already connected devices here.

I don't recall doing this deliberately. Perhaps a merge-introduced problem?

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm
(right):

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:50:
class MockStorageObserver
On 2013/01/04 21:06:03, vandebo wrote:
> Duplicate... pull this into a test util file.

Done.

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_storage_notifications.h:56: class
BASE_EXPORT RemovableStorageObserver {
Sounds good. I was mimicking the existing layout pretty closely, but I'm going
to split out this change separately as a way to get started. I think that'll
break the change into three pieces, which will be good. :-)

On 2013/01/04 19:58:38, vandebo wrote:
> The Observer should not be an inner class and should be in its own header
file.

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_storage_notifications.h:70: void
AddRemovableStorageObserver(RemovableStorageObserver* obs);
Done. This was a mouthful!

On 2013/01/04 19:58:38, vandebo wrote:
> Since there's only one observer in this class, these can be just AddObserver
and
> RemoveObserver

https://codereview.chromium.org/11573048/diff/3001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_storage_notifications.h:73: void
ProcessRemovableStorageAttached(const std::string& id,
On 2013/01/04 19:58:38, vandebo wrote:
> The process methods should not be public.  I was planning to make them
protected
> and make the implementations inherit from this class.

Done.

[vandebo (ex-Chrome), 2013-01-11 22:06:50]

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:6:
#include "base/system_monitor/system_monitor.h"
remove?

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:272: class
MediaFileSystemRegistryTest : public ChromeRenderViewHostTestHarness {
Can you move just this class out of the anonymous namespace?

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:95:
FilePath("/etc/mtab"), &message_loop_)) {}
Should this point to a temp/testing path?

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:207:
mock_storage_observer_.reset(new MockRemovableStorageObserver);
Move to start of method

[Hongbo Min, 2013-01-17 09:38:03]

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:321:
RemovableStorageNotifications::GetInstance()->ProcessAttach(
Is RemovableStorageNotifications instance always created for testing? Do we need
to add if statement to check it?

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util_unittest.cc (right):

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util_unittest.cc:31: const string16&
name,
nits: incorrect indent.

[Greg Billock, 2013-01-17 22:10:45]

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/extensions...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:6:
#include "base/system_monitor/system_monitor.h"
On 2013/01/11 22:06:50, vandebo wrote:
> remove?

Done.

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:272: class
MediaFileSystemRegistryTest : public ChromeRenderViewHostTestHarness {
On 2013/01/11 22:06:50, vandebo wrote:
> Can you move just this class out of the anonymous namespace?

I can accomplish that, but things are kind of strewn around -- I don't think
people expect a lot of namespace changes in the file, so it'll be a problem
magnet.

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:95:
FilePath("/etc/mtab"), &message_loop_)) {}
On 2013/01/11 22:06:50, vandebo wrote:
> Should this point to a temp/testing path?

I've now fixed this up with a proper fake.

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:207:
mock_storage_observer_.reset(new MockRemovableStorageObserver);
On 2013/01/11 22:06:50, vandebo wrote:
> Move to start of method

We can't add the observer until the notifications class is made, before which we
need to write the mtab. I think this is in the right place.

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:321:
RemovableStorageNotifications::GetInstance()->ProcessAttach(
Good question. This test isn't fully converted yet -- I'll fix this up.

On 2013/01/17 09:38:03, Hongbo Min wrote:
> Is RemovableStorageNotifications instance always created for testing? Do we
need
> to add if statement to check it?

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util_unittest.cc (right):

https://codereview.chromium.org/11573048/diff/57002/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util_unittest.cc:31: const string16&
name,
On 2013/01/17 09:38:03, Hongbo Min wrote:
> nits: incorrect indent.

Done.

[vandebo (ex-Chrome), 2013-01-18 18:42:58]

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:272: class
MediaFileSystemRegistryTest : public ChromeRenderViewHostTestHarness {
On 2013/01/17 22:10:45, Greg Billock wrote:
> On 2013/01/11 22:06:50, vandebo wrote:
> > Can you move just this class out of the anonymous namespace?
> 
> I can accomplish that, but things are kind of strewn around -- I don't think
> people expect a lot of namespace changes in the file, so it'll be a problem
> magnet.

Can you move it to the top of the file so it's before the anonymous namespace ?

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:304: class
FakeRemovableStorageNotifications
I don't see what this class does here?

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:409:
FakeRemovableStorageNotifications notifications_;
Not used

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:767: // Need
to create BrowserMainParts here? Or does one of these harnesses do it?
I don't think a BrowserMainParts is created... not sure we want to make one.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
std::vector<chrome::RemovableStorageNotifications::StorageInfo>
Would a using clause make this wrap nicer?

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/portable_device_watcher_win.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/portable_device_watcher_win.h:63: // !!! change to
take the device notifs as arg
nit: TODO

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:26:
#include "testing/gmock/include/gmock/gmock.h"
Aren't you removing gmock use?

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_storage_notifications.h:66: //
TODO(gbillock): remove these friends by making the classes owned by the
nit: I don't think you're going to fix the tests that way, and you don't need
to.  Move the TODO down to line 71?

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/volume_mount_watcher_win.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/volume_mount_watcher_win.h:26: // TODO(gbillock):
Take the RemovableStorageNotifications as an argument.
This is so that you could call ProcessAttach/Detach ?  I don't think that will
work.  I think you need to take an inner class of
RemovableDeviceNotificationsWindowWin that makes that functionality available
here and to PortableDeviceWatcherWin.  The TODO is fine as long as we agree on
what it really means.

[Hongbo Min, 2013-01-20 03:41:19]

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_storage_notifications.h:48: //
TODO(gbillock): Change this method signature to use StorageInfo.
What I see is you have already changed the signature to use StorageInfo, am I
right?

[Greg Billock, 2013-01-22 20:00:39]

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/39001/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:272: class
MediaFileSystemRegistryTest : public ChromeRenderViewHostTestHarness {
On 2013/01/18 18:42:58, vandebo wrote:
> On 2013/01/17 22:10:45, Greg Billock wrote:
> > On 2013/01/11 22:06:50, vandebo wrote:
> > > Can you move just this class out of the anonymous namespace?
> > 
> > I can accomplish that, but things are kind of strewn around -- I don't think
> > people expect a lot of namespace changes in the file, so it'll be a problem
> > magnet.
> 
> Can you move it to the top of the file so it's before the anonymous namespace
?

This will require a lot of other motion as well -- this is the test fixture, so
it'll require either a bunch of within-file forward declarations or re-ordering.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:304: class
FakeRemovableStorageNotifications
On 2013/01/18 18:42:58, vandebo wrote:
> I don't see what this class does here?

Needed to set up the singleton (done in the base class). (It'd be awesome to get
rid of the need for that, but that's out of scope for this change.)

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:409:
FakeRemovableStorageNotifications notifications_;
see above
On 2013/01/18 18:42:58, vandebo wrote:
> Not used

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:767: // Need
to create BrowserMainParts here? Or does one of these harnesses do it?
On 2013/01/18 18:42:58, vandebo wrote:
> I don't think a BrowserMainParts is created... not sure we want to make one.

Yeah, I looked into that, and got nowhere. Used the fake above.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
std::vector<chrome::RemovableStorageNotifications::StorageInfo>
On 2013/01/18 18:42:58, vandebo wrote:
> Would a using clause make this wrap nicer?

I think that's C++11 though.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/portable_device_watcher_win.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/portable_device_watcher_win.h:63: // !!! change to
take the device notifs as arg
Done.

On 2013/01/18 18:42:58, vandebo wrote:
> nit: TODO

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:26:
#include "testing/gmock/include/gmock/gmock.h"
Done here and elsewhere.

On 2013/01/18 18:42:58, vandebo wrote:
> Aren't you removing gmock use?

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/removable_storage_notifications.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_storage_notifications.h:48: //
TODO(gbillock): Change this method signature to use StorageInfo.
On 2013/01/20 03:41:19, Hongbo Min wrote:
> What I see is you have already changed the signature to use StorageInfo, am I
> right?

Done.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_storage_notifications.h:66: //
TODO(gbillock): remove these friends by making the classes owned by the
On 2013/01/18 18:42:58, vandebo wrote:
> nit: I don't think you're going to fix the tests that way, and you don't need
> to.  Move the TODO down to line 71?

Done.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
File chrome/browser/system_monitor/volume_mount_watcher_win.h (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/system_mon...
chrome/browser/system_monitor/volume_mount_watcher_win.h:26: // TODO(gbillock):
Take the RemovableStorageNotifications as an argument.
On 2013/01/18 18:42:58, vandebo wrote:
> This is so that you could call ProcessAttach/Detach ?  I don't think that will
> work.  I think you need to take an inner class of
> RemovableDeviceNotificationsWindowWin that makes that functionality available
> here and to PortableDeviceWatcherWin.  The TODO is fine as long as we agree on
> what it really means.

Yes, that's the goal -- basically the platform-specific impl should expose some
kind of interface it needs to relate to its tooling classes. The main thing is
to make the ownership tree simple and reduce (ideally to zero) singleton
references.

[vandebo (ex-Chrome), 2013-01-22 23:37:24]

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:304: class
FakeRemovableStorageNotifications
On 2013/01/22 20:00:39, Greg Billock wrote:
> On 2013/01/18 18:42:58, vandebo wrote:
> > I don't see what this class does here?
> 
> Needed to set up the singleton (done in the base class). (It'd be awesome to
get
> rid of the need for that, but that's out of scope for this change.)

Ahh, I see.  There's a second copy (and a third) of this class in this CL.  Pull
it out to a new file - either fake_removable_storage_notifications, or a util
file.

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
std::vector<chrome::RemovableStorageNotifications::StorageInfo>
On 2013/01/22 20:00:39, Greg Billock wrote:
> On 2013/01/18 18:42:58, vandebo wrote:
> > Would a using clause make this wrap nicer?
> 
> I think that's C++11 though.

Nope, there's plenty of using directives in the code already.

[Greg Billock, 2013-01-23 00:28:01]

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:304: class
FakeRemovableStorageNotifications
On 2013/01/22 23:37:24, vandebo wrote:
> On 2013/01/22 20:00:39, Greg Billock wrote:
> > On 2013/01/18 18:42:58, vandebo wrote:
> > > I don't see what this class does here?
> > 
> > Needed to set up the singleton (done in the base class). (It'd be awesome to
> get
> > rid of the need for that, but that's out of scope for this change.)
> 
> Ahh, I see.  There's a second copy (and a third) of this class in this CL. 
Pull
> it out to a new file - either fake_removable_storage_notifications, or a util
> file.

Used "test_*" since that seems the moniker in the directory. (perhaps should
rename the mock observer to that as well...)

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
std::vector<chrome::RemovableStorageNotifications::StorageInfo>
Right. I meant 'using' for inner classes. I could get rid of "chrome" but the
real mouthful is the "RemovableStorageNotifications::StorageInfo"

I'm kind of inclined to think that this should just be "StorageNotifications" or
something like that, as we should delegate the caring of removability to the
observer. That would help out a lot...

On 2013/01/22 23:37:24, vandebo wrote:
> On 2013/01/22 20:00:39, Greg Billock wrote:
> > On 2013/01/18 18:42:58, vandebo wrote:
> > > Would a using clause make this wrap nicer?
> > 
> > I think that's C++11 though.
> 
> Nope, there's plenty of using directives in the code already.

[Greg Billock, 2013-01-23 23:37:40]

On 2013/01/23 00:28:01, Greg Billock wrote:
>
https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
> File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
> chrome/browser/media_gallery/media_file_system_registry_unittest.cc:304: class
> FakeRemovableStorageNotifications
> On 2013/01/22 23:37:24, vandebo wrote:
> > On 2013/01/22 20:00:39, Greg Billock wrote:
> > > On 2013/01/18 18:42:58, vandebo wrote:
> > > > I don't see what this class does here?
> > > 
> > > Needed to set up the singleton (done in the base class). (It'd be awesome
to
> > get
> > > rid of the need for that, but that's out of scope for this change.)
> > 
> > Ahh, I see.  There's a second copy (and a third) of this class in this CL. 
> Pull
> > it out to a new file - either fake_removable_storage_notifications, or a
util
> > file.
> 
> Used "test_*" since that seems the moniker in the directory. (perhaps should
> rename the mock observer to that as well...)
> 
>
https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
> File chrome/browser/media_gallery/media_galleries_dialog_controller.cc
(right):
> 
>
https://codereview.chromium.org/11573048/diff/60004/chrome/browser/media_gall...
> chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
> std::vector<chrome::RemovableStorageNotifications::StorageInfo>
> Right. I meant 'using' for inner classes. I could get rid of "chrome" but the
> real mouthful is the "RemovableStorageNotifications::StorageInfo"
> 
> I'm kind of inclined to think that this should just be "StorageNotifications"
or
> something like that, as we should delegate the caring of removability to the
> observer. That would help out a lot...
> 
> On 2013/01/22 23:37:24, vandebo wrote:
> > On 2013/01/22 20:00:39, Greg Billock wrote:
> > > On 2013/01/18 18:42:58, vandebo wrote:
> > > > Would a using clause make this wrap nicer?
> > > 
> > > I think that's C++11 though.
> > 
> > Nope, there's plenty of using directives in the code already.

OK, Relevant test failures are fixed to follow the merge now. Should be ready
for a look. We could split this up following the plan to route notifications to
SystemMonitor and the new class. Do you want to go that route? Or just deal with
this large change? It's mostly mechanical, so I've kept it all together.

[vandebo (ex-Chrome), 2013-01-24 19:19:10]

Almost all trivial comments.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
std::vector<chrome::RemovableStorageNotifications::StorageInfo>
You don't need chrome:: here (already in the chrome namespace), which lets you
do:

  std::vector<RemovableStorageNotifications::StorageInfo> removable_storages = 
      RemovableStorageNotifications::GetInstance()->GetAttachedStorage();

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:66: for
(std::vector<RemovableStorageNotifications::StorageInfo>::
nit: add a typedef for the vector<StorageInfo> type?

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:108:
scoped_ptr<chrome::test::TestRemovableStorageNotifications> notifications_;
Does this need to be a scoped_ptr, can it just be an member?

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:121:
EXPECT_EQ(1, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:129:
EXPECT_EQ(1, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:220:
EXPECT_EQ(1, observer().attach_calls());
nit: check detach count here

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:227:
EXPECT_EQ(1, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:238:
EXPECT_EQ(2, observer().attach_calls());
nit: checkout detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:245:
EXPECT_EQ(2, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:264:
EXPECT_EQ(1, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:298:
EXPECT_EQ(0, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:310:
EXPECT_EQ(1, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:317:
EXPECT_EQ(1, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:328:
EXPECT_EQ(2, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:335:
EXPECT_EQ(2, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:348:
EXPECT_EQ(1, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:355:
EXPECT_EQ(1, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:368:
EXPECT_EQ(1, observer().attach_calls());
nit: check detach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:376:
EXPECT_EQ(1, observer().detach_calls());
nit: check attach count here.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:204:
mock_storage_observer_.reset(new MockRemovableStorageObserver);
move the observer to line 201 so it will get notifications for any initial
devices.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:341:
EXPECT_EQ(1, observer().attach_calls());
nit: detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:350:
EXPECT_EQ(1, observer().detach_calls());
nit: attach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:378:
EXPECT_EQ(1, observer().attach_calls());
nit: detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:382:
EXPECT_EQ(1, observer().detach_calls());
nit: attach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:390:
EXPECT_EQ(2, observer().attach_calls());
nit: detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:399:
EXPECT_EQ(2, observer().detach_calls());
nit: attach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:99:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
nit: detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:122:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
nit: attach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:166:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
nit: detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:180:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
nit: detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:102:
{
Remove the scope too now that InSequence is gone

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:123:
EXPECT_EQ(expect_attach_calls, observer_.attach_calls());
nit: check detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:138:
{
Remove the scope too now that InSequence is gone

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:156:
EXPECT_EQ(expect_attach_calls, observer_.attach_calls());
nit: check detach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:167:
{
Remove the scope too now that InSequence is gone

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:183:
EXPECT_EQ(expect_detach_calls, observer_.detach_calls());
nit: check attach count

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:207:
{
Remove the scope too now that InSequence is gone

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/test_removable_storage_notifications.h
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
namespace test {
I don't often see the use of namespace test, are you copying this from
somewhere?

[Greg Billock, 2013-01-24 20:46:03]

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/media_gall...
File chrome/browser/media_gallery/media_galleries_dialog_controller.cc (right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/media_gall...
chrome/browser/media_gallery/media_galleries_dialog_controller.cc:32:
std::vector<chrome::RemovableStorageNotifications::StorageInfo>
On 2013/01/24 19:19:10, vandebo wrote:
> You don't need chrome:: here (already in the chrome namespace), which lets you
> do:
> 
>   std::vector<RemovableStorageNotifications::StorageInfo> removable_storages =

>       RemovableStorageNotifications::GetInstance()->GetAttachedStorage();

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:66: for
(std::vector<RemovableStorageNotifications::StorageInfo>::
On 2013/01/24 19:19:10, vandebo wrote:
> nit: add a typedef for the vector<StorageInfo> type?

Yeah, I thought about this. I don't like typedefs, but this is a pretty heavy
name and used all over. :-/ Since there wasn't one before, I bit the bullet.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:108:
scoped_ptr<chrome::test::TestRemovableStorageNotifications> notifications_;
On 2013/01/24 19:19:10, vandebo wrote:
> Does this need to be a scoped_ptr, can it just be an member?

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:121:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux_unittest.cc:129:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:220:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:227:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:238:
EXPECT_EQ(2, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: checkout detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:245:
EXPECT_EQ(2, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:264:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:298:
EXPECT_EQ(0, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:310:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:317:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:328:
EXPECT_EQ(2, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:335:
EXPECT_EQ(2, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:348:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:355:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:368:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:376:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count here.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:204:
mock_storage_observer_.reset(new MockRemovableStorageObserver);
On 2013/01/24 19:19:10, vandebo wrote:
> move the observer to line 201 so it will get notifications for any initial
> devices.

Those vary by test, so I think adding if after makes the tests easier to
understand.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:341:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:350:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: attach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:378:
EXPECT_EQ(1, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:382:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: attach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:390:
EXPECT_EQ(2, observer().attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:399:
EXPECT_EQ(2, observer().detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: attach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:99:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:122:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: attach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:166:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac_unittest.mm:180:
EXPECT_EQ(1, mock_storage_observer_->attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:102:
{
On 2013/01/24 19:19:10, vandebo wrote:
> Remove the scope too now that InSequence is gone

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:123:
EXPECT_EQ(expect_attach_calls, observer_.attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:138:
{
On 2013/01/24 19:19:10, vandebo wrote:
> Remove the scope too now that InSequence is gone

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:156:
EXPECT_EQ(expect_attach_calls, observer_.attach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check detach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:167:
{
On 2013/01/24 19:19:10, vandebo wrote:
> Remove the scope too now that InSequence is gone

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:183:
EXPECT_EQ(expect_detach_calls, observer_.detach_calls());
On 2013/01/24 19:19:10, vandebo wrote:
> nit: check attach count

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:207:
{
On 2013/01/24 19:19:10, vandebo wrote:
> Remove the scope too now that InSequence is gone

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/test_removable_storage_notifications.h
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
namespace test {
All the other test_* files in this package use this idiom. Just trying to blend
in... :-)

On 2013/01/24 19:19:10, vandebo wrote:
> I don't often see the use of namespace test, are you copying this from
> somewhere?

[vandebo (ex-Chrome), 2013-01-24 23:39:00]

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:66: for
(std::vector<RemovableStorageNotifications::StorageInfo>::
On 2013/01/24 20:46:03, Greg Billock wrote:
> On 2013/01/24 19:19:10, vandebo wrote:
> > nit: add a typedef for the vector<StorageInfo> type?
> 
> Yeah, I thought about this. I don't like typedefs, but this is a pretty heavy
> name and used all over. :-/ Since there wasn't one before, I bit the bullet.

I'm not the biggest fan of typedefs, but when it starts to affect formatting,
either the name has to change, or a typedef should be used...

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:227:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 20:46:03, Greg Billock wrote:
> On 2013/01/24 19:19:10, vandebo wrote:
> > nit: check attach count here.
> 
> Done.

Missed this one.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:204:
mock_storage_observer_.reset(new MockRemovableStorageObserver);
On 2013/01/24 20:46:03, Greg Billock wrote:
> On 2013/01/24 19:19:10, vandebo wrote:
> > move the observer to line 201 so it will get notifications for any initial
> > devices.
> 
> Those vary by test, so I think adding if after makes the tests easier to
> understand.

Previously it was set before and the tests worked then...

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/test_removable_storage_notifications.h
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
namespace test {
On 2013/01/24 20:46:03, Greg Billock wrote:
> All the other test_* files in this package use this idiom. Just trying to
blend
> in... :-)
> 
> On 2013/01/24 19:19:10, vandebo wrote:
> > I don't often see the use of namespace test, are you copying this from
> > somewhere?
> 
grep "namespace t" `find chrome/browser -name "test_*"` finds no real matches...

[Greg Billock, 2013-01-25 20:01:21]

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:66: for
(std::vector<RemovableStorageNotifications::StorageInfo>::
On 2013/01/24 23:39:01, vandebo wrote:
> On 2013/01/24 20:46:03, Greg Billock wrote:
> > On 2013/01/24 19:19:10, vandebo wrote:
> > > nit: add a typedef for the vector<StorageInfo> type?
> > 
> > Yeah, I thought about this. I don't like typedefs, but this is a pretty
heavy
> > name and used all over. :-/ Since there wasn't one before, I bit the bullet.
> 
> I'm not the biggest fan of typedefs, but when it starts to affect formatting,
> either the name has to change, or a typedef should be used...

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:227:
EXPECT_EQ(1, observer().detach_calls());
On 2013/01/24 23:39:01, vandebo wrote:
> On 2013/01/24 20:46:03, Greg Billock wrote:
> > On 2013/01/24 19:19:10, vandebo wrote:
> > > nit: check attach count here.
> > 
> > Done.
> 
> Missed this one.

Done.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:204:
mock_storage_observer_.reset(new MockRemovableStorageObserver);
Done. I was thinking I'd have to change a lot of the attach/detach count
expectations, but in fact it turns out they don't get used where the
pre-attachment would make them wrong, so this works out fine.

On 2013/01/24 23:39:01, vandebo wrote:
> On 2013/01/24 20:46:03, Greg Billock wrote:
> > On 2013/01/24 19:19:10, vandebo wrote:
> > > move the observer to line 201 so it will get notifications for any initial
> > > devices.
> > 
> > Those vary by test, so I think adding if after makes the tests easier to
> > understand.
> 
> Previously it was set before and the tests worked then...

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/test_removable_storage_notifications.h
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
namespace test {
On 2013/01/24 23:39:01, vandebo wrote:
> On 2013/01/24 20:46:03, Greg Billock wrote:
> > All the other test_* files in this package use this idiom. Just trying to
> blend
> > in... :-)
> > 
> > On 2013/01/24 19:19:10, vandebo wrote:
> > > I don't often see the use of namespace test, are you copying this from
> > > somewhere?
> > 
> grep "namespace t" `find chrome/browser -name "test_*"` finds no real
matches...

http://code.google.com/p/chromium/source/search?q=namespace+test+file%3Achrom...

I'm happy to get rid of it; I just don't think it matters.

https://codereview.chromium.org/11573048/diff/106001/chrome/browser/system_mo...
File
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
(right):

https://codereview.chromium.org/11573048/diff/106001/chrome/browser/system_mo...
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:241:
EXPECT_EQ(1, observer().detach_calls());
Had copy-pasted this wrong...

[Greg Billock, 2013-01-26 00:47:51]

On 2013/01/25 20:01:21, Greg Billock wrote:
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
> File chrome/browser/system_monitor/media_storage_util.cc (right):
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
> chrome/browser/system_monitor/media_storage_util.cc:66: for
> (std::vector<RemovableStorageNotifications::StorageInfo>::
> On 2013/01/24 23:39:01, vandebo wrote:
> > On 2013/01/24 20:46:03, Greg Billock wrote:
> > > On 2013/01/24 19:19:10, vandebo wrote:
> > > > nit: add a typedef for the vector<StorageInfo> type?
> > > 
> > > Yeah, I thought about this. I don't like typedefs, but this is a pretty
> heavy
> > > name and used all over. :-/ Since there wasn't one before, I bit the
bullet.
> > 
> > I'm not the biggest fan of typedefs, but when it starts to affect
formatting,
> > either the name has to change, or a typedef should be used...
> 
> Done.
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
> File
>
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
>
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:227:
> EXPECT_EQ(1, observer().detach_calls());
> On 2013/01/24 23:39:01, vandebo wrote:
> > On 2013/01/24 20:46:03, Greg Billock wrote:
> > > On 2013/01/24 19:19:10, vandebo wrote:
> > > > nit: check attach count here.
> > > 
> > > Done.
> > 
> > Missed this one.
> 
> Done.
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
> File
> chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
>
chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:204:
> mock_storage_observer_.reset(new MockRemovableStorageObserver);
> Done. I was thinking I'd have to change a lot of the attach/detach count
> expectations, but in fact it turns out they don't get used where the
> pre-attachment would make them wrong, so this works out fine.
> 
> On 2013/01/24 23:39:01, vandebo wrote:
> > On 2013/01/24 20:46:03, Greg Billock wrote:
> > > On 2013/01/24 19:19:10, vandebo wrote:
> > > > move the observer to line 201 so it will get notifications for any
initial
> > > > devices.
> > > 
> > > Those vary by test, so I think adding if after makes the tests easier to
> > > understand.
> > 
> > Previously it was set before and the tests worked then...
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
> File chrome/browser/system_monitor/test_removable_storage_notifications.h
> (right):
> 
>
https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
> chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
> namespace test {
> On 2013/01/24 23:39:01, vandebo wrote:
> > On 2013/01/24 20:46:03, Greg Billock wrote:
> > > All the other test_* files in this package use this idiom. Just trying to
> > blend
> > > in... :-)
> > > 
> > > On 2013/01/24 19:19:10, vandebo wrote:
> > > > I don't often see the use of namespace test, are you copying this from
> > > > somewhere?
> > > 
> > grep "namespace t" `find chrome/browser -name "test_*"` finds no real
> matches...
> 
>
http://code.google.com/p/chromium/source/search?q=namespace+test+file%253Achr...
> 
> I'm happy to get rid of it; I just don't think it matters.
> 
>
https://codereview.chromium.org/11573048/diff/106001/chrome/browser/system_mo...
> File
>
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/11573048/diff/106001/chrome/browser/system_mo...
>
chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:241:
> EXPECT_EQ(1, observer().detach_calls());
> Had copy-pasted this wrong...

look good? I'd like to get this in the CQ tonight if I can...

[vandebo (ex-Chrome), 2013-01-26 01:00:21]

LGTM after the minor fix below.

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
File chrome/browser/system_monitor/test_removable_storage_notifications.h
(right):

https://codereview.chromium.org/11573048/diff/95047/chrome/browser/system_mon...
chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
namespace test {
On 2013/01/25 20:01:21, Greg Billock wrote:
> On 2013/01/24 23:39:01, vandebo wrote:
> > On 2013/01/24 20:46:03, Greg Billock wrote:
> > > All the other test_* files in this package use this idiom. Just trying to
> > blend
> > > in... :-)
> > > 
> > > On 2013/01/24 19:19:10, vandebo wrote:
> > > > I don't often see the use of namespace test, are you copying this from
> > > > somewhere?
> > > 
> > grep "namespace t" `find chrome/browser -name "test_*"` finds no real
> matches...
> 
>
http://code.google.com/p/chromium/source/search?q=namespace+test+file%253Achr...
> 
> I'm happy to get rid of it; I just don't think it matters.

It's fine, I was just trying to understand best practices.

https://codereview.chromium.org/11573048/diff/106001/chrome/browser/system_mo...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/11573048/diff/106001/chrome/browser/system_mo...
chrome/browser/system_monitor/media_storage_util.cc:81: for
(std::vector<RemovableStorageNotifications::StorageInfo>::
One more instance here.

[Lei Zhang, 2013-01-26 01:27:49]

lgtm with nits eblow:

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/me...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/me...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:358:
chrome::RemovableStorageNotifications::GetInstance()->ProcessAttach(
nit: no need for chrome:: Since we're already inside namespace chrome. Ditto
below.

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc:217:
location);
nit: funny indentation.

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:143:
std::string unique_id;
Unlike the old code that expects an attachment for a particular (device + id),
now you are just checking for an attachment call. In which case, there's no need
to get this data from GetDeviceInfo(). Ditto below around line 172.

[Greg Billock, 2013-01-26 01:42:07]

https://chromiumcodereview.appspot.com/11573048/diff/95047/chrome/browser/sys...
File chrome/browser/system_monitor/test_removable_storage_notifications.h
(right):

https://chromiumcodereview.appspot.com/11573048/diff/95047/chrome/browser/sys...
chrome/browser/system_monitor/test_removable_storage_notifications.h:11:
namespace test {
no worries. I'm not necessarily claiming it as best practices. :-) I think it's
fine to have this convention in this module, but I don't think it is uniform...

On 2013/01/26 01:00:21, vandebo wrote:
> On 2013/01/25 20:01:21, Greg Billock wrote:
> > On 2013/01/24 23:39:01, vandebo wrote:
> > > On 2013/01/24 20:46:03, Greg Billock wrote:
> > > > All the other test_* files in this package use this idiom. Just trying
to
> > > blend
> > > > in... :-)
> > > > 
> > > > On 2013/01/24 19:19:10, vandebo wrote:
> > > > > I don't often see the use of namespace test, are you copying this from
> > > > > somewhere?
> > > > 
> > > grep "namespace t" `find chrome/browser -name "test_*"` finds no real
> > matches...
> > 
> >
>
http://code.google.com/p/chromium/source/search?q=namespace+test+file%25253Ac...
> > 
> > I'm happy to get rid of it; I just don't think it matters.
> 
> It's fine, I was just trying to understand best practices.

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/me...
File chrome/browser/media_gallery/media_file_system_registry_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/me...
chrome/browser/media_gallery/media_file_system_registry_unittest.cc:358:
chrome::RemovableStorageNotifications::GetInstance()->ProcessAttach(
On 2013/01/26 01:27:49, Lei Zhang wrote:
> nit: no need for chrome:: Since we're already inside namespace chrome. Ditto
> below.

Done.

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
chrome/browser/system_monitor/media_storage_util.cc:81: for
(std::vector<RemovableStorageNotifications::StorageInfo>::
On 2013/01/26 01:00:21, vandebo wrote:
> One more instance here.

Done.

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.cc:217:
location);
On 2013/01/26 01:27:49, Lei Zhang wrote:
> nit: funny indentation.

Done.

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
File
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11573048/diff/106001/chrome/browser/sy...
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:143:
std::string unique_id;
Yeah, this is just to get 'removable' basically. I can pass NULLs for the
others.

On 2013/01/26 01:27:49, Lei Zhang wrote:
> Unlike the old code that expects an attachment for a particular (device + id),
> now you are just checking for an attachment call. In which case, there's no
need
> to get this data from GetDeviceInfo(). Ditto below around line 172.

[commit-bot: I haz the power, 2013-01-26 01:43:54]

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

[commit-bot: I haz the power, 2013-01-26 01:44:01]

Failed to apply patch for
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
  Hunk #1 FAILED at 11.
  Hunk #2 succeeded at 23 (offset 2 lines).
  Hunk #3 succeeded at 44 (offset 2 lines).
  Hunk #4 succeeded at 73 (offset 4 lines).
  Hunk #5 FAILED at 89.
  Hunk #6 FAILED at 144.
  Hunk #7 FAILED at 165.
  Hunk #8 succeeded at 221 (offset 15 lines).
  Hunk #9 succeeded at 429 (offset 86 lines).
  4 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc.rej

Patch:      
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
Index:
chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
diff --git
a/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
b/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
index
81514e2437a2071dbf6463260e1257bb81858cbc..558e02c6e3b6562c6cb24a10043a7217353e62e2
100644
---
a/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
+++
b/chrome/browser/system_monitor/removable_device_notifications_window_win_unittest.cc
@@ -11,9 +11,8 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
-#include "base/system_monitor/system_monitor.h"
-#include "base/test/mock_devices_changed_observer.h"
 #include "chrome/browser/system_monitor/media_storage_util.h"
+#include "chrome/browser/system_monitor/mock_removable_storage_observer.h"
 #include "chrome/browser/system_monitor/portable_device_watcher_win.h"
 #include "chrome/browser/system_monitor/removable_device_constants.h"
 #include "chrome/browser/system_monitor/test_portable_device_watcher_win.h"
@@ -21,7 +20,6 @@
 #include "chrome/browser/system_monitor/test_volume_mount_watcher_win.h"
 #include "chrome/browser/system_monitor/volume_mount_watcher_win.h"
 #include "content/public/test/test_browser_thread.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chrome {
@@ -43,7 +41,6 @@ class RemovableDeviceNotificationsWindowWinTest : public
testing::Test {
   virtual void SetUp() OVERRIDE;
   virtual void TearDown() OVERRIDE;
 
-  void AddMassStorageDeviceAttachExpectation(const FilePath& drive);
   void PreAttachDevices();
 
   // Runs all the pending tasks on UI thread, FILE thread and blocking thread.
@@ -71,8 +68,7 @@ class RemovableDeviceNotificationsWindowWinTest : public
testing::Test {
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread file_thread_;
 
-  base::SystemMonitor system_monitor_;
-  base::MockDevicesChangedObserver observer_;
+  MockRemovableStorageObserver observer_;
 };
 
 RemovableDeviceNotificationsWindowWinTest::
@@ -92,48 +88,40 @@ void RemovableDeviceNotificationsWindowWinTest::SetUp() {
       volume_mount_watcher_.get(), new TestPortableDeviceWatcherWin));
   window_->InitWithTestData(false);
   RunUntilIdle();
-  system_monitor_.AddDevicesChangedObserver(&observer_);
+  window_->AddObserver(&observer_);
 }
 
 void RemovableDeviceNotificationsWindowWinTest::TearDown() {
   RunUntilIdle();
-  system_monitor_.RemoveDevicesChangedObserver(&observer_);
-}
-
-void RemovableDeviceNotificationsWindowWinTest::
-    AddMassStorageDeviceAttachExpectation(const FilePath& drive) {
-  std::string unique_id;
-  string16 device_name;
-  bool removable;
-  ASSERT_TRUE(volume_mount_watcher_->GetDeviceInfo(
-      drive, NULL, &unique_id, &device_name, &removable));
-  if (removable) {
-    MediaStorageUtil::Type type =
-        MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM;
-    std::string device_id = MediaStorageUtil::MakeDeviceId(type, unique_id);
-    EXPECT_CALL(observer_, OnRemovableStorageAttached(device_id, device_name,
-                                                      drive.value()))
-        .Times(1);
-  }
+  window_->RemoveObserver(&observer_);
 }
 
 void RemovableDeviceNotificationsWindowWinTest::PreAttachDevices() {
   window_.reset();
-  {
-    testing::InSequence sequence;
-    ASSERT_TRUE(volume_mount_watcher_.get());
-    volume_mount_watcher_->set_pre_attach_devices(true);
-    std::vector<FilePath> initial_devices =
-        volume_mount_watcher_->GetAttachedDevices();
-    for (std::vector<FilePath>::const_iterator it = initial_devices.begin();
-         it != initial_devices.end(); ++it) {
-      AddMassStorageDeviceAttachExpectation(*it);
-    }
+
+  int expect_attach_calls = 0;
+  ASSERT_TRUE(volume_mount_watcher_.get());
+  volume_mount_watcher_->set_pre_attach_devices(true);
+  std::vector<FilePath> initial_devices =
+      volume_mount_watcher_->GetAttachedDevices();
+  for (std::vector<FilePath>::const_iterator it = initial_devices.begin();
+       it != initial_devices.end(); ++it) {
+    std::string unique_id;
+    string16 device_name;
+    bool removable;
+    ASSERT_TRUE(volume_mount_watcher_->GetDeviceInfo(
+        *it, NULL, &unique_id, &device_name, &removable));
+    if (removable)
+      expect_attach_calls++;
   }
+
   window_.reset(new TestRemovableDeviceNotificationsWindowWin(
       volume_mount_watcher_.get(), new TestPortableDeviceWatcherWin));
   window_->InitWithTestData(true);
+
   RunUntilIdle();
+  EXPECT_EQ(expect_attach_calls, observer_.attach_calls());
+  EXPECT_EQ(0, observer_.detach_calls());
 }
 
 void RemovableDeviceNotificationsWindowWinTest::RunUntilIdle() {
@@ -147,18 +135,24 @@ void RemovableDeviceNotificationsWindowWinTest::
   volume_broadcast.dbcv_devicetype = DBT_DEVTYP_VOLUME;
   volume_broadcast.dbcv_unitmask = 0x0;
   volume_broadcast.dbcv_flags = 0x0;
-  {
-    testing::InSequence sequence;
-    for (DeviceIndices::const_iterator it = device_indices.begin();
-         it != device_indices.end(); ++it) {
-      volume_broadcast.dbcv_unitmask |= 0x1 << *it;
-      AddMassStorageDeviceAttachExpectation(
-          VolumeMountWatcherWin::DriveNumberToFilePath(*it));
-    }
+
+  int expect_attach_calls = 0;
+  for (DeviceIndices::const_iterator it = device_indices.begin();
+       it != device_indices.end(); ++it) {
+    volume_broadcast.dbcv_unitmask |= 0x1 << *it;
+    bool removable;
+    ASSERT_TRUE(volume_mount_watcher_->GetDeviceInfo(
+        VolumeMountWatcherWin::DriveNumberToFilePath(*it),
+        NULL, NULL, NULL, &removable));
+    if (removable)
+      expect_attach_calls++;
   }
   window_->InjectDeviceChange(DBT_DEVICEARRIVAL,
                               reinterpret_cast<DWORD>(&volume_broadcast));
+
   RunUntilIdle();
+  EXPECT_EQ(expect_attach_calls, observer_.attach_calls());
+  EXPECT_EQ(0, observer_.detach_calls());
 }
 
 void RemovableDeviceNotificationsWindowWinTest::
@@ -168,27 +162,23 @@ void RemovableDeviceNotificationsWindowWinTest::
   volume_broadcast.dbcv_devicetype = DBT_DEVTYP_VOLUME;
   volume_broadcast.dbcv_unitmask = 0x0;
   volume_broadcast.dbcv_flags = 0x0;
-  {
-    testing::InSequence sequence;
-    for (DeviceIndices::const_iterator it = device_indices.begin();
-         it != device_indices.end(); ++it) {
-      volume_broadcast.dbcv_unitmask |= 0x1 << *it;
-      std::string unique_id;
-      bool removable;
-      ASSERT_TRUE(volume_mount_watcher_->GetDeviceInfo(
-          VolumeMountWatcherWin::DriveNumberToFilePath(*it), NULL, &unique_id,
-          NULL, &removable));
-      if (removable) {
-        MediaStorageUtil::Type type =
-            MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM;
-        std::string device_id = MediaStorageUtil::MakeDeviceId(type,
unique_id);
-        EXPECT_CALL(observer_, OnRemovableStorageDetached(device_id)).Times(1);
-      }
-    }
+
+  int expect_detach_calls = 0;
+  for (DeviceIndices::const_iterator it = device_indices.begin();
+       it != device_indices.end(); ++it) {
+    volume_broadcast.dbcv_unitmask |= 0x1 << *it;
+    bool removable;
+    ASSERT_TRUE(volume_mount_watcher_->GetDeviceInfo(
+        VolumeMountWatcherWin::DriveNumberToFilePath(*it), NULL, NULL,
+        NULL, &removable));
+    if (removable)
+      expect_detach_calls++;
   }
   window_->InjectDeviceChange(DBT_DEVICEREMOVECOMPLETE,
                               reinterpret_cast<DWORD>(&volume_broadcast));
   RunUntilIdle();
+  EXPECT_EQ(0, observer_.attach_calls());
+  EXPECT_EQ(expect_detach_calls, observer_.detach_calls());
 }
 
 void RemovableDeviceNotificationsWindowWinTest::DoMTPDeviceTest(
@@ -209,32 +199,32 @@ void
RemovableDeviceNotificationsWindowWinTest::DoMTPDeviceTest(
   dev_interface_broadcast->dbcc_classguid = guidDevInterface;
   memcpy(dev_interface_broadcast->dbcc_name, pnp_device_id.data(),
          device_id_size);
-  {
-    testing::InSequence sequence;
-    PortableDeviceWatcherWin::StorageObjectIDs storage_object_ids =
-        TestPortableDeviceWatcherWin::GetMTPStorageObjectIds(pnp_device_id);
-    for (PortableDeviceWatcherWin::StorageObjectIDs::const_iterator it =
-         storage_object_ids.begin(); it != storage_object_ids.end(); ++it) {
-      std::string unique_id;
-      string16 name;
-      string16 location;
-      TestPortableDeviceWatcherWin::GetMTPStorageDetails(pnp_device_id, *it,
-                                                         &location, &unique_id,
-                                                         &name);
-      if (test_attach) {
-        EXPECT_CALL(observer_, OnRemovableStorageAttached(unique_id, name,
-                                                          location))
-            .Times((name.empty() || unique_id.empty()) ? 0 : 1);
-      } else {
-        EXPECT_CALL(observer_, OnRemovableStorageDetached(unique_id))
-            .Times((name.empty() || unique_id.empty()) ? 0 : 1);
-      }
-    }
+
+  int expect_attach_calls = observer_.attach_calls();
+  …
(message too large)

[commit-bot: I haz the power, 2013-01-29 20:10:19]

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

[commit-bot: I haz the power, 2013-01-29 22:29:11]

Change committed as 179438