Refactor MountedDiskMonitior and adds its test.

Refactor MountedDiskMonitior and adds its test.

As a preparation of VolumeManager refactoring, this CL allows injecting
DiskMountManager to MounteddiskMonitor. Along with the change, this CL also
adds unittest for it.

BUG=279276
TEST=Ran unit_tests

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

[hidehiko, 2013-09-09 11:14:25]

Thank you for your review in advance,
- hidehiko

[hashimoto, 2013-09-09 12:15:17]

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
File chrome/browser/chromeos/file_manager/mounted_disk_monitor.h (right):

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor.h:61: void
SetResumingTimeSpanForTesting(
nit: set_resuming_time_span_for_testing because this method is inlined?

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
File chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc
(right):

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:18: class
FakePowerManagerClient : public chromeos::PowerManagerClient {
I think you can use the one declared in
chromeos/dbus/fake_power_manager_client.h

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:135: 
nit: Could you briefly describe what kind of scenario is tested here?

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:136:
TEST_F(MountedDiskMonitorTest, ScenarioTest) {
nit: ScenarioTest sounds too general. What if we want to add new tests later?
How about renaming this test to something like BasicScenarioTest or
SuspendAndResume?

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:138: new
chromeos::disks::DiskMountManager::Disk(
nit: Could you make it easier to understand what do these parameters mean?

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:142: new
chromeos::disks::DiskMountManager::Disk(
ditto.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:172:
EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk1));
Could you briefly describe what do you want to test with this first pair of
mount and unmount? Or, how about separating this part to another test?

[hidehiko, 2013-09-09 12:44:10]

Thank you for your review. PTAL?

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
File chrome/browser/chromeos/file_manager/mounted_disk_monitor.h (right):

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor.h:61: void
SetResumingTimeSpanForTesting(
On 2013/09/09 12:15:17, hashimoto wrote:
> nit: set_resuming_time_span_for_testing because this method is inlined?

Done.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
File chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc
(right):

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:18: class
FakePowerManagerClient : public chromeos::PowerManagerClient {
On 2013/09/09 12:15:17, hashimoto wrote:
> I think you can use the one declared in
> chromeos/dbus/fake_power_manager_client.h

Great to know. I checked once, but I seemed to overlook it...
Done.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:135: 
On 2013/09/09 12:15:17, hashimoto wrote:
> nit: Could you briefly describe what kind of scenario is tested here?

Done.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:136:
TEST_F(MountedDiskMonitorTest, ScenarioTest) {
On 2013/09/09 12:15:17, hashimoto wrote:
> nit: ScenarioTest sounds too general. What if we want to add new tests later?
> How about renaming this test to something like BasicScenarioTest or
> SuspendAndResume?

SuspendAndResume sounds better.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:138: new
chromeos::disks::DiskMountManager::Disk(
On 2013/09/09 12:15:17, hashimoto wrote:
> nit: Could you make it easier to understand what do these parameters mean?

Extracted into a utility function.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:142: new
chromeos::disks::DiskMountManager::Disk(
On 2013/09/09 12:15:17, hashimoto wrote:
> ditto.

Done.

https://codereview.chromium.org/23676008/diff/1/chrome/browser/chromeos/file_...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:172:
EXPECT_FALSE(mounted_disk_monitor_->DiskIsRemounting(*disk1));
On 2013/09/09 12:15:17, hashimoto wrote:
> Could you briefly describe what do you want to test with this first pair of
> mount and unmount? Or, how about separating this part to another test?

Done.

[hashimoto, 2013-09-09 14:34:23]

lgtm

https://codereview.chromium.org/23676008/diff/7001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc
(right):

https://codereview.chromium.org/23676008/diff/7001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:119: //
The ownership of Disks are taken by DiskMountManager.
nit: Now this comment is not needed because scoped_ptr is used? The same goes
for the comment in another test.

[hidehiko, 2013-09-09 15:18:54]

Thank you for your review! Sending to CQ.

https://codereview.chromium.org/23676008/diff/7001/chrome/browser/chromeos/fi...
File chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc
(right):

https://codereview.chromium.org/23676008/diff/7001/chrome/browser/chromeos/fi...
chrome/browser/chromeos/file_manager/mounted_disk_monitor_unittest.cc:119: //
The ownership of Disks are taken by DiskMountManager.
On 2013/09/09 14:34:24, hashimoto wrote:
> nit: Now this comment is not needed because scoped_ptr is used? The same goes
> for the comment in another test.

Done.

[commit-bot: I haz the power, 2013-09-09 15:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23676008/19001

[commit-bot: I haz the power, 2013-09-09 19:44:55]

Change committed as 222072