Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
207383004:
Adds a new removable storage provider for imageWriterPrivate on Chrome OS (Closed)
|
Created:
6 years, 9 months ago by Drew Haven Modified:
6 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, extensions-reviews_chromium.org, stephenlin1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionOn some Chrome OS devices there are drives that appear removable to the user but are listed in the system as fixed. This is the case with SD cards on parrot, where they are exposed as a fixed mmcblk device.
This CL creates a new Chrome OS implementation for listing removable drives that uses the chromeos::disks::DiskMountManager, similar to how the Chrome OS ImageBurner is set up.
BUG=352430
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260455
Patch Set 1 #
Total comments: 18
Patch Set 2 : Integrates review feedback: scoped_ptr abound! #Patch Set 3 : Simplifies MockDiskMountManager changes. #Patch Set 4 : Updates test checks to avoid ordering issues. #
Total comments: 4
Patch Set 5 : Some more test refactoring. #
Total comments: 10
Patch Set 6 : Small formatting update. #
Messages
Total messages: 33 (0 generated)
Satoru, Could you take a look at this CL for me? I added a small method to the MockDiskMountManager to facilitate my tests. Hopefully that's acceptable. Thanks, Drew
Toni, Would you mind reviewing this change for me? It turns out I should looked more at the image burner code when setting up the recovery tool. Drew
https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos.cc (right): https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. s/2013/2014 https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos.cc:14: add // static https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc:30: disk_mount_manager_mock_ = new MockDiskMountManager(); Mock is not really what you need here, but since we don't have a fake for DiskMountManager, I guess it's OK. https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc:106: provider->GetAllDevices( GetAllDevices is static, right? I'd suggest calling RemovableStorageProvider::GetAllDevices() (and dropping |provider| pointer) https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... File chromeos/disks/mock_disk_mount_manager.h (right): https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:18: can you file a bug against me to replace MockDisakManager with a fake implementation; and add a TODO(tbarzic): Replace the mock with a fake implementation (crbug.com/<issue#>). https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:71: void AddDiskEntry(Disk* disk); you can use CreateDiskEntryForMountDevice/RemoveDiskEntryForMountDevice (with extra arguments that you need) instead creating new methods that essentially do the same thing.
+hashimoto for chromeos/disks.
https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... File chromeos/disks/mock_disk_mount_manager.h (right): https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:70: // Adds a fake disk entry. This takes ownership of the pointer. > "This takes ownership of the pointer" You should use scoped_ptr instead of having this kind of comment. http://www.chromium.org/developers/smart-pointer-guidelines https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:71: void AddDiskEntry(Disk* disk); On 2014/03/22 02:01:02, tbarzic wrote: > you can use CreateDiskEntryForMountDevice/RemoveDiskEntryForMountDevice (with > extra arguments that you need) instead creating new methods that essentially do > the same thing. +1 https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:74: void RemoveDiskEntry(Disk* disk); Why don't you pass device_path to this method?
So, I can't set all the options I want from the existing methods. I could simply remove that requirement but then I won't be able to test some of the negative cases. https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos.cc (right): https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/03/22 02:01:02, tbarzic wrote: > s/2013/2014 Done. https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos.cc:14: On 2014/03/22 02:01:02, tbarzic wrote: > add // static Done. https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc:30: disk_mount_manager_mock_ = new MockDiskMountManager(); On 2014/03/22 02:01:02, tbarzic wrote: > Mock is not really what you need here, but since we don't have a fake for > DiskMountManager, I guess it's OK. Acknowledged. https://codereview.chromium.org/207383004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc:106: provider->GetAllDevices( On 2014/03/22 02:01:02, tbarzic wrote: > GetAllDevices is static, right? > I'd suggest calling > RemovableStorageProvider::GetAllDevices() > (and dropping |provider| pointer) Whoops, so for some reason I forgot this was a static method and the class never needs to be instantiated. https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... File chromeos/disks/mock_disk_mount_manager.h (right): https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:18: On 2014/03/22 02:01:02, tbarzic wrote: > can you file a bug against me to replace MockDisakManager with a fake > implementation; and add a > TODO(tbarzic): Replace the mock with a fake implementation (crbug.com/<issue#>). Done. https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:70: // Adds a fake disk entry. This takes ownership of the pointer. On 2014/03/24 04:37:09, hashimoto wrote: > > "This takes ownership of the pointer" > You should use scoped_ptr instead of having this kind of comment. > http://www.chromium.org/developers/smart-pointer-guidelines Done. https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:71: void AddDiskEntry(Disk* disk); On 2014/03/24 04:37:09, hashimoto wrote: > On 2014/03/22 02:01:02, tbarzic wrote: > > you can use CreateDiskEntryForMountDevice/RemoveDiskEntryForMountDevice (with > > extra arguments that you need) instead creating new methods that essentially > do > > the same thing. > +1 The reason I wanted this was because many of the parameters that are set by CreateDiskEntryForMountDevice are fixed and I wanted to do a more precise test to be sure that we really only list the devices we expect, e.g. combinations of hidden, non-parent, and os-devices. This might be a bit too much of a characterization test and duplicating application logic in a test, but I wanted to be sure we were doing what we expected in this case. If you think it's really over-specifying things, I can remove these functions and just fall back to using one major positive-case. https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:74: void RemoveDiskEntry(Disk* disk); On 2014/03/24 04:37:09, hashimoto wrote: > Why don't you pass device_path to this method? Done.
https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... File chromeos/disks/mock_disk_mount_manager.h (right): https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... chromeos/disks/mock_disk_mount_manager.h:71: void AddDiskEntry(Disk* disk); On 2014/03/24 22:33:25, Drew Haven wrote: > On 2014/03/24 04:37:09, hashimoto wrote: > > On 2014/03/22 02:01:02, tbarzic wrote: > > > you can use CreateDiskEntryForMountDevice/RemoveDiskEntryForMountDevice > (with > > > extra arguments that you need) instead creating new methods that essentially > > do > > > the same thing. > > +1 > > The reason I wanted this was because many of the parameters that are set by > CreateDiskEntryForMountDevice are fixed and I wanted to do a more precise test > to be sure that we really only list the devices we expect, e.g. combinations of > hidden, non-parent, and os-devices. > > This might be a bit too much of a characterization test and duplicating > application logic in a test, but I wanted to be sure we were doing what we > expected in this case. > > If you think it's really over-specifying things, I can remove these functions > and just fall back to using one major positive-case. What I had in mind was to change CreateDiskEntryForMountDevice(<current_args>) to CreateDiskEntryForMountDevice(<current_args>, is_parent, has_media, on_boot_device), so it can set the values you need.
On 2014/03/24 22:48:42, tbarzic wrote: > https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... > File chromeos/disks/mock_disk_mount_manager.h (right): > > https://codereview.chromium.org/207383004/diff/1/chromeos/disks/mock_disk_mou... > chromeos/disks/mock_disk_mount_manager.h:71: void AddDiskEntry(Disk* disk); > On 2014/03/24 22:33:25, Drew Haven wrote: > > On 2014/03/24 04:37:09, hashimoto wrote: > > > On 2014/03/22 02:01:02, tbarzic wrote: > > > > you can use CreateDiskEntryForMountDevice/RemoveDiskEntryForMountDevice > > (with > > > > extra arguments that you need) instead creating new methods that > essentially > > > do > > > > the same thing. > > > +1 > > > > The reason I wanted this was because many of the parameters that are set by > > CreateDiskEntryForMountDevice are fixed and I wanted to do a more precise test > > to be sure that we really only list the devices we expect, e.g. combinations > of > > hidden, non-parent, and os-devices. > > > > This might be a bit too much of a characterization test and duplicating > > application logic in a test, but I wanted to be sure we were doing what we > > expected in this case. > > > > If you think it's really over-specifying things, I can remove these functions > > and just fall back to using one major positive-case. > > What I had in mind was to change > CreateDiskEntryForMountDevice(<current_args>) > to > CreateDiskEntryForMountDevice(<current_args>, is_parent, has_media, > on_boot_device), so it can set the values you need. Ah, gotcha. I'll give that a shot. You're right that it's only used in one place anyway.
So I updated the MockDiskMountManager changes to simplify them and changed the other call-site. It actually exposed a problem in my tests. What's a good way to check that an element is in a vector by value (not instance equality)? In this case the order I get devices out isn't the same as the way I put them in. Is this easy to do or should I just only test returning one value at a time?
On 2014/03/25 01:42:53, Drew Haven wrote: > So I updated the MockDiskMountManager changes to simplify them and changed the > other call-site. > > It actually exposed a problem in my tests. What's a good way to check that an > element is in a vector by value (not instance equality)? In this case the order > I get devices out isn't the same as the way I put them in. Is this easy to do > or should I just only test returning one value at a time? yeah, they will be sorted by device path. Can't you just add a function that iterates through list until it finds a device with the same id?
On 2014/03/25 02:08:33, tbarzic wrote: > On 2014/03/25 01:42:53, Drew Haven wrote: > > So I updated the MockDiskMountManager changes to simplify them and changed the > > other call-site. > > > > It actually exposed a problem in my tests. What's a good way to check that an > > element is in a vector by value (not instance equality)? In this case the > order > > I get devices out isn't the same as the way I put them in. Is this easy to do > > or should I just only test returning one value at a time? > > yeah, they will be sorted by device path. > > Can't you just add a function that iterates through list until it finds a device > with the same id? I was hoping for something slicker, like an inline comparator or something. I've been writing a lot of JavaScript and Python lately. I just went with your suggestion.
https://codereview.chromium.org/207383004/diff/50001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/207383004/diff/50001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc:80: bool DeviceListHasEntry(StorageDeviceList* list, storage_unit_id should be unique, so it should be ok to search for the device only by the storage id, and match the rest of the properties separately. Something like: Device* device = FindDevice(list, id); ASSERT_TRUE(device); EXPECT_EQ(expected_capacity, device->capacity); EXPECT_EQ(expected_vendor, device->vendor_name); ... (you could also extract EXPECT_EQ block to a separate function ExpectDeviceEquals) The test result should be the same, but you'd get a better failure message.
lgtm, but please consider the approach I suggested for unit tests. https://codereview.chromium.org/207383004/diff/50001/components/storage_monit... File components/storage_monitor/storage_monitor_chromeos_unittest.cc (right): https://codereview.chromium.org/207383004/diff/50001/components/storage_monit... components/storage_monitor/storage_monitor_chromeos_unittest.cc:206: false, nit: add comments here: false /* is_parent */, true /* has_media */, false /* on_boot_device */); Or // styled ones if you prefer them.
I did the refactoring as you suggested. Thanks for the review. https://codereview.chromium.org/207383004/diff/50001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc (right): https://codereview.chromium.org/207383004/diff/50001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_chromeos_unittest.cc:80: bool DeviceListHasEntry(StorageDeviceList* list, Done. It looks reasonably clean to factor out the EXPECT block, but then the message doesn't point to the failing test, but it prevents repeating myself over and over. Is there a way to print more verbose stack traces on test failures, or does the macro just expand the current file/line and we'll never get them? :( On 2014/03/26 00:33:56, tbarzic wrote: > storage_unit_id should be unique, so it should be ok to search for the device > only by the storage id, > and match the rest of the properties separately. > Something like: > > Device* device = FindDevice(list, id); > ASSERT_TRUE(device); > > EXPECT_EQ(expected_capacity, device->capacity); > EXPECT_EQ(expected_vendor, device->vendor_name); > ... > > (you could also extract EXPECT_EQ block to a separate function > ExpectDeviceEquals) > > The test result should be the same, but you'd get a better failure message. https://codereview.chromium.org/207383004/diff/50001/components/storage_monit... File components/storage_monitor/storage_monitor_chromeos_unittest.cc (right): https://codereview.chromium.org/207383004/diff/50001/components/storage_monit... components/storage_monitor/storage_monitor_chromeos_unittest.cc:206: false, On 2014/03/26 00:40:09, tbarzic wrote: > nit: > add comments here: > false /* is_parent */, > true /* has_media */, > false /* on_boot_device */); > > Or // styled ones if you prefer them. Done.
Steve, Could I get you to take a look at this to approve the StorageMonitor test change? Thanks, Drew
chromeos/disks lgtm https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... File chromeos/disks/mock_disk_mount_manager.cc (right): https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:188: Disk* disk = new DiskMountManager::Disk(std::string(mount_info.source_path), nit: std::string(...) is unnecessary. No need to convert std::string to std::string manually. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:189: std::string(mount_info.mount_path), ditto. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:191: std::string(mount_info.source_path), ditto. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:192: device_label, // device_label nit: > "// device label" This kind of comment may be meaningful for the fields filled with std::string(), but not for fields like this. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:205: on_boot_device, // on_boot_device ditto for these four fields.
StorageMonitor test LGTM
https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... File chromeos/disks/mock_disk_mount_manager.cc (right): https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:188: Disk* disk = new DiskMountManager::Disk(std::string(mount_info.source_path), On 2014/03/27 02:22:40, hashimoto wrote: > nit: std::string(...) is unnecessary. > No need to convert std::string to std::string manually. That was there before, so I just left it. Might as well clean it up and improve the code base! https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:189: std::string(mount_info.mount_path), On 2014/03/27 02:22:40, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:191: std::string(mount_info.source_path), On 2014/03/27 02:22:40, hashimoto wrote: > ditto. Done. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:192: device_label, // device_label On 2014/03/27 02:22:40, hashimoto wrote: > nit: > "// device label" > This kind of comment may be meaningful for the fields filled with std::string(), > but not for fields like this. Done. https://codereview.chromium.org/207383004/diff/120001/chromeos/disks/mock_dis... chromeos/disks/mock_disk_mount_manager.cc:205: on_boot_device, // on_boot_device On 2014/03/27 02:22:40, hashimoto wrote: > ditto for these four fields. Done.
The CQ bit was checked by haven@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haven@chromium.org/207383004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/03/27 20:02:35, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Oh, it looks like there is no owners file for chromeos/disks so it rolled up to chromeos. I'll see if I can find someone for those files. +stevenjb
On 2014/03/27 21:06:38, Drew Haven wrote: > On 2014/03/27 20:02:35, I haz the power (commit-bot) wrote: > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > Oh, it looks like there is no owners file for chromeos/disks so it rolled up to > chromeos. I'll see if I can find someone for those files. > > +stevenjb stephenjb, Could you take a quick look and potentially LGTM the chromeos/disks changes on this CL?
chromeos/disks lgtm
The CQ bit was checked by haven@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haven@chromium.org/207383004/140001
The CQ bit was unchecked by haven@chromium.org
The CQ bit was checked by haven@chromium.org
The CQ bit was unchecked by cmp@chromium.org
The CQ bit was checked by cmp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haven@chromium.org/207383004/140001
Message was sent while issue was closed.
Change committed as 260455 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
