Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10876101:
Initial implementation of experimental SystemInfo.Storage API for Mac OS (Closed)
|
Created:
8 years, 3 months ago by joshuagl Modified:
8 years, 3 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, Mark Mentovai Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionInitial implementation of experimental SystemInfo.Storage API for Mac OS
This matches the current Windows implementation.
BUG=141229, 145101
TEST=browser_tests --gtest_filter=SystemInfoStorageApiTest.* and manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155777
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 18
Patch Set 5 : #
Total comments: 9
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Total comments: 9
Patch Set 13 : #Patch Set 14 : #
Messages
Total messages: 42 (0 generated)
I thought it best to get some feedback ASAP on this CL as I'm new to the project. I'm not 100% happy with the code as is but it does the job and I'm hoping folks more familiar with Chromium, and (separately) Mac OS API's will help refine it through review.
Joshua, my comment inlined, and add mihai for reviewing. http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:17: #include <CoreFoundation/CoreFoundation.h> nit: alphabetic order. http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:90: std::string StorageInfoProviderMac::StorageType(std::string vol) { Nit: use const std::string& instead? http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:106: while (next_media) { Whey need to query the type info every time when querying a storage unit info? Seems that we could query the type info and construct a mapping <dev_file_path, type> firstly and then find out which type is for the vol. It can avoid the unnecessary operation to traverse the IO iterator.
Mihai, Could you please review this patch? Thanks.
Thanks Hongbo, I'll work through your suggestions today as well as fixing up the errors from the linter. Aside: I looked in src/tools for a lint-like tool before submitting but I can't seem to find anything - perchance do you know where I can find a tool to give me the lint results before I submit a patch? http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:17: #include <CoreFoundation/CoreFoundation.h> On 2012/08/28 07:42:26, Hongbo Min wrote: > nit: alphabetic order. I just saw this, amongst other things, in the linter too. Will fix. http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:90: std::string StorageInfoProviderMac::StorageType(std::string vol) { On 2012/08/28 07:42:26, Hongbo Min wrote: > Nit: use const std::string& instead? Will do. http://codereview.chromium.org/10876101/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:106: while (next_media) { On 2012/08/28 07:42:26, Hongbo Min wrote: > Whey need to query the type info every time when querying a storage unit info? > > Seems that we could query the type info and construct a mapping <dev_file_path, > type> firstly and then find out which type is for the vol. It can avoid the > unnecessary operation to traverse the IO iterator. Yes, something like that does make more sense. Thanks.
Took me a couple of goes to appease the linter but this latest change passes the lint tests and should address Hongbo's review comments. Please review, thanks!
Mark may also have an opinion on the IOKit stuff. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:56: if (mount_point == "/Volumes/Recovery HD") A user can have more than one Recovery HD if they have multiple OS X installs on the same machine. Is there a better way to test this? What flag is set on the device to exclude it from these UIs? http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:96: io_iterator_t media_iterator; base/mac/scoped_ioobject.h http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:112: CFTypeRef dev_path_cf; base/mac/scoped_cftyperef.h http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:124: CFStringGetCString((CFStringRef)dev_path_cf, There's string conversion utilities to do this for you in base/sys_string_conversions.h. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:129: CFTypeRef removable_cf; ScopedCFTypeRef http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:131: IORegistryEntryCreateCFProperty(next_media, nit: continuations are indented by 4
Thanks for the very helpful review. I'll address your comments and get an updated patch set out shortly. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:56: if (mount_point == "/Volumes/Recovery HD") On 2012/08/28 18:13:14, rsesek wrote: > A user can have more than one Recovery HD if they have multiple OS X installs on > the same machine. > > Is there a better way to test this? What flag is set on the device to exclude it > from these UIs? Thanks for the pointer! From a quick bit of test code here it looks like I can avoid both this test and the above test of f_fstypename by checking whether the MNT_DONTBROWSE flag is set on the file system. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:96: io_iterator_t media_iterator; On 2012/08/28 18:13:14, rsesek wrote: > base/mac/scoped_ioobject.h thanks for the pointer, I'll use this in the next revision. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:112: CFTypeRef dev_path_cf; On 2012/08/28 18:13:14, rsesek wrote: > base/mac/scoped_cftyperef.h will use in the next revision http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:124: CFStringGetCString((CFStringRef)dev_path_cf, On 2012/08/28 18:13:14, rsesek wrote: > There's string conversion utilities to do this for you in > base/sys_string_conversions.h. excellent, I'll make use of this http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:129: CFTypeRef removable_cf; On 2012/08/28 18:13:14, rsesek wrote: > ScopedCFTypeRef will use http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:131: IORegistryEntryCreateCFProperty(next_media, On 2012/08/28 18:13:14, rsesek wrote: > nit: continuations are indented by 4 will fix.
An updated implementation integrating all feedback so far. Thanks all. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:56: if (mount_point == "/Volumes/Recovery HD") On 2012/08/28 18:13:14, rsesek wrote: > A user can have more than one Recovery HD if they have multiple OS X installs on > the same machine. > > Is there a better way to test this? What flag is set on the device to exclude it > from these UIs? Done. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:96: io_iterator_t media_iterator; On 2012/08/28 18:13:14, rsesek wrote: > base/mac/scoped_ioobject.h Done. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:112: CFTypeRef dev_path_cf; On 2012/08/28 18:13:14, rsesek wrote: > base/mac/scoped_cftyperef.h Done. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:124: CFStringGetCString((CFStringRef)dev_path_cf, On 2012/08/28 18:13:14, rsesek wrote: > There's string conversion utilities to do this for you in > base/sys_string_conversions.h. Done. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:129: CFTypeRef removable_cf; On 2012/08/28 18:13:14, rsesek wrote: > ScopedCFTypeRef Done. http://codereview.chromium.org/10876101/diff/5002/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:131: IORegistryEntryCreateCFProperty(next_media, On 2012/08/28 18:13:14, rsesek wrote: > nit: continuations are indented by 4 Done.
Generally, we can avoid call StorageType multiple times. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:42: std::string StorageType(const std::string& volume); The responsibility of StorageType should construct dev_to_removable_map_ for later usage. How about "void BuildStorageTypeMap()" instead http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:53: Here may be a better place to call StorageType. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:60: dev_to_removable_map_[volume_dev] = StorageType(volume_dev); The StorageType will be called num_volumes*num_volumes. You may want to move it out the loop and call it once in QueryInfo before the loop starts. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:130: if (dev_path == vol) { map the dev_file_path_ to the type instead of only querying it for a specific volume.
Thanks Hongbo, you're quite right - building the map before iterating the devices makes much more sense. I should've seen that. Heres an updated CL. Thanks. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:42: std::string StorageType(const std::string& volume); On 2012/08/29 02:38:02, Hongbo Min wrote: > The responsibility of StorageType should construct dev_to_removable_map_ for > later usage. > > How about "void BuildStorageTypeMap()" instead Done. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:53: On 2012/08/29 02:38:02, Hongbo Min wrote: > Here may be a better place to call StorageType. Done. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:60: dev_to_removable_map_[volume_dev] = StorageType(volume_dev); On 2012/08/29 02:38:02, Hongbo Min wrote: > The StorageType will be called num_volumes*num_volumes. > > You may want to move it out the loop and call it once in QueryInfo before the > loop starts. Done. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:60: dev_to_removable_map_[volume_dev] = StorageType(volume_dev); On 2012/08/29 02:38:02, Hongbo Min wrote: > The StorageType will be called num_volumes*num_volumes. > > You may want to move it out the loop and call it once in QueryInfo before the > loop starts. Of course that's the right thing to do, I should have seen that. Thank you. http://codereview.chromium.org/10876101/diff/13001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:130: if (dev_path == vol) { On 2012/08/29 02:38:02, Hongbo Min wrote: > map the dev_file_path_ to the type instead of only querying it for a specific > volume. Done.
http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:71: int num_volumes = getmntinfo(&mounted_volumes, 0); getmntinfo gets called multiple times. But actually only one call is necessary for QueryInfo impl. It should be better if we can only call getmntinfo only once for QueryInfo, right? Be noticed that QueryUnitInfo may still need call getmntinfo to retrieve all units firstly. http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:76: std::string mount_point(mounted_volumes[i].f_mntonname); On mac, Is it possible to query the unit info only via the |id| instead of enumerating all unit infos and pick up the matched one?
Thanks again Hongbo. My only outstanding (minor) concern about the CL at this point is that if BuildStorageTypeMap() returns before the map is constructed (if one of the IOKit functions fails) we end up assigning an empty string to the type field of the StorageUnitInfo. So far as I can tell there's no equivalent to a Python dictionaries get() method, where I could supply a default value for type when the requested key in the map doesn't exist. I don't think it's very likely that we'll return early here so probably not a huge concern but if there's a C++/Chromium pattern I'm missing here I'd be happy to revise the code further to use it. Please review :-) http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:71: int num_volumes = getmntinfo(&mounted_volumes, 0); On 2012/08/29 10:12:15, Hongbo Min wrote: > getmntinfo gets called multiple times. But actually only one call is necessary > for QueryInfo impl. > > It should be better if we can only call getmntinfo only once for QueryInfo, > right? > > Be noticed that QueryUnitInfo may still need call getmntinfo to retrieve all > units firstly. Done. http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:76: std::string mount_point(mounted_volumes[i].f_mntonname); On 2012/08/29 10:12:15, Hongbo Min wrote: > On mac, Is it possible to query the unit info only via the |id| instead of > enumerating all unit infos and pick up the matched one? Yes, we can use statfs(). Iterating all of the mounted volumes is a legacy of an earlier implementation, thanks for catching this. http://codereview.chromium.org/10876101/diff/6005/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:76: std::string mount_point(mounted_volumes[i].f_mntonname); On 2012/08/29 10:12:15, Hongbo Min wrote: > On mac, Is it possible to query the unit info only via the |id| instead of > enumerating all unit infos and pick up the matched one? Done.
http://codereview.chromium.org/10876101/diff/6007/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/6007/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:77: info->type = dev_to_removable_map_[volume_dev]; If you are concerning about no value for the key volume_dev, you can firstly find it and then check whether the iterator is a end iterator, if yes, then a default value is assigned to type. Or make BuildStorageTypeMao return a bool value indicating whether it succeed to get types. If the return value is false, QueryInfo return false too. I think if the IOMasterPort/IOService* returns false, the system may be in a unexpected state, right?
OK, I've addressed my own nit here. And in the process discovered the kIOMasterPortDefault constant so I've saved a few LOC and tidied the patch up. Please review. http://codereview.chromium.org/10876101/diff/6007/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/6007/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:77: info->type = dev_to_removable_map_[volume_dev]; On 2012/08/30 00:13:32, Hongbo Min wrote: > If you are concerning about no value for the key volume_dev, you can firstly > find it and then check whether the iterator is a end iterator, if yes, then a > default value is assigned to type. Sure. I was just wondering if I was missing an obvious pattern. I guess not? > Or make BuildStorageTypeMao return a bool value indicating whether it succeed to > get types. If the return value is false, QueryInfo return false too. > > I think if the IOMasterPort/IOService* returns false, the system may be in a > unexpected state, right? I guess so. The documentation isn't especially clear on when this might happen but I think it's worth handling the failure. I'll push another minor change to do so.
http://codereview.chromium.org/10876101/diff/16001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:77: std::string type = dev_to_removable_map_[volume_dev]; Since QueryUnitInfo is exposed as public, what will happen if calling QueryUnitInfo without calling QueryInfo firstly, says the map is still not built yet? It can not get the type. Similar with statfs which can retrieve the information for the given id, is it also possible to retrieve the type information with the given id?
Here's another iteration which addresses Hongbo's concern on the public QueryUnitInfo() method being called before QueryInfo() has been. Please review. Thank you. http://codereview.chromium.org/10876101/diff/16001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/16001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:77: std::string type = dev_to_removable_map_[volume_dev]; On 2012/08/30 07:40:05, Hongbo Min wrote: > Since QueryUnitInfo is exposed as public, what will happen if calling > QueryUnitInfo without calling QueryInfo firstly, says the map is still not built > yet? It can not get the type. > > Similar with statfs which can retrieve the information for the given id, is it > also possible to retrieve the type information with the given id? So far as I can determine from the man pages, statfs doesn't include information on whether the volume is mounted from a removable device. That's why I went the IOKit route for that information. I know of no way to query whether an individual volume is removable. However it's easy to check whether the dev_to_removable_map_ is empty at the top of QueryUnitInfo() and call BuildStorageTypeMap() in that case.
http://codereview.chromium.org/10876101/diff/21001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/21001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:74: if (dev_to_removable_map_.empty()) how to about checking whether the dev_to_removable_map_ contains the value of |id|? Because after QuerInfo is called, the storage device may be attached/detached again.
It looks like some of this code will also be useful in the system monitor implementation for Mac.
This revision accounts for changes in the storage IDL definition and addresses Hongbo's review comments. http://codereview.chromium.org/10876101/diff/21001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/21001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:74: if (dev_to_removable_map_.empty()) On 2012/08/31 00:01:47, Hongbo Min wrote: > how to about checking whether the dev_to_removable_map_ contains the value of > |id|? Because after QuerInfo is called, the storage device may be > attached/detached again. Done.
On 2012/08/31 20:34:52, joshuagl wrote: > This revision accounts for changes in the storage IDL definition and addresses > Hongbo's review comments. > > http://codereview.chromium.org/10876101/diff/21001/chrome/browser/extensions/... > File > chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc > (right): > > http://codereview.chromium.org/10876101/diff/21001/chrome/browser/extensions/... > chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:74: > if (dev_to_removable_map_.empty()) > On 2012/08/31 00:01:47, Hongbo Min wrote: > > how to about checking whether the dev_to_removable_map_ contains the value of > > |id|? Because after QuerInfo is called, the storage device may be > > attached/detached again. > > Done. Thanks Joshua's continuous improvements. It is LGTM now. Mihai, need your LGTM before commit. Thanks.
LGTM, though I'd like Robert to have another look too. http://codereview.chromium.org/10876101/diff/16003/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/16003/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:42: std::map<std::string, std::string> dev_to_removable_map_; Nit: dev_path_to_type_map_ seems like a more appropriate name.
Thanks Mihai, map renamed to dev_path_to_type_map_ http://codereview.chromium.org/10876101/diff/16003/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/16003/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:42: std::map<std::string, std::string> dev_to_removable_map_; On 2012/09/01 00:27:21, Mihai Parparita wrote: > Nit: dev_path_to_type_map_ seems like a more appropriate name. Done.
Just a few more comments. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:55: for (int i = 0; i < num_volumes; i++) { Use prefix operator http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:83: // TODO(joshuagl) we're reporting different values than Disk Utility. nit: colon after ) for TODOs http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:127: dev_path_to_type_map_[dev_path] = systeminfo::kStorageTypeUnknown; Shouldn't there be a continue statement after this? Or, make the if statement below an else if. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:133: } nit: blank line after
Updated for rsesek's review. Thanks. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc (right): http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:55: for (int i = 0; i < num_volumes; i++) { On 2012/09/04 16:42:20, rsesek wrote: > Use prefix operator Done. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:83: // TODO(joshuagl) we're reporting different values than Disk Utility. On 2012/09/04 16:42:20, rsesek wrote: > nit: colon after ) for TODOs Done. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:127: dev_path_to_type_map_[dev_path] = systeminfo::kStorageTypeUnknown; On 2012/09/04 16:42:20, rsesek wrote: > Shouldn't there be a continue statement after this? Or, make the if statement > below an else if. Done. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:127: dev_path_to_type_map_[dev_path] = systeminfo::kStorageTypeUnknown; On 2012/09/04 16:42:20, rsesek wrote: > Shouldn't there be a continue statement after this? Or, make the if statement > below an else if. There certainly should. I've gone for making the if statement an else if. http://codereview.chromium.org/10876101/diff/26001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_mac.cc:133: } On 2012/09/04 16:42:20, rsesek wrote: > nit: blank line after Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joshua.lock@intel.com/10876101/18003
Presubmit check for 10876101-18003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** joshua.lock@intel.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Presubmit checks took 1.3s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joshua.lock@intel.com/10876101/18003
Presubmit check for 10876101-18003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** joshua.lock@intel.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joshua.lock@intel.com/10876101/18003
Presubmit check for 10876101-18003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** joshua.lock@intel.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Presubmit checks took 1.3s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Checking 'commit' box
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joshua.lock@intel.com/10876101/18003
Presubmit check for 10876101-18003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** joshua.lock@intel.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Presubmit checks took 1.4s to calculate.
Read the error message, please.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joshua.lock@intel.com/10876101/25003
Did you sign the CLA? I don't see your name there. Please don't tick the commit box until you've signed the corporate CLA: http://code.google.com/legal/corporate-cla-v1.0.html
On 2012/09/08 00:07:20, rsesek wrote: > Did you sign the CLA? I don't see your name there. Please don't tick the commit > box until you've signed the corporate CLA: > http://code.google.com/legal/corporate-cla-v1.0.html As I understood it I was already on the CCLA, I had to be added in order to contribute some patches to depot_tools. http://src.chromium.org/viewvc/chrome?view=rev&revision=149742
Yes, you are approved under Intel’s corporate CLA. Please add yourself to the AUTHORS file. On Fri, Sep 7, 2012 at 8:10 PM, <joshua.lock@intel.com> wrote: > On 2012/09/08 00:07:20, rsesek wrote: > >> Did you sign the CLA? I don't see your name there. Please don't tick the >> > commit > >> box until you've signed the corporate CLA: >> http://code.google.com/legal/**corporate-cla-v1.0.html<http://code.google.com... >> > > As I understood it I was already on the CCLA, I had to be added in order to > contribute some patches to depot_tools. > > http://src.chromium.org/**viewvc/chrome?view=rev&**revision=149742<http://src... > > https://chromiumcodereview.**appspot.com/10876101/<https://chromiumcodereview... >
Thanks for checking, Mark. I still don't see him in the spreadsheet, but this is now OK to commit. Thanks for waiting.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joshua.lock@intel.com/10876101/25003
Change committed as 155777 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
