Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
291333006:
Adds removable-drive listing for OS X. (Closed)
|
Created:
6 years, 7 months ago by Drew Haven Modified:
6 years, 6 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, stephenlin1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds removable-drive listing for OS X.
Listing disks on OS X is very similar to components/storage_monitor/storage_monitor.h except that we want to list all whole-drives and not just mountable volumes.
The code here basically follows that code, but with minor changes. Since the Disk Arbitration code is so stateful and involves so many callbacks I created an internal class to handle the state without changing the interface.
BUG=376382
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276927
Patch Set 1 #Patch Set 2 : Refactors (again) and adds a few unit tests. #Patch Set 3 : Removes unnecessary changes. #
Total comments: 20
Patch Set 4 : Cleans up naming, files and tests. #Patch Set 5 : A few small wording cleanups. #Patch Set 6 : Minor rename and fixes ordering of deletion. #Patch Set 7 : Fixes missed rename in the test. #
Total comments: 18
Patch Set 8 : More cleanup, mostly tests. #
Total comments: 2
Patch Set 9 : Rewrite to use IOKit, greatly simplifying this CL. #
Total comments: 18
Patch Set 10 : Removes references to DiskArbitration from RemovableStorageProvider for mac. #Patch Set 11 : A little more cleanup. #
Total comments: 4
Patch Set 12 : Changes some failure conditions to continue instead. #
Total comments: 5
Patch Set 13 : Adds AssertIOAllowed #Messages
Total messages: 21 (0 generated)
Robert and Rachel, Here's the version of the device-listing code that I have for Mac. I was hoping you two could review it. What I'm really curious about is whether my DAWrapper abstraction is reasonable. The RemovableStorageProvider has all static methods for historic reasons, and this wrapper seemed like the easiest way to encapsulate and isolate the DA state from the rest of the code so that it would not leak out of the RemovableStorageProvider. Thanks for taking a look.
Some high-level comments to get started. This is not what we discussed over email. This is registering the callback on the browser's UI thread, not in the utility process. What prompted this change? https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h:56: class DAWrapper : public base::SupportsWeakPtr<DAWrapper> { A better name for this would be RemovableStorageProviderMac. Also, is there any reason why it should be done within this common header, instead of a _mac.h header? https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h:100: class DAWrapperSpouse { This does not belong in a public .h file. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:17: DAWrapper* da_wrapper = new DAWrapper(); Comment about ownership. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:47: session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes); Really? I thought you said this was going to execute in the utility process. You're scheduling this on the browser's UI thread. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: content::BrowserThread::PostTask( Why are you using message passing here? It's all being done on the UI thread anyways… https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:106: CFBooleanRef internal = (CFBooleanRef)CFDictionaryGetValue( C-style casts are banned. Use GetValueFromDictionary like you do above. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:131: bool isUSBStick = naming: is_usb_stick https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:148: bool isSDCard = naming: is_sd_card
Thanks for taking the time to review this. This isn't quite what we were talking about because the code has been split into two parts: listing the drives in the browser process and doing the actual writing on the utility process. For my purposes the writing depends on the listing so I wanted to do the listing first. Technically the only coupling is through disk identification strings, e.g. "disk3". I spent Friday and today cleaning up this CL and adding tests. Tomorrow I'll work on the other one which also uses a thread with an explicit mac message pump. I think I've already learned a few things about the threading from cleaning this one up. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h:56: class DAWrapper : public base::SupportsWeakPtr<DAWrapper> { On 2014/06/02 19:56:05, rsesek wrote: > A better name for this would be RemovableStorageProviderMac. Also, is there any > reason why it should be done within this common header, instead of a _mac.h > header? I think I originally put this here just to make it accessible from my tests, but you're right that I should add a new header. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h:100: class DAWrapperSpouse { On 2014/06/02 19:56:05, rsesek wrote: > This does not belong in a public .h file. Moved to test_utils.h https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:17: DAWrapper* da_wrapper = new DAWrapper(); On 2014/06/02 19:56:05, rsesek wrote: > Comment about ownership. I wasn't sure how to address this exactly. We get to a point where we're waiting on DiskArbitration callbacks but there are no callbacks on the message queue that have references to this object. The lifetime of this object should roughly be that of the callback. I don't think I can use callback ownership because this class has to keep a copy of the callback to call later and so would create a circular reference. I opted for this pattern of making it a bare pointer and then deleting it when the callback gets called. To be sure it gets deleted I added a timer that times out and calls the callback with a failure after a small interval. I also added unit-tests to be sure that this behavior was occurring by checking the value of a weak pointer to this object after the callback has run. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:47: session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes); On 2014/06/02 19:56:05, rsesek wrote: > Really? I thought you said this was going to execute in the utility process. > You're scheduling this on the browser's UI thread. Discussed out of band. See general comment. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: content::BrowserThread::PostTask( On 2014/06/02 19:56:05, rsesek wrote: > Why are you using message passing here? It's all being done on the UI thread > anyways… The reason is because we get a bunch of DiskAppearedCallbacks in the queue and we want to know when we've processed all of them, so we basically just go through and count them up before processing, then as we process disks we decrement the counter and once we reach zero we know we've processed all the callbacks. I tried to explain that in the method comments in the header. Let me know if that makes sense, otherwise I can reword it. I learned the trick from StorageMonitorMac. One case that is not covered yet is if there are no callbacks (no disks somehow...) but I'll try to take care of that with a timeout. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:106: CFBooleanRef internal = (CFBooleanRef)CFDictionaryGetValue( On 2014/06/02 19:56:05, rsesek wrote: > C-style casts are banned. Use GetValueFromDictionary like you do above. Done. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:131: bool isUSBStick = On 2014/06/02 19:56:05, rsesek wrote: > naming: is_usb_stick Done. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:148: bool isSDCard = On 2014/06/02 19:56:05, rsesek wrote: > naming: is_sd_card Done.
rsesek definitely knows this better than I do - taking myself off reviewer list, unless you think you need me as a second reviewer.
https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: content::BrowserThread::PostTask( On 2014/06/03 02:03:23, Drew Haven wrote: > On 2014/06/02 19:56:05, rsesek wrote: > > Why are you using message passing here? It's all being done on the UI thread > > anyways… > > The reason is because we get a bunch of DiskAppearedCallbacks in the queue and > we want to know when we've processed all of them, so we basically just go > through and count them up before processing, then as we process disks we > decrement the counter and once we reach zero we know we've processed all the > callbacks. I tried to explain that in the method comments in the header. Let > me know if that makes sense, otherwise I can reword it. I learned the trick > from StorageMonitorMac. > > One case that is not covered yet is if there are no callbacks (no disks > somehow...) but I'll try to take care of that with a timeout. But does that actually work as you describe? There's no guarantee that the AddDisk() callback will run after a second DeviceAppearedCallback(), meaning you could be incrementing to 1 updates_pending_, then decrementing it right back down to 0. Am I missing something? https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:11: #include "chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h" This header comes first, followed by a blank line, followed by the system includes. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:18: void RemovableStorageProvider::GetAllDevicesImpl( // static https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:117: void* context) { Why register for this callback if you're not doing work in it? https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h (right): https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:3: // found in the LICENSE file. nit: blankl ine after https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:35: static void ProviderDeleter(RemovableStorageProviderMac* wrapper, Does this need to be public? https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:48: void ProcessDisk(base::ScopedCFTypeRef<CFDictionaryRef> dict); nit: blank lines between each of these functions. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc (right): https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc:30: RemovableStorageProviderTestRunner() : thread_("Test thread") {} Name the test here, so that if the thread is leaked, it's easy to track down. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc:59: base::Bind(RemovableStorageProviderMac::ProviderDeleter, This seems a bit weird to me. Wouldn't it be better to test the public interface that clients use (i.e. the static functions) and ensure that those behave correctly? https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc:147: CFMutableDictionaryRef dict = Put this in a scoper to start with.
https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: content::BrowserThread::PostTask( On 2014/06/04 19:10:43, rsesek wrote: > On 2014/06/03 02:03:23, Drew Haven wrote: > > On 2014/06/02 19:56:05, rsesek wrote: > > > Why are you using message passing here? It's all being done on the UI thread > > > anyways… > > > > The reason is because we get a bunch of DiskAppearedCallbacks in the queue and > > we want to know when we've processed all of them, so we basically just go > > through and count them up before processing, then as we process disks we > > decrement the counter and once we reach zero we know we've processed all the > > callbacks. I tried to explain that in the method comments in the header. Let > > me know if that makes sense, otherwise I can reword it. I learned the trick > > from StorageMonitorMac. > > > > One case that is not covered yet is if there are no callbacks (no disks > > somehow...) but I'll try to take care of that with a timeout. > > But does that actually work as you describe? There's no guarantee that the > AddDisk() callback will run after a second DeviceAppearedCallback(), meaning you > could be incrementing to 1 updates_pending_, then decrementing it right back > down to 0. Am I missing something? As far as I can tell this is entirely undocumented and I suppose we're just hoping it works that way. I've been thinking about it and the only way around it I can think of with DiskArb is to instantiate a global service, much like StorageMonitor that will keep a watchful and on-going eye on the disks. I don't know of a way to immediately list all current disks. The existing tool starts up it's monitor when the application starts up. So, I guess we have to decide if the empirical evidence is strong enough (risky), I'll have to integrate more deeply into something like StorageMonitor (integration headaches) or create my own service for this (complexity and code duplication). Preference? https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:11: #include "chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h" On 2014/06/04 19:10:43, rsesek wrote: > This header comes first, followed by a blank line, followed by the system > includes. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Oh, I had interpreted this as optional since the linter seemed more worried about sorting than placing these headers first. I'll have to fix this in a few files. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:18: void RemovableStorageProvider::GetAllDevicesImpl( On 2014/06/04 19:10:43, rsesek wrote: > // static Done. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:117: void* context) { On 2014/06/04 19:10:43, rsesek wrote: > Why register for this callback if you're not doing work in it? Good point. It's a hold-over from when I wasn't sure if I'd need it. Since I was just going to do an immediate listing it is unnecessary, but if we switch to a persistent list I'll have I'll use it again. Removing. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h (right): https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:3: // found in the LICENSE file. On 2014/06/04 19:10:43, rsesek wrote: > nit: blankl ine after Done. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:35: static void ProviderDeleter(RemovableStorageProviderMac* wrapper, On 2014/06/04 19:10:43, rsesek wrote: > Does this need to be public? I think it can be made private now. The main issue was testing and not wanting to force it to always be wrapped. But with the way the tests are now it should be fine. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:48: void ProcessDisk(base::ScopedCFTypeRef<CFDictionaryRef> dict); On 2014/06/04 19:10:43, rsesek wrote: > nit: blank lines between each of these functions. Done. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc (right): https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc:30: RemovableStorageProviderTestRunner() : thread_("Test thread") {} On 2014/06/04 19:10:43, rsesek wrote: > Name the test here, so that if the thread is leaked, it's easy to track down. Done. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc:59: base::Bind(RemovableStorageProviderMac::ProviderDeleter, On 2014/06/04 19:10:43, rsesek wrote: > This seems a bit weird to me. Wouldn't it be better to test the public interface > that clients use (i.e. the static functions) and ensure that those behave > correctly? Two reasons: 1. I can keep a DCHECK for the UI thread in the function because it really must be run on the UI thread but then in my test I can still run this here. 2. I can keep a weak pointer to the storage_provider_ to check that it was deleted. This function mimics the code in the actual API, which is essentially the same in that it just creates an object and calls this function. Perhaps with a better interface I could test this better. I'll put some thought into that. https://codereview.chromium.org/291333006/diff/140001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac_unittest.cc:147: CFMutableDictionaryRef dict = On 2014/06/04 19:10:43, rsesek wrote: > Put this in a scoper to start with. Done.
Ping. Also, see new comment. https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: content::BrowserThread::PostTask( On 2014/06/05 01:52:50, Drew Haven wrote: > On 2014/06/04 19:10:43, rsesek wrote: > > On 2014/06/03 02:03:23, Drew Haven wrote: > > > On 2014/06/02 19:56:05, rsesek wrote: > > > > Why are you using message passing here? It's all being done on the UI > thread > > > > anyways… > > > > > > The reason is because we get a bunch of DiskAppearedCallbacks in the queue > and > > > we want to know when we've processed all of them, so we basically just go > > > through and count them up before processing, then as we process disks we > > > decrement the counter and once we reach zero we know we've processed all the > > > callbacks. I tried to explain that in the method comments in the header. > Let > > > me know if that makes sense, otherwise I can reword it. I learned the trick > > > from StorageMonitorMac. > > > > > > One case that is not covered yet is if there are no callbacks (no disks > > > somehow...) but I'll try to take care of that with a timeout. > > > > But does that actually work as you describe? There's no guarantee that the > > AddDisk() callback will run after a second DeviceAppearedCallback(), meaning > you > > could be incrementing to 1 updates_pending_, then decrementing it right back > > down to 0. Am I missing something? > > As far as I can tell this is entirely undocumented and I suppose we're just > hoping it works that way. I've been thinking about it and the only way around > it I can think of with DiskArb is to instantiate a global service, much like > StorageMonitor that will keep a watchful and on-going eye on the disks. I don't > know of a way to immediately list all current disks. > > The existing tool starts up it's monitor when the application starts up. > > So, I guess we have to decide if the empirical evidence is strong enough > (risky), I'll have to integrate more deeply into something like StorageMonitor > (integration headaches) or create my own service for this (complexity and code > duplication). > > Preference? I talked with Greg Billock who did the similar work in the Storage Monitor. It looks like it just appears to be what the API does but it isn't really documented anywhere. I think in the long run we want to merge this into the storage monitor and there are open bugs to look at that, but at the moment this appears to work well. My guess is that when the session starts up on the loop it immediately posts an event for each of the known disks that are already known by the system and so they are all immediately on the queue. I can't prove this though, since we can't look at the source. Is it possible we can leave this as is and look at further integration at another time?
https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: content::BrowserThread::PostTask( On 2014/06/09 23:13:02, Drew Haven wrote: > On 2014/06/05 01:52:50, Drew Haven wrote: > > On 2014/06/04 19:10:43, rsesek wrote: > > > On 2014/06/03 02:03:23, Drew Haven wrote: > > > > On 2014/06/02 19:56:05, rsesek wrote: > > > > > Why are you using message passing here? It's all being done on the UI > > thread > > > > > anyways… > > > > > > > > The reason is because we get a bunch of DiskAppearedCallbacks in the queue > > and > > > > we want to know when we've processed all of them, so we basically just go > > > > through and count them up before processing, then as we process disks we > > > > decrement the counter and once we reach zero we know we've processed all > the > > > > callbacks. I tried to explain that in the method comments in the header. > > Let > > > > me know if that makes sense, otherwise I can reword it. I learned the > trick > > > > from StorageMonitorMac. > > > > > > > > One case that is not covered yet is if there are no callbacks (no disks > > > > somehow...) but I'll try to take care of that with a timeout. > > > > > > But does that actually work as you describe? There's no guarantee that the > > > AddDisk() callback will run after a second DeviceAppearedCallback(), meaning > > you > > > could be incrementing to 1 updates_pending_, then decrementing it right back > > > down to 0. Am I missing something? > > > > As far as I can tell this is entirely undocumented and I suppose we're just > > hoping it works that way. I've been thinking about it and the only way around > > it I can think of with DiskArb is to instantiate a global service, much like > > StorageMonitor that will keep a watchful and on-going eye on the disks. I > don't > > know of a way to immediately list all current disks. > > > > The existing tool starts up it's monitor when the application starts up. > > > > So, I guess we have to decide if the empirical evidence is strong enough > > (risky), I'll have to integrate more deeply into something like StorageMonitor > > (integration headaches) or create my own service for this (complexity and code > > duplication). > > > > Preference? > > I talked with Greg Billock who did the similar work in the Storage Monitor. It > looks like it just appears to be what the API does but it isn't really > documented anywhere. > > I think in the long run we want to merge this into the storage monitor and there > are open bugs to look at that, but at the moment this appears to work well. My > guess is that when the session starts up on the loop it immediately posts an > event for each of the known disks that are already known by the system and so > they are all immediately on the queue. I can't prove this though, since we > can't look at the source. Then what is the rationale for not using storage monitor directly, if it's the ultimate goal? > Is it possible we can leave this as is and look at further integration at > another time? I don't think relying on undocumented behavior is the right way to go, unless there's really nothing better. I was curious how `diskutil list` works, and it actually does not use DiskArbitration to do this, but instead just uses IOKit. (The following is from reverse engineering _DMgetDisks in the DiskManagement.framework PrivateFramework, and it seems to work): NSMutableArray* array = [NSMutableArray array]; CFMutableDictionaryRef matching = IOServiceMatching("IOMedia"); CFDictionaryAddValue(matching, CFSTR("Whole"), kCFBooleanTrue); io_service_t iterator; IOServiceGetMatchingServices(kIOMasterPortDefault, matching, &iterator); io_object_t obj; while ((obj = IOIteratorNext(iterator))) { DADiskRef disk = DADiskCreateFromIOMedia(NULL, session, obj); [array addObject:disk]; IOObjectRelease(obj); CFRelease(disk); } IOObjectRelease(iterator); Unless you need continued monitoring of active disks, I think this would work here; and then you won't need to worry about timeouts, updates_pending_, or adding blocking work to the UI thread. This could just be done on the file-blocking pool. https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:111: bool RemovableStorageProviderMac::DiskIsValidTarget(CFDictionaryRef dict) { This shouldn't be duplicated with the code in your other CL. Split the CLs if necessary to make that happen. https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h (right): https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:62: int updates_pending_; Member variables should also be commented.
On 2014/06/10 20:52:23, rsesek wrote: > https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... > File > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc > (right): > > https://codereview.chromium.org/291333006/diff/40001/chrome/browser/extension... > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:52: > content::BrowserThread::PostTask( > On 2014/06/09 23:13:02, Drew Haven wrote: > > On 2014/06/05 01:52:50, Drew Haven wrote: > > > On 2014/06/04 19:10:43, rsesek wrote: > > > > On 2014/06/03 02:03:23, Drew Haven wrote: > > > > > On 2014/06/02 19:56:05, rsesek wrote: > > > > > > Why are you using message passing here? It's all being done on the UI > > > thread > > > > > > anyways… > > > > > > > > > > The reason is because we get a bunch of DiskAppearedCallbacks in the > queue > > > and > > > > > we want to know when we've processed all of them, so we basically just > go > > > > > through and count them up before processing, then as we process disks we > > > > > decrement the counter and once we reach zero we know we've processed all > > the > > > > > callbacks. I tried to explain that in the method comments in the > header. > > > Let > > > > > me know if that makes sense, otherwise I can reword it. I learned the > > trick > > > > > from StorageMonitorMac. > > > > > > > > > > One case that is not covered yet is if there are no callbacks (no disks > > > > > somehow...) but I'll try to take care of that with a timeout. > > > > > > > > But does that actually work as you describe? There's no guarantee that the > > > > AddDisk() callback will run after a second DeviceAppearedCallback(), > meaning > > > you > > > > could be incrementing to 1 updates_pending_, then decrementing it right > back > > > > down to 0. Am I missing something? > > > > > > As far as I can tell this is entirely undocumented and I suppose we're just > > > hoping it works that way. I've been thinking about it and the only way > around > > > it I can think of with DiskArb is to instantiate a global service, much like > > > StorageMonitor that will keep a watchful and on-going eye on the disks. I > > don't > > > know of a way to immediately list all current disks. > > > > > > The existing tool starts up it's monitor when the application starts up. > > > > > > So, I guess we have to decide if the empirical evidence is strong enough > > > (risky), I'll have to integrate more deeply into something like > StorageMonitor > > > (integration headaches) or create my own service for this (complexity and > code > > > duplication). > > > > > > Preference? > > > > I talked with Greg Billock who did the similar work in the Storage Monitor. > It > > looks like it just appears to be what the API does but it isn't really > > documented anywhere. > > > > I think in the long run we want to merge this into the storage monitor and > there > > are open bugs to look at that, but at the moment this appears to work well. > My > > guess is that when the session starts up on the loop it immediately posts an > > event for each of the known disks that are already known by the system and so > > they are all immediately on the queue. I can't prove this though, since we > > can't look at the source. > > Then what is the rationale for not using storage monitor directly, if it's the > ultimate goal? > > > Is it possible we can leave this as is and look at further integration at > > another time? > > I don't think relying on undocumented behavior is the right way to go, unless > there's really nothing better. I was curious how `diskutil list` works, and it > actually does not use DiskArbitration to do this, but instead just uses IOKit. > > (The following is from reverse engineering _DMgetDisks in the > DiskManagement.framework PrivateFramework, and it seems to work): > > NSMutableArray* array = [NSMutableArray array]; > CFMutableDictionaryRef matching = IOServiceMatching("IOMedia"); > CFDictionaryAddValue(matching, CFSTR("Whole"), kCFBooleanTrue); > io_service_t iterator; > IOServiceGetMatchingServices(kIOMasterPortDefault, matching, &iterator); > io_object_t obj; > while ((obj = IOIteratorNext(iterator))) { > DADiskRef disk = DADiskCreateFromIOMedia(NULL, session, obj); > [array addObject:disk]; > IOObjectRelease(obj); > CFRelease(disk); > } > IOObjectRelease(iterator); > > > Unless you need continued monitoring of active disks, I think this would work > here; and then you won't need to worry about timeouts, updates_pending_, or > adding blocking work to the UI thread. This could just be done on the > file-blocking pool. > > https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... > File > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc > (right): > > https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:111: > bool RemovableStorageProviderMac::DiskIsValidTarget(CFDictionaryRef dict) { > This shouldn't be duplicated with the code in your other CL. Split the CLs if > necessary to make that happen. > > https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... > File > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h > (right): > > https://codereview.chromium.org/291333006/diff/180001/chrome/browser/extensio... > chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.h:62: > int updates_pending_; > Member variables should also be commented. Well, damn. That's what I get for looking only at the old tool and a few Google searches and just assuming that's the way it's done. A simple synchronous API is exactly what I needed. This CL is trivial now.
This is _much_ simpler now, but it can be even more so. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:49: CFStringCompare(kind, CFSTR("IOMedia"), 0) == kCFCompareEqualTo; You already know this is of type IOMedia. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:78: CFMutableDictionaryRef matching = IOServiceMatching("IOMedia"); Use kIOMediaClass. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:79: CFDictionaryAddValue(matching, CFSTR("Whole"), kCFBooleanTrue); Use kIOMediaWholeKey. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:79: CFDictionaryAddValue(matching, CFSTR("Whole"), kCFBooleanTrue); You can also add kIOMediaRemovableKey and kIOMediaEjectableKey to the matching dictionary, and then you don't need to check those in DiskIsValidTarget(). C.f. https://developer.apple.com/library/mac/documentation/Kernel/Reference/IOMedi... https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:89: DADiskCreateFromIOMedia(NULL, session, disk_obj)); You shouldn't even need to create a DADiskRef. DiskArbitration is merely copying data from IOKit into the DA type. Take a look at IORegistryEntrySearchCFProperty() with the keys I comment with below. FYI, the code for DADiskCreateFromIOMedia is open source: http://opensource.apple.com/source/DiskArbitration/DiskArbitration-129/diskar... https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:93: CFStringRef cf_bsd_name = base::mac::GetValueFromDictionary<CFStringRef>( kIOBSDNameKey https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:97: CFStringRef cf_vendor = base::mac::GetValueFromDictionary<CFStringRef>( kIOPropertyVendorNameKey https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:102: dict, kDADiskDescriptionDeviceModelKey); kIOPropertyProductNameKey https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:106: dict, kDADiskDescriptionMediaSizeKey); kIOMediaSizeKey
So, I altered the behavior a little bit in this change. I stopped checking for the USB protocol and that string for the Apple USB Card reader, namely due to this bug: http://crbug.com/318451 I'm inclined to go out with a slightly broader list of devices, but still only whole disks that are removable, ejectable and writable. Then when we go into testing/dogfood we can see if there are any issues to address before we go stable. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:49: CFStringCompare(kind, CFSTR("IOMedia"), 0) == kCFCompareEqualTo; On 2014/06/11 16:54:57, rsesek wrote: > You already know this is of type IOMedia. Done. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:78: CFMutableDictionaryRef matching = IOServiceMatching("IOMedia"); On 2014/06/11 16:54:57, rsesek wrote: > Use kIOMediaClass. Done. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:79: CFDictionaryAddValue(matching, CFSTR("Whole"), kCFBooleanTrue); On 2014/06/11 16:54:57, rsesek wrote: > Use kIOMediaWholeKey. Aaah, I was looking at the disk-arb matching dictionaries, but I found those didn't work and got discouraged. I'll do this. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:89: DADiskCreateFromIOMedia(NULL, session, disk_obj)); On 2014/06/11 16:54:57, rsesek wrote: > You shouldn't even need to create a DADiskRef. DiskArbitration is merely copying > data from IOKit into the DA type. Take a look at > IORegistryEntrySearchCFProperty() with the keys I comment with below. > > FYI, the code for DADiskCreateFromIOMedia is open source: > http://opensource.apple.com/source/DiskArbitration/DiskArbitration-129/diskar... This worked well. Interestingly I don't seem to be able to search for the vendor/model directly, but have to search for the characteristics dictionary instead, but that's a minor annoyance. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:93: CFStringRef cf_bsd_name = base::mac::GetValueFromDictionary<CFStringRef>( On 2014/06/11 16:54:57, rsesek wrote: > kIOBSDNameKey Done. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:97: CFStringRef cf_vendor = base::mac::GetValueFromDictionary<CFStringRef>( On 2014/06/11 16:54:57, rsesek wrote: > kIOPropertyVendorNameKey Done. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:102: dict, kDADiskDescriptionDeviceModelKey); On 2014/06/11 16:54:58, rsesek wrote: > kIOPropertyProductNameKey Done. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:106: dict, kDADiskDescriptionMediaSizeKey); On 2014/06/11 16:54:57, rsesek wrote: > kIOMediaSizeKey Done.
Very nice! Last few comments. https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/200001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:89: DADiskCreateFromIOMedia(NULL, session, disk_obj)); On 2014/06/11 21:48:13, Drew Haven wrote: > On 2014/06/11 16:54:57, rsesek wrote: > > You shouldn't even need to create a DADiskRef. DiskArbitration is merely > copying > > data from IOKit into the DA type. Take a look at > > IORegistryEntrySearchCFProperty() with the keys I comment with below. > > > > FYI, the code for DADiskCreateFromIOMedia is open source: > > > http://opensource.apple.com/source/DiskArbitration/DiskArbitration-129/diskar... > > This worked well. Interestingly I don't seem to be able to search for the > vendor/model directly, but have to search for the characteristics dictionary > instead, but that's a minor annoyance. I believe that's because vendor/model are in a separate "plane" of the IORegistry. https://codereview.chromium.org/291333006/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:70: LOG(ERROR) << "Unable to find device characteristics."; Include the BSD name in this message, to make debugging easier. https://codereview.chromium.org/291333006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:71: return false; Do you want to continue instead, skipping this one device instead of aborting the operation?
https://codereview.chromium.org/291333006/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:70: LOG(ERROR) << "Unable to find device characteristics."; On 2014/06/12 13:56:07, rsesek wrote: > Include the BSD name in this message, to make debugging easier. Done. https://codereview.chromium.org/291333006/diff/240001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:71: return false; On 2014/06/12 13:56:07, rsesek wrote: > Do you want to continue instead, skipping this one device instead of aborting > the operation? Good idea. I'll do the same above. The only hard failure will be an error listing devices at all.
Argh, missed this last item on the last round. https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:25: CFMutableDictionaryRef matching = IOServiceMatching(kIOMediaClass); This is leaked.
https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:25: CFMutableDictionaryRef matching = IOServiceMatching(kIOMediaClass); On 2014/06/12 20:53:39, rsesek wrote: > This is leaked. Oddly IOServiceGetMatchingServices consumes a reference to the dictionary. From the docs: "A CF dictionary containing matching information, of which one reference is always consumed by this function."
LGTM https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:25: CFMutableDictionaryRef matching = IOServiceMatching(kIOMediaClass); On 2014/06/12 21:10:30, Drew Haven wrote: > On 2014/06/12 20:53:39, rsesek wrote: > > This is leaked. > > Oddly IOServiceGetMatchingServices consumes a reference to the dictionary. From > the docs: "A CF dictionary containing matching information, of which one > reference is always consumed by this function." That's probably why I didn't mention it the first time, and then I forgot :).
https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:22: bool RemovableStorageProvider::PopulateDeviceList( Also, you should add a base::ThreadRestrictions::AssertIOAllowed() here. This may not look like it's doing IO, but this is actually waiting on a Mach IPC connection to a kernel thread, so this can block.
https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... File chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc (right): https://codereview.chromium.org/291333006/diff/260001/chrome/browser/extensio... chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc:22: bool RemovableStorageProvider::PopulateDeviceList( On 2014/06/12 21:46:12, rsesek wrote: > Also, you should add a base::ThreadRestrictions::AssertIOAllowed() here. > > This may not look like it's doing IO, but this is actually waiting on a Mach IPC > connection to a kernel thread, so this can block. A reasonable request. Added.
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/291333006/280001
Message was sent while issue was closed.
Change committed as 276927 |
