Refine StorageMonitor storage device getter

Refine StorageMonitor storage device getter
    
* Make GetAttachedStorage to return all available storages, including fixed and removable storages, instead of only removable storages.

* Rename GetAttachedStorage to GetAllAvailableStorages for better understanding.
    
BUG=None
TEST=unit_tests --gtest_filter=StorageMonitorTest.*
TEST=unit_tests --gtest_filter=StorageMonitorLinuxTest.*
TEST=unit_tests --gtest_filter=StorageMonitorWinTest.*
TEST=browser_tests --gtest_filter=SystemInfoStorageApiTest.*

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

[Haojian Wu, 2013-06-02 13:57:24]

Hi, reviewers. I'm one of students in Chromium GSoC this year. I will co-work
with my mentor hongbo@ on the systemInfo.storage API this summer. After
discussed with hongbo, I will take over CL
https://codereview.chromium.org/16186002/ from hongbo@ as first step in my GSoC
program. This CL is just copied from the CL
https://codereview.chromium.org/16186002/ and will replace it.
Any help and advice will be greatly appreciate.

[Hongbo Min, 2013-06-03 01:34:41]

Haojian, according to the Greg's comments, we won't introduce a new method to
return all known storages, instead, we will make GetAttachedStorage method
return all known storages, not only removable storages since StorageInfo has
enough information to determine if a storage is removable.

Could you please update the CL and the commit description correspondingly?
Thanks.

[Haojian Wu, 2013-06-03 12:36:20]

On 2013/06/03 01:34:41, Hongbo Min wrote:
> Haojian, according to the Greg's comments, we won't introduce a new method to
> return all known storages, instead, we will make GetAttachedStorage method
> return all known storages, not only removable storages since StorageInfo has
> enough information to determine if a storage is removable.
> 
> Could you please update the CL and the commit description correspondingly?
> Thanks.

Done. The CL has been updated now.

[vandebo (ex-Chrome), 2013-06-03 15:46:16]

On 2013/06/02 13:57:24, Haojian Wu wrote:
> Hi, reviewers. I'm one of students in Chromium GSoC this year. I will co-work
> with my mentor hongbo@ on the systemInfo.storage API this summer. After
> discussed with hongbo, I will take over CL
> https://codereview.chromium.org/16186002/ from hongbo@ as first step in my
GSoC
> program. This CL is just copied from the CL
> https://codereview.chromium.org/16186002/ and will replace it.
> Any help and advice will be greatly appreciate.

Hi Haojian, welcome to the project. When sending a change out for review to
multiple reviewers, you should indicate what you'd like each reviewer to review.
 If all reviewers review the whole thing, it takes more time.  Since you're new
to the project you may also want to have Hongbo do initial reviews until the
change looks close.

[Haojian Wu, 2013-06-04 02:44:30]

Thanks for the tips. Actually I'm not exactly clear about indicating who to
review different parts of the CL since I don't know the part of code which each
reviewer maintains. I will have a try.

The CL includes the platform-specific impls, related unit test code, and
consumer code of GetAttachedStorage.

hongbo@ please do initial reviews. vandebo@, Greg@ please review the big change
of GetAttachedStorage function. lei@ please review platform-specific impls
changes.

[Hongbo Min, 2013-06-04 02:52:28]

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/storage_monitor.h:147: typedef
std::map<std::string, StorageInfo> AllAvailableStorageMap;
StorageMap might be a better name.

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/storage_monitor_linux.cc (left):

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/storage_monitor_linux.cc:513: if (removable)
why do you remove this if clause?

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/storage_monitor_mac.mm (left):

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/storage_monitor_mac.mm:356:
StorageInfo::IsRemovableDevice(info.device_id()) &&
Why to remove it?

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/volume_mount_watcher_win.cc (left):

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/volume_mount_watcher_win.cc:494: if
(!StorageInfo::IsRemovableDevice(info.device_id()))
Don't try to remove this if clause, otherwise, the fixed volumes will also
trigger attach notification.

[Hongbo Min, 2013-06-04 02:53:56]

Haojian, thanks for your effort on this.

BTW, do you have right to run trybots?

[Haojian Wu, 2013-06-04 03:01:10]

On 2013/06/04 02:53:56, Hongbo Min wrote:
> Haojian, thanks for your effort on this.
> 
> BTW, do you have right to run trybots?

I suppose not. I clicked the trybots for a try just now, but does no effect. It
seems only committers can have the right. 

I think it would be better if I can use the trybots since I only have
linux/windows environment right now.

[Haojian Wu, 2013-06-04 03:52:09]

About removing the judging statement of removable device in platform-specific
impls. Here are explanation:

In StorageMonitor imples, ProcessAttach will add a new StorageInfo into the
storage_map_.
Since GetAttachedStorage function now returns all known storages instead of
removable only, so we should notificate receivers to make StorageMonitor add new
storageInfo to the storage_map_ when a storage device is attached no matter
whether it's fixed or removable.

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
File chrome/browser/storage_monitor/storage_monitor.h (right):

https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_mon...
chrome/browser/storage_monitor/storage_monitor.h:147: typedef
std::map<std::string, StorageInfo> AllAvailableStorageMap;
On 2013/06/04 02:52:29, Hongbo Min wrote:
> StorageMap might be a better name.

Done.

[Greg Billock, 2013-06-04 05:37:25]

On Mon, Jun 3, 2013 at 8:52 PM, <Hokein.Wu@gmail.com> wrote:

> About removing the judging statement of removable device in
> platform-specific
> impls. Here are explanation:
>
> In StorageMonitor imples, ProcessAttach will add a new StorageInfo into the
> storage_map_.
> Since GetAttachedStorage function now returns all known storages instead of
> removable only, so we should notificate receivers to make StorageMonitor
> add new
> storageInfo to the storage_map_ when a storage device is attached no matter
> whether it's fixed or removable.
>
>
agreed. This will require some adjustment in consumers.


>
>
> https://codereview.chromium.**org/15988011/diff/4001/chrome/**
>
browser/storage_monitor/**storage_monitor.h<https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_monitor/storage_monitor.h>
> File chrome/browser/storage_**monitor/storage_monitor.h (right):
>
> https://codereview.chromium.**org/15988011/diff/4001/chrome/**
>
browser/storage_monitor/**storage_monitor.h#newcode147<https://codereview.chromium.org/15988011/diff/4001/chrome/browser/storage_monitor/storage_monitor.h#newcode147>
> chrome/browser/storage_**monitor/storage_monitor.h:147: typedef
> std::map<std::string, StorageInfo> AllAvailableStorageMap;
> On 2013/06/04 02:52:29, Hongbo Min wrote:
>
>> StorageMap might be a better name.
>>
>
> Done.
>
>
https://codereview.chromium.**org/15988011/<https://codereview.chromium.org/1...
>

[Hongbo Min, 2013-06-05 09:23:25]

LGTM. Thanks.

[Haojian Wu, 2013-06-06 05:47:58]

Ping:

vandebo@ for GetAttachedStorage function.
lei@ for platform-specific impls and unittests.
Greg@ for adjustment in GetAttachedStorage consumers and everything else.

Thanks.

[Lei Zhang, 2013-06-06 10:15:07]

lgtm once you address the minor issues below:

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_storage_util.cc:57: if (it->device_id() ==
id && StorageInfo::IsRemovableDevice(id))
Since |id| is known already, you can check if it is not removable first, and
possibly immediately return false.

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.cc (right):

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.cc:162: observer_list_->Notify(
nit:need {}s around this block

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.cc:178: if
(StorageInfo::IsRemovableDevice(info.device_id()))
same style nit as above.

[Haojian Wu, 2013-06-06 11:24:02]

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/media_storage_util.cc:57: if (it->device_id() ==
id && StorageInfo::IsRemovableDevice(id))
On 2013/06/06 10:15:09, Lei Zhang wrote:
> Since |id| is known already, you can check if it is not removable first, and
> possibly immediately return false.

Done.

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
File chrome/browser/storage_monitor/storage_monitor.cc (right):

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.cc:162: observer_list_->Notify(
On 2013/06/06 10:15:09, Lei Zhang wrote:
> nit:need {}s around this block

Done.

https://codereview.chromium.org/15988011/diff/11002/chrome/browser/storage_mo...
chrome/browser/storage_monitor/storage_monitor.cc:178: if
(StorageInfo::IsRemovableDevice(info.device_id()))
On 2013/06/06 10:15:09, Lei Zhang wrote:
> same style nit as above.

Done.

[Haojian Wu, 2013-06-10 10:30:39]

ping vandebo@, Greg@. Thanks.

[wrong vandebo, 2013-06-11 22:58:27]

Lei looked at the whole CL, so you're good.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/15988011/24001

[commit-bot: I haz the power, 2013-06-12 05:22:47]

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

[commit-bot: I haz the power, 2013-06-12 10:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/15988011/31002

[commit-bot: I haz the power, 2013-06-12 13:40:32]

Change committed as 205758