Skip to content

Commit

Permalink
Return fixed storage from StorageMonitor::GetAllAvailableStorages(). …
Browse files Browse the repository at this point in the history 
…(1/2)

Fetch information about all devices from OS in the DiskMountManager.
Ignore virtual block devices.

Split disk changes callback into separate callbacks for auto mountable
and fixed storage disks, so clients that are only interested
in auto mountable devices do not need to do additional filtering.


Bug: 420633
Change-Id: I3380c1a0ff49d7167760633cc3ed30d151170a08
Reviewed-on: https://chromium-review.googlesource.com/756273
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521019}
  • Loading branch information
Aga Wronska authored and Commit Bot committed Dec 1, 2017
1 parent 7284655 commit ddc1a75
Show file tree
Hide file tree
Showing 18 changed files with 280 additions and 141 deletions.
9 changes: 6 additions & 3 deletions chrome/browser/chromeos/app_mode/kiosk_external_updater.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,13 @@ KioskExternalUpdater::~KioskExternalUpdater() {
disks::DiskMountManager::GetInstance()->RemoveObserver(this);
}

void KioskExternalUpdater::OnDiskEvent(
void KioskExternalUpdater::OnAutoMountableDiskEvent(
disks::DiskMountManager::DiskEvent event,
const disks::DiskMountManager::Disk* disk) {
}
const disks::DiskMountManager::Disk& disk) {}

void KioskExternalUpdater::OnBootDeviceDiskEvent(
disks::DiskMountManager::DiskEvent event,
const disks::DiskMountManager::Disk& disk) {}

void KioskExternalUpdater::OnDeviceEvent(
disks::DiskMountManager::DeviceEvent event,
Expand Down
8 changes: 6 additions & 2 deletions chrome/browser/chromeos/app_mode/kiosk_external_updater.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,12 @@ class KioskExternalUpdater : public disks::DiskMountManager::Observer,
};

// disks::DiskMountManager::Observer overrides.
void OnDiskEvent(disks::DiskMountManager::DiskEvent event,
const disks::DiskMountManager::Disk* disk) override;
void OnAutoMountableDiskEvent(
disks::DiskMountManager::DiskEvent event,
const disks::DiskMountManager::Disk& disk) override;
void OnBootDeviceDiskEvent(
disks::DiskMountManager::DiskEvent event,
const disks::DiskMountManager::Disk& disk) override;
void OnDeviceEvent(disks::DiskMountManager::DeviceEvent event,
const std::string& device_path) override;
void OnMountEvent(
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/chromeos/file_manager/fake_disk_mount_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,10 @@ bool FakeDiskMountManager::AddMountPointForTest(
void FakeDiskMountManager::InvokeDiskEventForTest(
chromeos::disks::DiskMountManager::DiskEvent event,
const chromeos::disks::DiskMountManager::Disk* disk) {
for (auto& observer : observers_)
observer.OnDiskEvent(event, disk);
for (auto& observer : observers_) {
disk->IsAutoMountable() ? observer.OnAutoMountableDiskEvent(event, *disk)
: observer.OnBootDeviceDiskEvent(event, *disk);
}
}

} // namespace file_manager
29 changes: 16 additions & 13 deletions chrome/browser/chromeos/file_manager/volume_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -530,66 +530,69 @@ void VolumeManager::OnFileSystemBeingUnmounted() {
DoUnmountEvent(chromeos::MOUNT_ERROR_NONE, *Volume::CreateForDrive(profile_));
}

void VolumeManager::OnDiskEvent(
void VolumeManager::OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DiskEvent event,
const chromeos::disks::DiskMountManager::Disk* disk) {
const chromeos::disks::DiskMountManager::Disk& disk) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

// Disregard hidden devices.
if (disk->is_hidden())
if (disk.is_hidden())
return;

switch (event) {
case chromeos::disks::DiskMountManager::DISK_ADDED:
case chromeos::disks::DiskMountManager::DISK_CHANGED: {
if (disk->device_path().empty()) {
DVLOG(1) << "Empty system path for " << disk->device_path();
if (disk.device_path().empty()) {
DVLOG(1) << "Empty system path for " << disk.device_path();
return;
}

bool mounting = false;
if (disk->mount_path().empty() && disk->has_media() &&
if (disk.mount_path().empty() && disk.has_media() &&
!profile_->GetPrefs()->GetBoolean(prefs::kExternalStorageDisabled)) {
// TODO(crbug.com/774890): Remove |mount_label| when the issue gets
// resolved. Currently we suggest a mount point name, because in case
// when disk's name contains '#', content will not load in Files App.
std::string mount_label = disk->device_label();
std::string mount_label = disk.device_label();
std::replace(mount_label.begin(), mount_label.end(), '#', '_');

// If disk is not mounted yet and it has media and there is no policy
// forbidding external storage, give it a try.
// Initiate disk mount operation. MountPath auto-detects the filesystem
// format if the second argument is empty. The third argument (mount
// label) is not used in a disk mount operation.
disk_mount_manager_->MountPath(disk->device_path(), std::string(),
disk_mount_manager_->MountPath(disk.device_path(), std::string(),
mount_label, chromeos::MOUNT_TYPE_DEVICE,
GetExternalStorageAccessMode(profile_));
mounting = true;
}

// Notify to observers.
for (auto& observer : observers_)
observer.OnDiskAdded(*disk, mounting);
observer.OnDiskAdded(disk, mounting);
return;
}

case chromeos::disks::DiskMountManager::DISK_REMOVED:
// If the disk is already mounted, unmount it.
if (!disk->mount_path().empty()) {
if (!disk.mount_path().empty()) {
disk_mount_manager_->UnmountPath(
disk->mount_path(),
chromeos::UNMOUNT_OPTIONS_LAZY,
disk.mount_path(), chromeos::UNMOUNT_OPTIONS_LAZY,
chromeos::disks::DiskMountManager::UnmountPathCallback());
}

// Notify to observers.
for (auto& observer : observers_)
observer.OnDiskRemoved(*disk);
observer.OnDiskRemoved(disk);
return;
}
NOTREACHED();
}

void VolumeManager::OnBootDeviceDiskEvent(
chromeos::disks::DiskMountManager::DiskEvent event,
const chromeos::disks::DiskMountManager::Disk& disk) {}

void VolumeManager::OnDeviceEvent(
chromeos::disks::DiskMountManager::DeviceEvent event,
const std::string& device_path) {
Expand Down
7 changes: 5 additions & 2 deletions chrome/browser/chromeos/file_manager/volume_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,12 @@ class VolumeManager : public KeyedService,
void OnFileSystemBeingUnmounted() override;

// chromeos::disks::DiskMountManager::Observer overrides.
void OnDiskEvent(
void OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DiskEvent event,
const chromeos::disks::DiskMountManager::Disk* disk) override;
const chromeos::disks::DiskMountManager::Disk& disk) override;
void OnBootDeviceDiskEvent(
chromeos::disks::DiskMountManager::DiskEvent event,
const chromeos::disks::DiskMountManager::Disk& disk) override;
void OnDeviceEvent(chromeos::disks::DiskMountManager::DeviceEvent event,
const std::string& device_path) override;
void OnMountEvent(chromeos::disks::DiskMountManager::MountEvent event,
Expand Down
93 changes: 58 additions & 35 deletions chrome/browser/chromeos/file_manager/volume_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,31 @@ TEST_F(VolumeManagerTest, OnDriveFileSystemUnmountWithoutMount) {
ASSERT_EQ(0U, observer.events().size());
volume_manager()->RemoveObserver(&observer);
}
TEST_F(VolumeManagerTest, OnBootDeviceDiskEvent) {
LoggingObserver observer;
volume_manager()->AddObserver(&observer);

const chromeos::disks::DiskMountManager::Disk disk(
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, false, true, false, false,
"", "");

volume_manager()->OnBootDeviceDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, disk);
EXPECT_EQ(0U, observer.events().size());

volume_manager()->OnBootDeviceDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, disk);
EXPECT_EQ(0U, observer.events().size());

volume_manager()->OnBootDeviceDiskEvent(
chromeos::disks::DiskMountManager::DISK_CHANGED, disk);
EXPECT_EQ(0U, observer.events().size());

volume_manager()->RemoveObserver(&observer);
}

TEST_F(VolumeManagerTest, OnDiskEvent_Hidden) {
TEST_F(VolumeManagerTest, OnAutoMountableDiskEvent_Hidden) {
LoggingObserver observer;
volume_manager()->AddObserver(&observer);

Expand All @@ -267,22 +290,22 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Hidden) {
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, false, false, false,
kIsHidden, "", "");

volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kDisk);
EXPECT_EQ(0U, observer.events().size());

volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, &kDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, kDisk);
EXPECT_EQ(0U, observer.events().size());

volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_CHANGED, &kDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_CHANGED, kDisk);
EXPECT_EQ(0U, observer.events().size());

volume_manager()->RemoveObserver(&observer);
}

TEST_F(VolumeManagerTest, OnDiskEvent_Added) {
TEST_F(VolumeManagerTest, OnAutoMountableDiskEvent_Added) {
// Enable external storage.
profile()->GetPrefs()->SetBoolean(prefs::kExternalStorageDisabled, false);

Expand All @@ -294,17 +317,17 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Added) {
"", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, false, false, false,
false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kEmptyDevicePathDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kEmptyDevicePathDisk);
EXPECT_EQ(0U, observer.events().size());

const bool kHasMedia = true;
const chromeos::disks::DiskMountManager::Disk kMediaDisk(
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, kHasMedia, false, false,
false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kMediaDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kMediaDisk);
ASSERT_EQ(1U, observer.events().size());
const LoggingObserver::Event& event = observer.events()[0];
EXPECT_EQ(LoggingObserver::Event::DISK_ADDED, event.type);
Expand All @@ -322,7 +345,7 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Added) {
volume_manager()->RemoveObserver(&observer);
}

TEST_F(VolumeManagerTest, OnDiskEvent_AddedNonMounting) {
TEST_F(VolumeManagerTest, OnAutoMountableDiskEvent_AddedNonMounting) {
// Enable external storage.
profile()->GetPrefs()->SetBoolean(prefs::kExternalStorageDisabled, false);

Expand All @@ -336,8 +359,8 @@ TEST_F(VolumeManagerTest, OnDiskEvent_AddedNonMounting) {
"device1", "mounted", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, kHasMedia, false, false,
false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kMountedMediaDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kMountedMediaDisk);
ASSERT_EQ(1U, observer.events().size());
const LoggingObserver::Event& event = observer.events()[0];
EXPECT_EQ(LoggingObserver::Event::DISK_ADDED, event.type);
Expand All @@ -359,8 +382,8 @@ TEST_F(VolumeManagerTest, OnDiskEvent_AddedNonMounting) {
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, kWithoutMedia, false,
false, false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kNoMediaDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kNoMediaDisk);
ASSERT_EQ(1U, observer.events().size());
const LoggingObserver::Event& event = observer.events()[0];
EXPECT_EQ(LoggingObserver::Event::DISK_ADDED, event.type);
Expand All @@ -384,8 +407,8 @@ TEST_F(VolumeManagerTest, OnDiskEvent_AddedNonMounting) {
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, kHasMedia, false, false,
false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kMediaDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kMediaDisk);
ASSERT_EQ(1U, observer.events().size());
const LoggingObserver::Event& event = observer.events()[0];
EXPECT_EQ(LoggingObserver::Event::DISK_ADDED, event.type);
Expand All @@ -398,16 +421,16 @@ TEST_F(VolumeManagerTest, OnDiskEvent_AddedNonMounting) {
}
}

TEST_F(VolumeManagerTest, OnDiskEvent_Removed) {
TEST_F(VolumeManagerTest, OnDiskAutoMountableEvent_Removed) {
LoggingObserver observer;
volume_manager()->AddObserver(&observer);

const chromeos::disks::DiskMountManager::Disk kMountedDisk(
"device1", "mount_path", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, false, false, false,
false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, &kMountedDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, kMountedDisk);

ASSERT_EQ(1U, observer.events().size());
const LoggingObserver::Event& event = observer.events()[0];
Expand All @@ -423,16 +446,16 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Removed) {
volume_manager()->RemoveObserver(&observer);
}

TEST_F(VolumeManagerTest, OnDiskEvent_RemovedNotMounted) {
TEST_F(VolumeManagerTest, OnAutoMountableDiskEvent_RemovedNotMounted) {
LoggingObserver observer;
volume_manager()->AddObserver(&observer);

const chromeos::disks::DiskMountManager::Disk kNotMountedDisk(
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, false, false, false,
false, "", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, &kNotMountedDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_REMOVED, kNotMountedDisk);

ASSERT_EQ(1U, observer.events().size());
const LoggingObserver::Event& event = observer.events()[0];
Expand All @@ -444,7 +467,7 @@ TEST_F(VolumeManagerTest, OnDiskEvent_RemovedNotMounted) {
volume_manager()->RemoveObserver(&observer);
}

TEST_F(VolumeManagerTest, OnDiskEvent_Changed) {
TEST_F(VolumeManagerTest, OnAutoMountableDiskEvent_Changed) {
// Changed event should cause mounting (if possible).
LoggingObserver observer;
volume_manager()->AddObserver(&observer);
Expand All @@ -453,8 +476,8 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Changed) {
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, true, false, false, false,
"", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_CHANGED, &kDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_CHANGED, kDisk);

EXPECT_EQ(1U, observer.events().size());
EXPECT_EQ(1U, disk_mount_manager_->mount_requests().size());
Expand All @@ -466,7 +489,7 @@ TEST_F(VolumeManagerTest, OnDiskEvent_Changed) {
volume_manager()->RemoveObserver(&observer);
}

TEST_F(VolumeManagerTest, OnDiskEvent_ChangedInReadonly) {
TEST_F(VolumeManagerTest, OnAutoMountableDiskEvent_ChangedInReadonly) {
profile()->GetPrefs()->SetBoolean(prefs::kExternalStorageReadOnly, true);

// Changed event should cause mounting (if possible).
Expand All @@ -477,8 +500,8 @@ TEST_F(VolumeManagerTest, OnDiskEvent_ChangedInReadonly) {
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, true, false, false, false,
"", "");
volume_manager()->OnDiskEvent(chromeos::disks::DiskMountManager::DISK_CHANGED,
&kDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_CHANGED, kDisk);

EXPECT_EQ(1U, observer.events().size());
EXPECT_EQ(1U, disk_mount_manager_->mount_requests().size());
Expand Down Expand Up @@ -783,10 +806,10 @@ TEST_F(VolumeManagerTest, ExternalStorageDisabledPolicyMultiProfile) {
"device1", "", false, "", "", "", "", "", "", "", "", "", "",
chromeos::DEVICE_TYPE_UNKNOWN, 0, false, false, true, false, false, false,
"", "");
volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kMediaDisk);
secondary.volume_manager()->OnDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, &kMediaDisk);
volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kMediaDisk);
secondary.volume_manager()->OnAutoMountableDiskEvent(
chromeos::disks::DiskMountManager::DISK_ADDED, kMediaDisk);

// The profile with external storage enabled should have mounted the volume.
bool has_volume_mounted = false;
Expand Down
Loading

0 comments on commit ddc1a75

Please sign in to comment.