Eliminate getting disk metadata for floppy drives.

Eliminate getting disk metadata for floppy drives.

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

[vandebo (ex-Chrome), 2013-03-06 00:38:26]

https://codereview.chromium.org/12450005/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/volume_mount_watcher_win.cc (right):

https://codereview.chromium.org/12450005/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:36: bool
IsRemovable(const string16& mount_point) {
Instead of splitting this into two functions that basically do the same thing,
how about changing it to DeviceType GetDeviceType(...); enum DeviceType {
Floppy, Removable, Fixed}; ?

Line 94 can then set the removable boolean as well as skip the rest if it's a
floppy and line 158 can test for exactly removable (which was the old definition
of IsRemovable).

https://codereview.chromium.org/12450005/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:158: if
(IsRemovable(volume_path))
This call site still wants the old notion of removable.

[Greg Billock, 2013-03-06 01:40:03]

https://codereview.chromium.org/12450005/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/volume_mount_watcher_win.cc (right):

https://codereview.chromium.org/12450005/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:36: bool
IsRemovable(const string16& mount_point) {
On 2013/03/06 00:38:26, vandebo wrote:
> Instead of splitting this into two functions that basically do the same thing,
> how about changing it to DeviceType GetDeviceType(...); enum DeviceType {
> Floppy, Removable, Fixed}; ?
> 
> Line 94 can then set the removable boolean as well as skip the rest if it's a
> floppy and line 158 can test for exactly removable (which was the old
definition
> of IsRemovable).

I like that plan.

https://codereview.chromium.org/12450005/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:158: if
(IsRemovable(volume_path))
On 2013/03/06 00:38:26, vandebo wrote:
> This call site still wants the old notion of removable.

Yes. I think we can accommodate that.

[vandebo (ex-Chrome), 2013-03-06 01:49:08]

https://codereview.chromium.org/12450005/diff/3001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/volume_mount_watcher_win.cc (right):

https://codereview.chromium.org/12450005/diff/3001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:28: FIXED
nit: add , at end so that future additions don't have to change this line.

https://codereview.chromium.org/12450005/diff/3001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:155: if
(GetDeviceType(volume_path) == REMOVABLE)
Actually I'm not sure we want this at all.  If we expect all devices to be in
cache, then we should query all existing devices on startup.

[vandebo (ex-Chrome), 2013-03-06 01:49:27]

LGTM with above comments/thoughts.

[Greg Billock, 2013-03-06 18:33:24]

https://codereview.chromium.org/12450005/diff/3001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/volume_mount_watcher_win.cc (right):

https://codereview.chromium.org/12450005/diff/3001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:28: FIXED
On 2013/03/06 01:49:08, vandebo wrote:
> nit: add , at end so that future additions don't have to change this line.

Done.

https://codereview.chromium.org/12450005/diff/3001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:155: if
(GetDeviceType(volume_path) == REMOVABLE)
On 2013/03/06 01:49:08, vandebo wrote:
> Actually I'm not sure we want this at all.  If we expect all devices to be in
> cache, then we should query all existing devices on startup.

That makes sense. Let's get rid of it. Although will we end up hanging on remote
network drives or anything like that then? Perhaps we should do that separately
to make sure we're ok. But more to the point, shouldn't this have resulted in no
floppy seeks before? I thought I had figured out that we ended up getting
metadata for all volumes, but this looks like it should skip floppies on
startup. Do you think there's a hole in our floppy detection logic? It looks a
bit ad hoc.

Also, should we treat CDROM device type the same way? It's a separate code, and
we don't want to spin the CDROM.

[commit-bot: I haz the power, 2013-03-06 22:10:26]

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

[commit-bot: I haz the power, 2013-03-07 01:03:51]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-07 17:27:26]

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

[commit-bot: I haz the power, 2013-03-07 19:41:29]

Change committed as 186762