Update Windows System Monitor Removable Device Impl.

Update Windows System Monitor Removable Device Impl.

Change the notifier to notify about all removable devices, to get more device information and to fetch the already connected removable devices.
Implement the Windows specific part of MediaStorageUtil

BUG=144496
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157256

[vandebo (ex-Chrome), 2012-09-14 07:59:21]

cpu: Please review or suggest a reviewer for the windows changes.
thestig: everything else.

http://codereview.chromium.org/10911234/diff/10013/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

http://codereview.chromium.org/10911234/diff/10013/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:207: void
ChromeBrowserMainPartsWin::PreMainMessageLoopRun() {
Not sure if this is the right way to do this.

http://codereview.chromium.org/10911234/diff/10013/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/10013/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:29:
const DWORD kMaxPathBufLen = MAX_PATH + 1;
The MSDN page for GetVolumeInformation says that it may be able to use a buffer
of size MAX_PATH + 1, so I use that size throughout.
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364993(v=vs.85).aspx

[cpu_(ooo_6.6-7.5), 2012-09-14 20:43:38]

please have rvargas review this patch. I will take a look but take my comments
as a drive-by.

[vandebo (ex-Chrome), 2012-09-14 20:45:30]

Hey Ricardo, can you take a look at the windows code in this CL?

[cpu_(ooo_6.6-7.5), 2012-09-14 20:49:48]

https://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

https://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:274:
WNDCLASSEX window_class;
this does not work on windows 8 metro mode.

I strongly suggest you consider a design that does not have poup native windows.
Does not work today, and will not work later when aura + ash runs on windows.

I hope you are not trying to land this in m23.

[vandebo (ex-Chrome), 2012-09-14 21:19:21]

https://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

https://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:274:
WNDCLASSEX window_class;
On 2012/09/14 20:49:48, cpu wrote:
> this does not work on windows 8 metro mode.
> 
> I strongly suggest you consider a design that does not have poup native
windows.
> Does not work today, and will not work later when aura + ash runs on windows.

In case you stopped reading at this point, we don't ever show this window, it's
a hidden window used just to receive the device arrival/disconnect messages. 
Will that not work in windows 8 metro mode?
It seems that it doesn't do anything bad because this code is already in the
codebase...

[rvargas (doing something else), 2012-09-15 02:33:55]

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:21:
namespace chrome {
I don't see any reason for extending the chrome namespace to include the using
directives nor the anon namespace.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:47:
*device_location = std::wstring(mount_point);
string16

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54:
if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))
This is not correct. What the blog says is to call the function on the "current"
drive root first and build the path again from there. Note that the guids are
actually consistent all the time, so most likely you don't have to care about
this issue at all. (but I don't know what's the end purpose of this code).

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:66:
WideToUTF16(volume_name, wcslen(volume_name), name);
ah?

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:93:
result.push_back(FilePath(volume_path));
I assume that you only care about one mount point.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:95:
NOTREACHED();
OS calls may (and will) fail at any time

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:110:
PDEV_BROADCAST_HDR dev_broadcast_hdr =
DEV_BROADCAST_HDR* ... which points me to the fact that the argument is not a
DWORD, it is an LPARAM

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:121:
FilePath::StringType path(L"_:\\");
nit: use string16 directly

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:149:
std::wstring location;
string16

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:272:
g_removable_device_notifications_window_win = this;
Basically this is a home grown singleton (without the documentation saying that
it has to be). Use a singleton or a lazy instance.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:334:
device_ids_[device] = device_id;
out of curiosity, base' SM keeps a list of removable devices and we keep another
one here?

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.h
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.h:32:
class RemovableDeviceNotificationsWindowWin
even if the implementation uses a window (looks like broken), there's no reason
for this class to be named "Window", being just the implementation of removable
device notifications for windows.

Yes, this is pre-existing code, so it doesn't have to be cleaned up with this
CL, but it's something to consider for a follow up.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.h:50:
void Init(GetDeviceInfoFunc getDeviceInfo,
InitForTest

[vandebo (ex-Chrome), 2012-09-15 19:45:10]

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:21:
namespace chrome {
On 2012/09/15 02:33:55, rvargas wrote:
> I don't see any reason for extending the chrome namespace to include the using
> directives nor the anon namespace.

Fixed.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:47:
*device_location = std::wstring(mount_point);
On 2012/09/15 02:33:55, rvargas wrote:
> string16

Changed all wchar/wstring to char16/string16

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54:
if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))
On 2012/09/15 02:33:55, rvargas wrote:
> This is not correct. What the blog says is to call the function on the
"current"
> drive root first and build the path again from there. Note that the guids are
> actually consistent all the time, so most likely you don't have to care about
> this issue at all. (but I don't know what's the end purpose of this code).

I still don't completely follow the blog on this point.  If you mean that
C:\drive_d_mount will always give one GUID even if D:\ is mounted there and D:\
consistently gives a different GUID, then I think it does need to be resolved
here because of GetDeviceInfoForPath()

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:93:
result.push_back(FilePath(volume_path));
On 2012/09/15 02:33:55, rvargas wrote:
> I assume that you only care about one mount point.

Correct.  Would you like a comment?

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:95:
NOTREACHED();
On 2012/09/15 02:33:55, rvargas wrote:
> OS calls may (and will) fail at any time

I think this function will fail gracefully - just skip the volume where the call
failed.  I added the NOTREACHED in case it starts consistently failing, a
developer will notice.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:110:
PDEV_BROADCAST_HDR dev_broadcast_hdr =
On 2012/09/15 02:33:55, rvargas wrote:
> DEV_BROADCAST_HDR* ... which points me to the fact that the argument is not a
> DWORD, it is an LPARAM

Fixed.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:121:
FilePath::StringType path(L"_:\\");
On 2012/09/15 02:33:55, rvargas wrote:
> nit: use string16 directly

Done.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:149:
std::wstring location;
On 2012/09/15 02:33:55, rvargas wrote:
> string16

Done.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:272:
g_removable_device_notifications_window_win = this;
On 2012/09/15 02:33:55, rvargas wrote:
> Basically this is a home grown singleton (without the documentation saying
that
> it has to be). Use a singleton or a lazy instance.

I looked at both before and again just now.  There is a subtle difference that I
think means I can't use either.  SystemMonitor is scoped object created in
content BrowserMainLoop.  The AtExitManager is in a scope above that.  So using
Singleton or LazyInstance with default traits would clean up this object too
late.

Not that it ameliorates the issue here, but it was recently decided (a couple
days ago) that SystemMonitor has outgrown its design and needs to be refactored.
 It's likely that each part of it will be split out into its own class.  So the
removable device notification which is updated in this CL will be managed by an
object in chrome/browser or will become a Singleton/LazyInstance.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:334:
device_ids_[device] = device_id;
On 2012/09/15 02:33:55, rvargas wrote:
> out of curiosity, base' SM keeps a list of removable devices and we keep
another
> one here?

Base SM has a vector of the attached devices.  We could fetch that (makes a
copy) and iterate it instead of keeping our own state here.  A future refactor
may alliterative this inefficiency.  We need to access it because the device id
is dependent on the content of the drive so we can't determine it once the
device has departed.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.h
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.h:32:
class RemovableDeviceNotificationsWindowWin
On 2012/09/15 02:33:55, rvargas wrote:
> even if the implementation uses a window (looks like broken), there's no
reason
> for this class to be named "Window", being just the implementation of
removable
> device notifications for windows.
> 
> Yes, this is pre-existing code, so it doesn't have to be cleaned up with this
> CL, but it's something to consider for a follow up.

I'll put it on my post 23 todo list.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.h:50:
void Init(GetDeviceInfoFunc getDeviceInfo,
On 2012/09/15 02:33:55, rvargas wrote:
> InitForTest

Done.

[rvargas (doing something else), 2012-09-17 06:10:11]

Note that you really don't need to create a new window just to get a message.
Base' SM power notifications also come from Windows' messages.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54:
if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))
On 2012/09/15 19:45:10, vandebo wrote:
> On 2012/09/15 02:33:55, rvargas wrote:
> > This is not correct. What the blog says is to call the function on the
> "current"
> > drive root first and build the path again from there. Note that the guids
are
> > actually consistent all the time, so most likely you don't have to care
about
> > this issue at all. (but I don't know what's the end purpose of this code).
> 
> I still don't completely follow the blog on this point.  If you mean that
> C:\drive_d_mount will always give one GUID even if D:\ is mounted there and
D:\
> consistently gives a different GUID, then I think it does need to be resolved
> here because of GetDeviceInfoForPath()

But you said below that you only care about one mount point, and that would be
two mount points for d.

OK, if you really have to get to the drive (something smells funny there), then
the issue is that in order to get to c:\drive_d_mount you first have to make
sure c:\ is really c:\... so you'd have to expand c: to a volume name and then
try volumex-y\mount_d_drive

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:93:
result.push_back(FilePath(volume_path));
On 2012/09/15 19:45:10, vandebo wrote:
> On 2012/09/15 02:33:55, rvargas wrote:
> > I assume that you only care about one mount point.
> 
> Correct.  Would you like a comment?

No need, I was just double checking.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:95:
NOTREACHED();
On 2012/09/15 19:45:10, vandebo wrote:
> On 2012/09/15 02:33:55, rvargas wrote:
> > OS calls may (and will) fail at any time
> 
> I think this function will fail gracefully - just skip the volume where the
call
> failed.  I added the NOTREACHED in case it starts consistently failing, a
> developer will notice.

Yeah, the function works fine. It's just that random DCHECKs are annoying when
running debug builds. If I hit this DCHECK and I'm not really working on this
code, I'd remove it, because there's no guarantee that the OS call is not going
to fail, and no side effect. If the failure was significant enough while
developing the code (now), the function should have failed.

If you want to help someone debugging a random failure, a DLOG is a better
option.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:272:
g_removable_device_notifications_window_win = this;
On 2012/09/15 19:45:10, vandebo wrote:
> On 2012/09/15 02:33:55, rvargas wrote:
> > Basically this is a home grown singleton (without the documentation saying
> that
> > it has to be). Use a singleton or a lazy instance.
> 
> I looked at both before and again just now.  There is a subtle difference that
I
> think means I can't use either.  SystemMonitor is scoped object created in
> content BrowserMainLoop.  The AtExitManager is in a scope above that.  So
using
> Singleton or LazyInstance with default traits would clean up this object too
> late.
> 
> Not that it ameliorates the issue here, but it was recently decided (a couple
> days ago) that SystemMonitor has outgrown its design and needs to be
refactored.
>  It's likely that each part of it will be split out into its own class.  So
the
> removable device notification which is updated in this CL will be managed by
an
> object in chrome/browser or will become a Singleton/LazyInstance.

We need at least some documentation stating that there can only be one instance
of this class.

[cpu_(ooo_6.6-7.5), 2012-09-17 17:45:00]

I like Ricardo's idea. power broadcasts we already handle so can we augment that
window to forward the device messages?

[vandebo (ex-Chrome), 2012-09-17 18:51:59]

On 2012/09/17 06:10:11, rvargas wrote:
> Note that you really don't need to create a new window just to get a message.
> Base' SM power notifications also come from Windows' messages.
>
On 2012/09/17 17:45:00, cpu wrote:
> I like Ricardo's idea. power broadcasts we already handle so can we augment
that
> window to forward the device messages?

I'll see what I can do to make that happen.  Not sure if there's enough time to
get something done and tested, so please continue reviewing this CL.

I don't remember exactly, but I suspect that initially this code was done
editing ui/views and there was pushback and it being the wrong place.  The Win8
counter argument is pretty strong though.

No luck testing this on Win8, can't get the Metro mode to work.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54:
if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))
On 2012/09/17 06:10:11, rvargas wrote:
> On 2012/09/15 19:45:10, vandebo wrote:
> > On 2012/09/15 02:33:55, rvargas wrote:
> > > This is not correct. What the blog says is to call the function on the
> > "current"
> > > drive root first and build the path again from there. Note that the guids
> are
> > > actually consistent all the time, so most likely you don't have to care
> about
> > > this issue at all. (but I don't know what's the end purpose of this code).
> > 
> > I still don't completely follow the blog on this point.  If you mean that
> > C:\drive_d_mount will always give one GUID even if D:\ is mounted there and
> D:\
> > consistently gives a different GUID, then I think it does need to be
resolved
> > here because of GetDeviceInfoForPath()
> 
> But you said below that you only care about one mount point, and that would be
> two mount points for d.

In terms of notifying SM about a device attached, we only care about one mount
point, yes.  But if our consumer asks for device info for a given path, we have
to consider any mount point relevant to that path.
 
> OK, if you really have to get to the drive (something smells funny there),
then
> the issue is that in order to get to c:\drive_d_mount you first have to make
> sure c:\ is really c:\... so you'd have to expand c: to a volume name and then
> try volumex-y\mount_d_drive

I see.  Updated.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:95:
NOTREACHED();
On 2012/09/17 06:10:11, rvargas wrote:
> On 2012/09/15 19:45:10, vandebo wrote:
> > On 2012/09/15 02:33:55, rvargas wrote:
> > > OS calls may (and will) fail at any time
> > 
> > I think this function will fail gracefully - just skip the volume where the
> call
> > failed.  I added the NOTREACHED in case it starts consistently failing, a
> > developer will notice.
> 
> Yeah, the function works fine. It's just that random DCHECKs are annoying when
> running debug builds. If I hit this DCHECK and I'm not really working on this
> code, I'd remove it, because there's no guarantee that the OS call is not
going
> to fail, and no side effect. If the failure was significant enough while
> developing the code (now), the function should have failed.
> 
> If you want to help someone debugging a random failure, a DLOG is a better
> option.

Done.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:272:
g_removable_device_notifications_window_win = this;
On 2012/09/17 06:10:11, rvargas wrote:
> On 2012/09/15 19:45:10, vandebo wrote:
> > On 2012/09/15 02:33:55, rvargas wrote:
> > > Basically this is a home grown singleton (without the documentation saying
> > that
> > > it has to be). Use a singleton or a lazy instance.
> > 
> > I looked at both before and again just now.  There is a subtle difference
that
> I
> > think means I can't use either.  SystemMonitor is scoped object created in
> > content BrowserMainLoop.  The AtExitManager is in a scope above that.  So
> using
> > Singleton or LazyInstance with default traits would clean up this object too
> > late.
> > 
> > Not that it ameliorates the issue here, but it was recently decided (a
couple
> > days ago) that SystemMonitor has outgrown its design and needs to be
> refactored.
> >  It's likely that each part of it will be split out into its own class.  So
> the
> > removable device notification which is updated in this CL will be managed by
> an
> > object in chrome/browser or will become a Singleton/LazyInstance.
> 
> We need at least some documentation stating that there can only be one
instance
> of this class.

Done.

[vandebo (ex-Chrome), 2012-09-17 20:34:41]

On 2012/09/17 17:45:00, cpu wrote:
> I like Ricardo's idea. power broadcasts we already handle so can we augment
that
> window to forward the device messages?

I've looked into this a bit.  The most straightforward way to use notifications
from that window is to direct the notifications to SM a'la power broadcasts and
then set the current class (which has dependencies on chrome/browser) as a
delegate in SM to handle it.  That however would require passing the delegate in
the SM constructor (previous discussion with willchan@) which would mean moving
where SM is constructed in content init to platform specific browser init code -
a much more invasive change than this one.

I'm already on the hook for refactoring SystemMonitor before M24. I'm happy to
improve the status-quo here as well, but it seems riskier to try to get it done
today.

[rvargas (doing something else), 2012-09-17 22:26:05]

I don't have more comments.

Another background question: why is it important to know if a volume is
removable or not?

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54:
if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))
On 2012/09/17 18:51:59, vandebo wrote:
> On 2012/09/17 06:10:11, rvargas wrote:
> > On 2012/09/15 19:45:10, vandebo wrote:
> > > On 2012/09/15 02:33:55, rvargas wrote:
> > > > This is not correct. What the blog says is to call the function on the
> > > "current"
> > > > drive root first and build the path again from there. Note that the
guids
> > are
> > > > actually consistent all the time, so most likely you don't have to care
> > about
> > > > this issue at all. (but I don't know what's the end purpose of this
code).
> > > 
> > > I still don't completely follow the blog on this point.  If you mean that
> > > C:\drive_d_mount will always give one GUID even if D:\ is mounted there
and
> > D:\
> > > consistently gives a different GUID, then I think it does need to be
> resolved
> > > here because of GetDeviceInfoForPath()
> > 
> > But you said below that you only care about one mount point, and that would
be
> > two mount points for d.
> 
> In terms of notifying SM about a device attached, we only care about one mount
> point, yes.  But if our consumer asks for device info for a given path, we
have
> to consider any mount point relevant to that path.
>  
> > OK, if you really have to get to the drive (something smells funny there),
> then
> > the issue is that in order to get to c:\drive_d_mount you first have to make
> > sure c:\ is really c:\... so you'd have to expand c: to a volume name and
then
> > try volumex-y\mount_d_drive
> 
> I see.  Updated.

I'm sorry... after thinking this through I realized that what I said doesn't
really makes sense. What you had before is actually correct... not that it makes
sense either, but it works around a bug that was fixed with Vista SP1.

http://codereview.chromium.org/10911234/diff/7045/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/7045/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:110:
DLOG(ERROR) << "GetVolmePathNamesForVolueName() failed. LastError: "
DPLOG

[cpu_(ooo_6.6-7.5), 2012-09-17 22:47:08]

I don't have new comments either. I am talking to him IRC, I still  like the
HWNDMessageHandler::OnPowerBroadcast pattern extended to this case.

[vandebo (ex-Chrome), 2012-09-17 23:09:15]

On 2012/09/17 22:26:05, rvargas wrote:
> I don't have more comments.
> 
> Another background question: why is it important to know if a volume is
> removable or not?

It's more important on other platforms, but basically it's part of permissions.
An extension can get access "media devices" where we device media devices as
removable storage that is MTP/PTP or has a DCIM directory.

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/4036/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:54:
if (!GetVolumeNameForVolumeMountPoint(guid, guid, kMaxPathBufLen))
On 2012/09/17 22:26:06, rvargas wrote:
> I'm sorry... after thinking this through I realized that what I said doesn't
> really makes sense. What you had before is actually correct... not that it
makes
> sense either, but it works around a bug that was fixed with Vista SP1.

Changed back.

http://codereview.chromium.org/10911234/diff/7045/chrome/browser/system_monit...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/7045/chrome/browser/system_monit...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:110:
DLOG(ERROR) << "GetVolmePathNamesForVolueName() failed. LastError: "
On 2012/09/17 22:26:06, rvargas wrote:
> DPLOG

Done.

[rvargas (doing something else), 2012-09-17 23:32:13]

lgtm

[Lei Zhang, 2012-09-18 00:06:26]

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
chrome/browser/system_monitor/media_storage_util.cc:252: #if !defined(POSIX)
OS_POSIX

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:71:
UINT type = GetDriveType(mount_point);
nit: merge with next line?

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:93:
DPLOG(ERROR);
I haven't looked at this closely, but it stands out when we don't need to
continue / break on error.

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:302:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
nit: This an be better formatted to show the arguments on lines 303 and 304 are
for Bind and not PostTask:

BrowserThread::PostTask(
    BrowserThread::FILE,
    FROM_HERE,
    base::Bind(
        &RemovableDeviceNotificationsWindowWin::CheckDeviceTypeOnFileThread,
...));

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
File chrome/browser/system_monitor/removable_device_notifications_window_win.h
(right):

https://chromiumcodereview.appspot.com/10911234/diff/11016/chrome/browser/sys...
chrome/browser/system_monitor/removable_device_notifications_window_win.h:43: //
|this| is constructored/destroyed just after/before SystemMonitor.
nit: only evil constructicons like to constructored, autobots are constructed.

[vandebo (ex-Chrome), 2012-09-18 00:16:21]

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util.cc (right):

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.cc:252: #if !defined(POSIX)
On 2012/09/18 00:06:26, Lei Zhang wrote:
> OS_POSIX

Done.

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:71:
UINT type = GetDriveType(mount_point);
On 2012/09/18 00:06:26, Lei Zhang wrote:
> nit: merge with next line?

Done.

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:93:
DPLOG(ERROR);
On 2012/09/18 00:06:26, Lei Zhang wrote:
> I haven't looked at this closely, but it stands out when we don't need to
> continue / break on error.

The iterator increment is right after the if, so this is the right thing.

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.cc:302:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
On 2012/09/18 00:06:26, Lei Zhang wrote:
> nit: This an be better formatted to show the arguments on lines 303 and 304
are
> for Bind and not PostTask:
> 
> BrowserThread::PostTask(
>     BrowserThread::FILE,
>     FROM_HERE,
>     base::Bind(
>         &RemovableDeviceNotificationsWindowWin::CheckDeviceTypeOnFileThread,
> ...));

Done.

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_window_win.h
(right):

http://codereview.chromium.org/10911234/diff/11016/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_window_win.h:43: //
|this| is constructored/destroyed just after/before SystemMonitor.
On 2012/09/18 00:06:26, Lei Zhang wrote:
> nit: only evil constructicons like to constructored, autobots are constructed.

Done.

[Lei Zhang, 2012-09-18 00:31:00]

lgtm

[cpu_(ooo_6.6-7.5), 2012-09-18 01:03:35]

lgtm if you code does not explode on win8 metro mode.

[vandebo (ex-Chrome), 2012-09-19 23:13:05]

On 2012/09/18 01:03:35, cpu wrote:
> lgtm if you code does not explode on win8 metro mode.

With the new dev channel that was spun yesterday I was finally able to test this
change under metro mode.  Not only does it not explode, it actually works just
fine.  That's not to say that I'm going to drop any promises I made -
SystemMonitor still needs to be refactored and it makes sense to make a general
mechanism to get window broadcast messages from the main window instead of
creating a hidden one.