Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11416089:
[Media Galleries] Filesystem interface for Mac PTP/MTP devices using ImageCaptureCore (part 3) (Closed)
|
Created:
8 years, 1 month ago by Greg Billock Modified:
7 years, 10 months ago CC:
chromium-reviews, sail+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFilesystem interface for Mac PTP/MTP devices using ImageCaptureCore
BUG=151681
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181532
Patch Set 1 #Patch Set 2 : Registration working for image capture devices #Patch Set 3 : Add hooks for FS delegate #Patch Set 4 : Stab at getting the delegate hooked up with the IC lib #Patch Set 5 : Interface class #Patch Set 6 : Working file metadata. Hooking up iterator. #Patch Set 7 : Giving files to enumerator #Patch Set 8 : Fix up iterator #Patch Set 9 : Merge to head #Patch Set 10 : Download path and lint fixes. #Patch Set 11 : Need IO permissions on worker thread pool #Patch Set 12 : Working on 10.8! #Patch Set 13 : Merge to head #Patch Set 14 : merge to head #Patch Set 15 : Rebase to current image capture api state #
Total comments: 25
Patch Set 16 : Add basic test and fixture #Patch Set 17 : Move files #
Total comments: 12
Patch Set 18 : Use single enumerator #
Total comments: 59
Patch Set 19 : Review fixes #
Total comments: 8
Patch Set 20 : Review fixes #Patch Set 21 : Add a bunch more tests #
Total comments: 2
Patch Set 22 : Rebase to head #Patch Set 23 : Merge to head #Patch Set 24 : Add locking in delegate #
Total comments: 11
Patch Set 25 : Merge to head #Patch Set 26 : Defensive enumerator #
Total comments: 10
Patch Set 27 : Cancel in UI thread, etc. #
Total comments: 10
Patch Set 28 : Merge to head #Patch Set 29 : Clarify mock object ownership #Patch Set 30 : Merge to head #Patch Set 31 : Merge to head #
Total comments: 3
Patch Set 32 : Add cocoa protocols include for 10.6 #Patch Set 33 : Fixup for 10.6 #Patch Set 34 : Hold off on enabling support -- ImageCapture notifications need fixing. #Patch Set 35 : Merge to head #Patch Set 36 : Can't not enable due to include defines. #Patch Set 37 : Disable a test for mac #Patch Set 38 : Merge to head, stop using base system monitor #Patch Set 39 : Don't use task runner for callback notifications #Patch Set 40 : Repair merge #Patch Set 41 : Disable GalleryNameMTP test for mac #Messages
Total messages: 47 (0 generated)
ok, this is ready to start looking at. It's adapted to the finalizing state of the ImageCapture library now. I think the thread adaptation is in a good state. Need to add tests...
quick first pass. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.h:31: explicit MTPDeviceDelegateImplMac( not explicit https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:110: DCHECK(MediaStorageUtil::CrackDeviceId(device_id, &type, &device_id_)); You won't get a |device_id_| in a release build. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:111: root_path_ = FilePath(location); Do this in the initializer list. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:138: file_info->size = i->second.size; Isn't this block the same as: *file_info = i->second; ? https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { You need to handle the recursive flag. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:209: base::Bind(&MTPDeviceDelegateImplMac::DeleteDelegateOnTaskRunner, You don't need DeleteDelegateOnTaskRunner. media_task_runner_->DeleteSoon(FROM_HERE, this); https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:254: LOG(INFO) << "Downloaded file " << name; debug statement https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:260: if (index >= static_cast<int>(file_paths_.size())) Make GetFile take a size_t. Make Enumerator::position_ a size_t as well. https://codereview.chromium.org/11416089/diff/28001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/11416089/diff/28001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:1052: 'browser/media_gallery/mtp_device_delegate_impl_mac.h', Move these guys to a mac sub-directory to be consistent with Linux (and soon Windows).
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.h:31: explicit MTPDeviceDelegateImplMac( On 2012/12/21 21:05:44, Lei Zhang wrote: > not explicit Oops! Didn't work through a rebase way back somewhere. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:110: DCHECK(MediaStorageUtil::CrackDeviceId(device_id, &type, &device_id_)); On 2012/12/21 21:05:44, Lei Zhang wrote: > You won't get a |device_id_| in a release build. I'm not seeing this. It looks like it's just governed by whether the device_id_ pointer gets passed. Are you saying in release we don't pass through the full device id? I'm back-dooring this anyway, by passing the device id in the synthetic location. Yes, I know this is absolutely hideous and an abomination. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:111: root_path_ = FilePath(location); On 2012/12/21 21:05:44, Lei Zhang wrote: > Do this in the initializer list. Done. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { On 2012/12/21 21:05:44, Lei Zhang wrote: > You need to handle the recursive flag. My synthetic directory has all files in the same directory, so I was just ignoring it. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:209: base::Bind(&MTPDeviceDelegateImplMac::DeleteDelegateOnTaskRunner, On 2012/12/21 21:05:44, Lei Zhang wrote: > You don't need DeleteDelegateOnTaskRunner. > > media_task_runner_->DeleteSoon(FROM_HERE, this); Done. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:254: LOG(INFO) << "Downloaded file " << name; On 2012/12/21 21:05:44, Lei Zhang wrote: > debug statement Done. https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:260: if (index >= static_cast<int>(file_paths_.size())) On 2012/12/21 21:05:44, Lei Zhang wrote: > Make GetFile take a size_t. Make Enumerator::position_ a size_t as well. Done. https://codereview.chromium.org/11416089/diff/28001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/11416089/diff/28001/chrome/chrome_browser.gyp... chrome/chrome_browser.gypi:1052: 'browser/media_gallery/mtp_device_delegate_impl_mac.h', On 2012/12/21 21:05:44, Lei Zhang wrote: > Move these guys to a mac sub-directory to be consistent with Linux (and soon > Windows). Done.
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:110: DCHECK(MediaStorageUtil::CrackDeviceId(device_id, &type, &device_id_)); On 2012/12/22 00:10:28, Greg Billock wrote: > On 2012/12/21 21:05:44, Lei Zhang wrote: > > You won't get a |device_id_| in a release build. > > I'm not seeing this. It looks like it's just governed by whether the device_id_ > pointer gets passed. Are you saying in release we don't pass through the full > device id? I'm back-dooring this anyway, by passing the device id in the > synthetic location. Yes, I know this is absolutely hideous and an abomination. What I'm trying to say is, this line won't get executed in a release build. You need to write this as: bool ret = CrackDeviceId(...) DCHECK(ret); https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { On 2012/12/22 00:10:28, Greg Billock wrote: > On 2012/12/21 21:05:44, Lei Zhang wrote: > > You need to handle the recursive flag. > > My synthetic directory has all files in the same directory, so I was just > ignoring it. Oh, is there no way to get the directory structure via ImageCapture?
https://chromiumcodereview.appspot.com/11416089/diff/36001/chrome/browser/med... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://chromiumcodereview.appspot.com/11416089/diff/36001/chrome/browser/med... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:56: // before CancelPendingTeasksAndDeleteDelegate is called. typo https://chromiumcodereview.appspot.com/11416089/diff/36001/chrome/browser/med... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:108: std::vector<Enumerator*> enumerators_; I don't think we'll have multiple enumerators. Also, enumerators tend to get iterated through all the way right after being created. Thus they'll be blocked waiting for device entries no matter what. Given this, would it simplify the implementation to just get the entire list of files when creating the enumerator? https://chromiumcodereview.appspot.com/11416089/diff/36001/chrome/browser/med... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://chromiumcodereview.appspot.com/11416089/diff/36001/chrome/browser/med... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:32: virtual ~CameraDeviceInterface() {} Should this be private since CloseCameraSessionAndDelete() is the intended way to delete this? https://chromiumcodereview.appspot.com/11416089/diff/36001/chrome/browser/med... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:171: download_events_[name] = &waiter; You got |download_events_| here, but can there ever be more than 1 download at a time?
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:110: DCHECK(MediaStorageUtil::CrackDeviceId(device_id, &type, &device_id_)); Oh, duh. Of course. Fixing. On 2012/12/22 00:18:30, Lei Zhang wrote: > On 2012/12/22 00:10:28, Greg Billock wrote: > > On 2012/12/21 21:05:44, Lei Zhang wrote: > > > You won't get a |device_id_| in a release build. > > > > I'm not seeing this. It looks like it's just governed by whether the > device_id_ > > pointer gets passed. Are you saying in release we don't pass through the full > > device id? I'm back-dooring this anyway, by passing the device id in the > > synthetic location. Yes, I know this is absolutely hideous and an abomination. > > What I'm trying to say is, this line won't get executed in a release build. You > need to write this as: > > bool ret = CrackDeviceId(...) > DCHECK(ret); https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { I think you can in theory -- if there's a USB and you get a hierarchy, there are at least folder events that are possible. I'm just ignoring those presently, though, and only remembering the files in a flat form. I think that'll work for PTP, but if we use this for USB, we'll want to do something more hierarchical. On 2012/12/22 00:18:30, Lei Zhang wrote: > On 2012/12/22 00:10:28, Greg Billock wrote: > > On 2012/12/21 21:05:44, Lei Zhang wrote: > > > You need to handle the recursive flag. > > > > My synthetic directory has all files in the same directory, so I was just > > ignoring it. > > Oh, is there no way to get the directory structure via ImageCapture? https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:56: // before CancelPendingTeasksAndDeleteDelegate is called. On 2012/12/22 02:44:35, Lei Zhang wrote: > typo Done. https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:108: std::vector<Enumerator*> enumerators_; Oh, that does simplify things. I'll assume that. On 2012/12/22 02:44:35, Lei Zhang wrote: > I don't think we'll have multiple enumerators. Also, enumerators tend to get > iterated through all the way right after being created. Thus they'll be blocked > waiting for device entries no matter what. > > Given this, would it simplify the implementation to just get the entire list of > files when creating the enumerator? The way I'm implementing it, the caller is free to start working with the files even before all of them are known. For cards with a lot of files, that's really nice -- the first ones come in, and you can download them while the other 1000 load, which can take a couple of minutes, in my experience. So I'll keep the blocking behavior, but I can definitely simplify the notification logic with only one iteration going on. https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:32: virtual ~CameraDeviceInterface() {} On 2012/12/22 02:44:35, Lei Zhang wrote: > Should this be private since CloseCameraSessionAndDelete() is the intended way > to delete this? I had it like that, but then TaskRunner::DeleteSoon won't work. It's convenient for unit tests to leave it like this, but perhaps that's misleading... https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:171: download_events_[name] = &waiter; On 2012/12/22 02:44:35, Lei Zhang wrote: > You got |download_events_| here, but can there ever be more than 1 download at a > time? I meant to ask you this. Is this always called in the sequenced runner? If not, I need some mutex gear around these structures. If so, I can simplify it a bit.
Couple of high level comments: - the CL description talks about PTP but the code says MTP - keeping a reference to the enumerator and returning a scoped_ptr<> seems really weird. At the very least the MTPDeviceDelegate API needs to be updated to state that the MTPDeviceDelegate IMPL needs to live longer than the enumerator. - there's a lot of multi threading going on but no locks. For example, it seems like multiple threads can be waiting for a file download. Doesn't this mean that there should be a lock around access to download_events_? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:8: #include <vector> newline after system includes https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class CameraDeviceInterface; make this an inner class? (you can still forward declare inner classes I think) https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:27: // and names of non-directories notified through the ItemAdded call will be "non-directories notified through" is a bit confusing. How about just "names of files will appear as children of that directory". https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:33: // Destructed via CancelPendingTasksAndDeleteDelegate. Do not call directly. make the destructor private and remove comment? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:80: bool HasAllFiles(); rename to received_all_files() ? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; use std::map<> unless there's a reason to use hash_map https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:98: // Notifications for all the files we're waiting on downloads for. Events for pending downloads? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:105: std::vector<FilePath::StringType> file_paths_; can this be vector<FilePath> instead? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:24: class CameraDeviceInterface The "Interface" part of the name doesn't add anything. Maybe "ImageCaptureDeviceListenerImpl" instead? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:26: public base::SupportsWeakPtr<CameraDeviceInterface> { Should avoid multiple inheritance. Use base::WeakPtrFactory instead? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:65: if (camera_device_.get()) { don't need this, method calls to nil are no-op https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:108: received_all_files_(false) { need to initialize enumerator_ https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: std::string device_id = FilePath(location).BaseName().value(); instead of implicitly encoding the device ID in the file path it should just be passed as an explicit parameter https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:139: file_info->size = i->second.size; would this work: *file_info = i->second https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:147: return base::PLATFORM_FILE_ERROR_NOT_FOUND; early return above instead? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:164: std::string name = device_file_path.BaseName().value(); should move to first use https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:221: FilePath fp = root_path_.Append(name); avoid abbreviations (more below) https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:288: return 0; remove https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:67: allMediaFiles_.reset([NSMutableArray arrayWithCapacity:1]); should retain. actually, just doing .reset([[NSMutableArray alloc] init]) would be better https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:177: ICCameraDevice* camera_; needs to be initialized in constructor, same with delegate_
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { If we can have a directory structure for data from PTP devices, that would be great. But maybe that can wait for a separate CL. On 2012/12/22 03:11:41, Greg Billock wrote: > I think you can in theory -- if there's a USB and you get a hierarchy, there are > at least folder events that are possible. I'm just ignoring those presently, > though, and only remembering the files in a flat form. I think that'll work for > PTP, but if we use this for USB, we'll want to do something more hierarchical. > > On 2012/12/22 00:18:30, Lei Zhang wrote: > > On 2012/12/22 00:10:28, Greg Billock wrote: > > > On 2012/12/21 21:05:44, Lei Zhang wrote: > > > > You need to handle the recursive flag. > > > > > > My synthetic directory has all files in the same directory, so I was just > > > ignoring it. > > > > Oh, is there no way to get the directory structure via ImageCapture? > https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:32: virtual ~CameraDeviceInterface() {} On 2012/12/22 03:11:41, Greg Billock wrote: > On 2012/12/22 02:44:35, Lei Zhang wrote: > > Should this be private since CloseCameraSessionAndDelete() is the intended way > > to delete this? > > I had it like that, but then TaskRunner::DeleteSoon won't work. It's convenient > for unit tests to leave it like this, but perhaps that's misleading... You just need to add: friend class base::DeleteHelper<T> https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:171: download_events_[name] = &waiter; On 2012/12/22 03:11:41, Greg Billock wrote: > On 2012/12/22 02:44:35, Lei Zhang wrote: > > You got |download_events_| here, but can there ever be more than 1 download at > a > > time? > > I meant to ask you this. Is this always called in the sequenced runner? If not, > I need some mutex gear around these structures. If so, I can simplify it a bit. I believe that to be the case.
On 2012/12/26 21:30:43, sail wrote: > Couple of high level comments: > - the CL description talks about PTP but the code says MTP The class is "MTP" but ImageCapture doesn't look like it does MTP yet, at least in 10.8. Switched up a bit. > - keeping a reference to the enumerator and returning a scoped_ptr<> seems > really weird. At the very least the MTPDeviceDelegate API needs to be updated to > state that the MTPDeviceDelegate IMPL needs to live longer than the enumerator. Yeah. Lei and I were talking about this. I'll update the interface as well. > - there's a lot of multi threading going on but no locks. For example, it > seems like multiple threads can be waiting for a file download. Doesn't this > mean that there should be a lock around access to download_events_? We talked about this, too. I wasn't quite sure where all the downloads were invoked or what the contract was. I'll add this to the comments as well. > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:8: #include > <vector> > newline after system includes > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class > CameraDeviceInterface; > make this an inner class? (you can still forward declare inner classes I think) > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:27: // and names > of non-directories notified through the ItemAdded call will be > "non-directories notified through" is a bit confusing. How about just "names of > files will appear as children of that directory". > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:33: // > Destructed via CancelPendingTasksAndDeleteDelegate. Do not call directly. > make the destructor private and remove comment? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:80: bool > HasAllFiles(); > rename to received_all_files() ? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: > base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; > use std::map<> unless there's a reason to use hash_map > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:98: // > Notifications for all the files we're waiting on downloads for. > Events for pending downloads? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:105: > std::vector<FilePath::StringType> file_paths_; > can this be vector<FilePath> instead? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:24: class > CameraDeviceInterface > The "Interface" part of the name doesn't add anything. Maybe > "ImageCaptureDeviceListenerImpl" instead? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:26: public > base::SupportsWeakPtr<CameraDeviceInterface> { > Should avoid multiple inheritance. Use base::WeakPtrFactory instead? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:65: if > (camera_device_.get()) { > don't need this, method calls to nil are no-op > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:108: > received_all_files_(false) { > need to initialize enumerator_ > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: > std::string device_id = FilePath(location).BaseName().value(); > instead of implicitly encoding the device ID in the file path it should just be > passed as an explicit parameter > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:139: > file_info->size = i->second.size; > would this work: > *file_info = i->second > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:147: return > base::PLATFORM_FILE_ERROR_NOT_FOUND; > early return above instead? > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:164: > std::string name = device_file_path.BaseName().value(); > should move to first use > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:221: FilePath > fp = root_path_.Append(name); > avoid abbreviations (more below) > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:288: return 0; > remove > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm > (right): > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:67: > allMediaFiles_.reset([NSMutableArray arrayWithCapacity:1]); > should retain. > actually, just doing .reset([[NSMutableArray alloc] init]) would be better > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:177: > ICCameraDevice* camera_; > needs to be initialized in constructor, same with delegate_
https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:32: virtual ~CameraDeviceInterface() {} On 2013/01/02 23:46:54, Lei Zhang wrote: > On 2012/12/22 03:11:41, Greg Billock wrote: > > On 2012/12/22 02:44:35, Lei Zhang wrote: > > > Should this be private since CloseCameraSessionAndDelete() is the intended > way > > > to delete this? > > > > I had it like that, but then TaskRunner::DeleteSoon won't work. It's > convenient > > for unit tests to leave it like this, but perhaps that's misleading... > > You just need to add: friend class base::DeleteHelper<T> I think I must have switched this around. It's in a scoped ptr in the delegate class now. https://codereview.chromium.org/11416089/diff/36001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:171: download_events_[name] = &waiter; On 2013/01/02 23:46:54, Lei Zhang wrote: > On 2012/12/22 03:11:41, Greg Billock wrote: > > On 2012/12/22 02:44:35, Lei Zhang wrote: > > > You got |download_events_| here, but can there ever be more than 1 download > at > > a > > > time? > > > > I meant to ask you this. Is this always called in the sequenced runner? If > not, > > I need some mutex gear around these structures. If so, I can simplify it a > bit. > > I believe that to be the case. OK. Sweetened this a bit. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:8: #include <vector> On 2012/12/26 21:30:43, sail wrote: > newline after system includes Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class CameraDeviceInterface; Could. Why do you think that's better? On 2012/12/26 21:30:43, sail wrote: > make this an inner class? (you can still forward declare inner classes I think) https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:27: // and names of non-directories notified through the ItemAdded call will be On 2012/12/26 21:30:43, sail wrote: > "non-directories notified through" is a bit confusing. How about just "names of > files will appear as children of that directory". Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:33: // Destructed via CancelPendingTasksAndDeleteDelegate. Do not call directly. On 2012/12/26 21:30:43, sail wrote: > make the destructor private and remove comment? Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:80: bool HasAllFiles(); On 2012/12/26 21:30:43, sail wrote: > rename to received_all_files() ? Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; On 2012/12/26 21:30:43, sail wrote: > use std::map<> unless there's a reason to use hash_map There could be tons of files. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:98: // Notifications for all the files we're waiting on downloads for. On 2012/12/26 21:30:43, sail wrote: > Events for pending downloads? Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:105: std::vector<FilePath::StringType> file_paths_; On 2012/12/26 21:30:43, sail wrote: > can this be vector<FilePath> instead? Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:24: class CameraDeviceInterface On 2012/12/26 21:30:43, sail wrote: > The "Interface" part of the name doesn't add anything. Maybe > "ImageCaptureDeviceListenerImpl" instead? Done, but just to "DeviceListenerImpl" to economize on space a bit. OK? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:26: public base::SupportsWeakPtr<CameraDeviceInterface> { On 2012/12/26 21:30:43, sail wrote: > Should avoid multiple inheritance. Use base::WeakPtrFactory instead? Could; this is pretty common though, and easy enough. I think it's basically intended to be used this way. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:65: if (camera_device_.get()) { On 2012/12/26 21:30:43, sail wrote: > don't need this, method calls to nil are no-op Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:108: received_all_files_(false) { On 2012/12/26 21:30:43, sail wrote: > need to initialize enumerator_ Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: std::string device_id = FilePath(location).BaseName().value(); On 2012/12/26 21:30:43, sail wrote: > instead of implicitly encoding the device ID in the file path it should just be > passed as an explicit parameter Yes. The machinery surrounding this doesn't allow that, but should. This is an ugly adaptation. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:139: file_info->size = i->second.size; On 2012/12/26 21:30:43, sail wrote: > would this work: > *file_info = i->second I think so. I thought I did that, but maybe I merged badly or it was elsewhere. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:147: return base::PLATFORM_FILE_ERROR_NOT_FOUND; On 2012/12/26 21:30:43, sail wrote: > early return above instead? Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:164: std::string name = device_file_path.BaseName().value(); eliminated. I think I had earlier code that used it, but anyway... On 2012/12/26 21:30:43, sail wrote: > should move to first use https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:221: FilePath fp = root_path_.Append(name); On 2012/12/26 21:30:43, sail wrote: > avoid abbreviations (more below) These are just aliasing abbreviations, like "i" or "iter". I think abbreviating makes that more clear. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:288: return 0; Oops! What happened to dead code detection... On 2012/12/26 21:30:43, sail wrote: > remove https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:67: allMediaFiles_.reset([NSMutableArray arrayWithCapacity:1]); On 2012/12/26 21:30:43, sail wrote: > should retain. > actually, just doing .reset([[NSMutableArray alloc] init]) would be better Done. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:177: ICCameraDevice* camera_; On 2012/12/26 21:30:43, sail wrote: > needs to be initialized in constructor, same with delegate_ Done.
https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class CameraDeviceInterface; On 2013/01/03 22:41:32, Greg Billock wrote: > Could. Why do you think that's better? > > On 2012/12/26 21:30:43, sail wrote: > > make this an inner class? (you can still forward declare inner classes I > think) > Currently this class is in the public name space. Making it an inner class pollutes the public name space less. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; On 2013/01/03 22:41:32, Greg Billock wrote: > On 2012/12/26 21:30:43, sail wrote: > > use std::map<> unless there's a reason to use hash_map > > There could be tons of files. Unless you can show a difference between hash_map and std::map for this use case please use std::map. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:24: class CameraDeviceInterface On 2013/01/03 22:41:32, Greg Billock wrote: > On 2012/12/26 21:30:43, sail wrote: > > The "Interface" part of the name doesn't add anything. Maybe > > "ImageCaptureDeviceListenerImpl" instead? > > Done, but just to "DeviceListenerImpl" to economize on space a bit. OK? Ok. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:26: public base::SupportsWeakPtr<CameraDeviceInterface> { On 2013/01/03 22:41:32, Greg Billock wrote: > On 2012/12/26 21:30:43, sail wrote: > > Should avoid multiple inheritance. Use base::WeakPtrFactory instead? > > Could; this is pretty common though, and easy enough. I think it's basically > intended to be used this way. That's not a good argument. Please avoid using multiple inheritance when ever possible. Also, in this case you don't need to expose the weak pointer to other classes. Using composition here better enforces that. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: std::string device_id = FilePath(location).BaseName().value(); On 2013/01/03 22:41:32, Greg Billock wrote: > On 2012/12/26 21:30:43, sail wrote: > > instead of implicitly encoding the device ID in the file path it should just > be > > passed as an explicit parameter > > Yes. The machinery surrounding this doesn't allow that, but should. This is an > ugly adaptation. In that case can you move this out to the code that instantiates this class and add a TODO there? There's no reason the new code needs to have all this. This will also simplify testing. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:221: FilePath fp = root_path_.Append(name); On 2013/01/03 22:41:32, Greg Billock wrote: > On 2012/12/26 21:30:43, sail wrote: > > avoid abbreviations (more below) > > These are just aliasing abbreviations, like "i" or "iter". I think abbreviating > makes that more clear. i and iter are used in all C++ programs. FilePath is not, it's a custom class. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:288: return 0; On 2013/01/03 22:41:32, Greg Billock wrote: > Oops! What happened to dead code detection... > > On 2012/12/26 21:30:43, sail wrote: > > remove > I'm not sure how the dead code detection works. The only thing I've seen cause an error is having unreferenced functions and variables. https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:55: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:68: //rrayWithCapacity:1]); remove https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:116: name_.reset([NSString stringWithString:name]); retain actually, you can just do [name retain] https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:185: }; DISALLOW copy/assign
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { On 2013/01/02 23:46:54, Lei Zhang wrote: > If we can have a directory structure for data from PTP devices, that would be > great. But maybe that can wait for a separate CL. I think PTP devices will report everything flattened, at least in the common cases. Let's hold off on it, as it'll introduce a bunch of hierarchical tracking that'll need to be added. > > On 2012/12/22 03:11:41, Greg Billock wrote: > > I think you can in theory -- if there's a USB and you get a hierarchy, there > are > > at least folder events that are possible. I'm just ignoring those presently, > > though, and only remembering the files in a flat form. I think that'll work > for > > PTP, but if we use this for USB, we'll want to do something more hierarchical. > > > > On 2012/12/22 00:18:30, Lei Zhang wrote: > > > On 2012/12/22 00:10:28, Greg Billock wrote: > > > > On 2012/12/21 21:05:44, Lei Zhang wrote: > > > > > You need to handle the recursive flag. > > > > > > > > My synthetic directory has all files in the same directory, so I was just > > > > ignoring it. > > > > > > Oh, is there no way to get the directory structure via ImageCapture? > > > https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class CameraDeviceInterface; On 2013/01/03 23:10:43, sail wrote: > On 2013/01/03 22:41:32, Greg Billock wrote: > > Could. Why do you think that's better? > > > > On 2012/12/26 21:30:43, sail wrote: > > > make this an inner class? (you can still forward declare inner classes I > > think) > > > > Currently this class is in the public name space. Making it an inner class > pollutes the public name space less. It's in chrome namespace, but that's not a lot better. :-) I'm only resisting because the outer class name is a bear, and makes the definition a serious mouthful. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; On 2013/01/03 23:10:43, sail wrote: > On 2013/01/03 22:41:32, Greg Billock wrote: > > On 2012/12/26 21:30:43, sail wrote: > > > use std::map<> unless there's a reason to use hash_map > > > > There could be tons of files. > > Unless you can show a difference between hash_map and std::map for this use case > please use std::map. The difference is O(n) access vs O(1) access. Is there something about the base hashtable header I don't know? It looks to me like the intended way to use hashtables to make the compilers all happy. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:26: public base::SupportsWeakPtr<CameraDeviceInterface> { On 2013/01/03 23:10:43, sail wrote: > On 2013/01/03 22:41:32, Greg Billock wrote: > > On 2012/12/26 21:30:43, sail wrote: > > > Should avoid multiple inheritance. Use base::WeakPtrFactory instead? > > > > Could; this is pretty common though, and easy enough. I think it's basically > > intended to be used this way. > > That's not a good argument. Please avoid using multiple inheritance when ever > possible. > > Also, in this case you don't need to expose the weak pointer to other classes. > Using composition here better enforces that. Neither of these are good arguments. The listener "superclass" is an interface, which are fine to multiply-inherit. If you argue that SupportsWeakPtr<T> shouldn't even exist in the first place, that's a bit more compelling, although the usage here is just about exactly the counterargument about why SWP exists--the listener expects to be used as a weak pointer. As to exposing the class as a weak pointer, I see what you're saying, I just don't think it's accurate -- anyone with a pointer can trivially make a weak pointer. There's nothing beyond ease-of-use and a mild documentation reason to have SupportsWeakPtr, so like I said, If you're an enemy of the whole idea, I can maybe even get behind that. :-) I just don't buy multiple inheritance or obstacles-to-clients as valid arguments. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: std::string device_id = FilePath(location).BaseName().value(); On 2013/01/03 23:10:43, sail wrote: > On 2013/01/03 22:41:32, Greg Billock wrote: > > On 2012/12/26 21:30:43, sail wrote: > > > instead of implicitly encoding the device ID in the file path it should just > > be > > > passed as an explicit parameter > > > > Yes. The machinery surrounding this doesn't allow that, but should. This is an > > ugly adaptation. > > In that case can you move this out to the code that instantiates this class and > add a TODO there? There's no reason the new code needs to have all this. > > This will also simplify testing. The outside code is in the File API, and introducing this dependency there is a worse outrage, in my view. The whole situation is a serious encapsulation flaw I'm attempting to fix in other ways, so I'm definitely not saying you're wrong to object to it. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:221: FilePath fp = root_path_.Append(name); On 2013/01/03 23:10:43, sail wrote: > On 2013/01/03 22:41:32, Greg Billock wrote: > > On 2012/12/26 21:30:43, sail wrote: > > > avoid abbreviations (more below) > > > > These are just aliasing abbreviations, like "i" or "iter". I think > abbreviating > > makes that more clear. > > i and iter are used in all C++ programs. FilePath is not, it's a custom class. OK, I can make them more self-documenting names, anyway, which will be good. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:288: return 0; On 2013/01/03 23:10:43, sail wrote: > On 2013/01/03 22:41:32, Greg Billock wrote: > > Oops! What happened to dead code detection... > > > > On 2012/12/26 21:30:43, sail wrote: > > > remove > > > > I'm not sure how the dead code detection works. The only thing I've seen cause > an error is having unreferenced functions and variables. Two returns means the second must be unreachable; I know I've seen compiler warnings of things like that... https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:55: }; On 2013/01/03 23:10:43, sail wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:68: //rrayWithCapacity:1]); On 2013/01/03 23:10:43, sail wrote: > remove Done. https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:116: name_.reset([NSString stringWithString:name]); On 2013/01/03 23:10:43, sail wrote: > retain > actually, you can just do [name retain] Done. https://codereview.chromium.org/11416089/diff/42001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:185: }; On 2013/01/03 23:10:43, sail wrote: > DISALLOW copy/assign Done.
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { On 2013/01/04 18:05:05, Greg Billock wrote: > I think PTP devices will report everything flattened, at least in the common > cases. Let's hold off on it, as it'll introduce a bunch of hierarchical tracking > that'll need to be added. It's true for some PTP devices, but most devices I have tried with has the directory structure.
https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... File chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/28001/chrome/browser/media_gall... chrome/browser/media_gallery/mtp_device_delegate_impl_mac.mm:152: bool recursive) { On 2013/01/04 20:28:19, Lei Zhang wrote: > On 2013/01/04 18:05:05, Greg Billock wrote: > > I think PTP devices will report everything flattened, at least in the common > > cases. Let's hold off on it, as it'll introduce a bunch of hierarchical > tracking > > that'll need to be added. > > It's true for some PTP devices, but most devices I have tried with has the > directory structure. They have a DCIM folder with all the media flat in it. (At least, that's what the few devices I've tried have.) For USB, obviously this is not true, but I don't know that we want to support USB with this code. I think it is a good idea to do this, but a good candidate to wait for a following change.
Didn't finish reviewing the code. I'll wait till the threading issues are resolved before doing a complete review. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class CameraDeviceInterface; On 2013/01/04 18:05:05, Greg Billock wrote: > On 2013/01/03 23:10:43, sail wrote: > > On 2013/01/03 22:41:32, Greg Billock wrote: > > > Could. Why do you think that's better? > > > > > > On 2012/12/26 21:30:43, sail wrote: > > > > make this an inner class? (you can still forward declare inner classes I > > > think) > > > > > > > Currently this class is in the public name space. Making it an inner class > > pollutes the public name space less. > > It's in chrome namespace, but that's not a lot better. :-) I'm only resisting > because the outer class name is a bear, and makes the definition a serious > mouthful. I'd really like this not to be public. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; On 2013/01/04 18:05:05, Greg Billock wrote: > On 2013/01/03 23:10:43, sail wrote: > > On 2013/01/03 22:41:32, Greg Billock wrote: > > > On 2012/12/26 21:30:43, sail wrote: > > > > use std::map<> unless there's a reason to use hash_map > > > > > > There could be tons of files. > > > > Unless you can show a difference between hash_map and std::map for this use > case > > please use std::map. > > The difference is O(n) access vs O(1) access. Is there something about the base > hashtable header I don't know? It looks to me like the intended way to use > hashtables to make the compilers all happy. Could you provide a reference for this? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: std::string device_id = FilePath(location).BaseName().value(); On 2013/01/04 18:05:05, Greg Billock wrote: > On 2013/01/03 23:10:43, sail wrote: > > On 2013/01/03 22:41:32, Greg Billock wrote: > > > On 2012/12/26 21:30:43, sail wrote: > > > > instead of implicitly encoding the device ID in the file path it should > just > > > be > > > > passed as an explicit parameter > > > > > > Yes. The machinery surrounding this doesn't allow that, but should. This is > an > > > ugly adaptation. > > > > In that case can you move this out to the code that instantiates this class > and > > add a TODO there? There's no reason the new code needs to have all this. > > > > This will also simplify testing. > > The outside code is in the File API, and introducing this dependency there is a > worse outrage, in my view. The whole situation is a serious encapsulation flaw > I'm attempting to fix in other ways, so I'm definitely not saying you're wrong > to object to it. Either way, it should be fixed before this can land. https://codereview.chromium.org/11416089/diff/51001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/51001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:191: // Artificially wake up any downloads pending with an error code. Is this needed because this class can be called from multiple threads? If so then you need locking. If not then you shouldn't need this.
Added pretty aggressive locking. I think it may be possible to scope it in a bit, but once you get going, locks tend to be grabby. :-/ https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:23: class CameraDeviceInterface; Moved this and shortened the name. On 2013/01/08 20:16:41, sail wrote: > On 2013/01/04 18:05:05, Greg Billock wrote: > > On 2013/01/03 23:10:43, sail wrote: > > > On 2013/01/03 22:41:32, Greg Billock wrote: > > > > Could. Why do you think that's better? > > > > > > > > On 2012/12/26 21:30:43, sail wrote: > > > > > make this an inner class? (you can still forward declare inner classes I > > > > think) > > > > > > > > > > Currently this class is in the public name space. Making it an inner class > > > pollutes the public name space less. > > > > It's in chrome namespace, but that's not a lot better. :-) I'm only resisting > > because the outer class name is a bear, and makes the definition a serious > > mouthful. > > I'd really like this not to be public. https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:96: base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_; log-n, so only a factor of 10 or so worse, not 1000. That still strikes me as a big hit. On 2013/01/08 20:16:41, sail wrote: > On 2013/01/04 18:05:05, Greg Billock wrote: > > On 2013/01/03 23:10:43, sail wrote: > > > On 2013/01/03 22:41:32, Greg Billock wrote: > > > > On 2012/12/26 21:30:43, sail wrote: > > > > > use std::map<> unless there's a reason to use hash_map > > > > > > > > There could be tons of files. > > > > > > Unless you can show a difference between hash_map and std::map for this use > > case > > > please use std::map. > > > > The difference is O(n) access vs O(1) access. Is there something about the > base > > hashtable header I don't know? It looks to me like the intended way to use > > hashtables to make the compilers all happy. > > Could you provide a reference for this? https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/35010/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:109: std::string device_id = FilePath(location).BaseName().value(); Moved to the factory method. I think that's cosmetically better, but a full solution is out of the scope of this change. It's not appropriate to make this change dependent on that. On 2013/01/08 20:16:41, sail wrote: > On 2013/01/04 18:05:05, Greg Billock wrote: > > On 2013/01/03 23:10:43, sail wrote: > > > On 2013/01/03 22:41:32, Greg Billock wrote: > > > > On 2012/12/26 21:30:43, sail wrote: > > > > > instead of implicitly encoding the device ID in the file path it should > > just > > > > be > > > > > passed as an explicit parameter > > > > > > > > Yes. The machinery surrounding this doesn't allow that, but should. This > is > > an > > > > ugly adaptation. > > > > > > In that case can you move this out to the code that instantiates this class > > and > > > add a TODO there? There's no reason the new code needs to have all this. > > > > > > This will also simplify testing. > > > > The outside code is in the File API, and introducing this dependency there is > a > > worse outrage, in my view. The whole situation is a serious encapsulation flaw > > I'm attempting to fix in other ways, so I'm definitely not saying you're wrong > > to object to it. > > Either way, it should be fixed before this can land. https://codereview.chromium.org/11416089/diff/51001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/51001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:191: // Artificially wake up any downloads pending with an error code. Looking at this more now. The SequencedTaskRunner used can block (and indeed, the caller depends on it to block). The usual wake-up routines for this get called from the device listener, and are scheduled within that task runner. This method as well is executed from within that task runner. But since that task runner has multiple threads, (since one thread is blocked), I think we need some locks here to control it. Lei, does that sound right? Or is the use of the task runner by the client sequenced such that it'll only be executing one thing in the delegate at a time, so this particular call, at least, doesn't need locking? The way the listener works is to just transpose UI thread callbacks from ImageCapture onto the task runner pool we're using. I think since these are not NonNestable, though, that we need to make this class thread-safe. On 2013/01/08 20:16:41, sail wrote: > Is this needed because this class can be called from multiple threads? > > If so then you need locking. > If not then you shouldn't need this.
https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:57: // before CancelPendingTasksAndDeleteDelegate is called. What you really want is for the Enumerator to stop working and stop accessing |delegate_| after CancelPendingTasksAndDeleteDelegate() gets called, right? How about when the Enumerator reaches the end, whether artificially or not, it can set |delegate_| to NULL and be done with it? https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:121: info.is_directory = true; |info| is not fully initialized. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:229: // directory. That's pretty much how PTP devices work, but if we want I still don't believe PTP devices are flat. Have you tried my 10 year old Kodak camera? If you plug it into a Linux machine and read its contents with gphoto2, you can see the directory structure. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:303: delegate_->GetFileInfo(delegate_->GetFile(position_ - 1), &info); While it does not make sense to call Size(), LastModifiedTime() or IsDirectory() before calling Next(), a caller can do so if it wanted to. In which case, |position_| would underflow. You probably want to return what EmptyFileEnumerator would return if |position_| is 0, just so you don't freak out Coverity and other security tools. The other case is if the device is empty. In which case GetFile(0) would return an empty path, and GetFileInfo() would fail. In which case, the value returned from |info| is uninitialized. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:323: void CreateMTPDeviceDelegate(const std::string& device_location, nit: |device_location| is of type FilePath::StringType to match the definition. Same with |device_name| below.
https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:57: // before CancelPendingTasksAndDeleteDelegate is called. On 2013/01/17 07:56:23, Lei Zhang wrote: > What you really want is for the Enumerator to stop working and stop accessing > |delegate_| after CancelPendingTasksAndDeleteDelegate() gets called, right? > > How about when the Enumerator reaches the end, whether artificially or not, it > can set |delegate_| to NULL and be done with it? I could put another interlock between them to relax this, but I think this contract is cheaper (since it is honored right now). I'd have to add guards in the iterator methods, as well as the additional setters, and make sure those are all synchronized correctly. But in general, the interlock is complicated since the threading model is that a caller can be blocked in the enumerator, the delegate needs to be able to know when it can (and must) wake up the enumerator. The async API ought to be serializable and much easier to deal with. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:121: info.is_directory = true; On 2013/01/17 07:56:23, Lei Zhang wrote: > |info| is not fully initialized. I'm relying on the empty constructor default values. Shall I be explicit? https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:229: // directory. That's pretty much how PTP devices work, but if we want On 2013/01/17 07:56:23, Lei Zhang wrote: > I still don't believe PTP devices are flat. Have you tried my 10 year old Kodak > camera? If you plug it into a Linux machine and read its contents with gphoto2, > you can see the directory structure. The claim isn't that they're flat, it's that the unsupported case -- devices where the organization is not flat AND that they use duplicate filenames in the hierarchy -- is pretty small. I plan to deal with it, but in another change, so that this one can deal with the MTPDeviceDelegate interface, locking, etc. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:303: delegate_->GetFileInfo(delegate_->GetFile(position_ - 1), &info); Yeah, you're right. This isn't very defensive. I'll change it. On 2013/01/17 07:56:23, Lei Zhang wrote: > While it does not make sense to call Size(), LastModifiedTime() or IsDirectory() > before calling Next(), a caller can do so if it wanted to. In which case, > |position_| would underflow. You probably want to return what > EmptyFileEnumerator would return if |position_| is 0, just so you don't freak > out Coverity and other security tools. > > The other case is if the device is empty. In which case GetFile(0) would return > an empty path, and GetFileInfo() would fail. In which case, the value returned > from |info| is uninitialized. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:323: void CreateMTPDeviceDelegate(const std::string& device_location, On 2013/01/17 07:56:23, Lei Zhang wrote: > nit: |device_location| is of type FilePath::StringType to match the definition. > Same with |device_name| below. Done.
lgtm, and hopefully we can remove a lot of the complexity once the fileapi becomes async. https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/62001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:121: info.is_directory = true; On 2013/01/25 18:41:40, Greg Billock wrote: > On 2013/01/17 07:56:23, Lei Zhang wrote: > > |info| is not fully initialized. > > I'm relying on the empty constructor default values. Shall I be explicit? Oh right, that's fine. https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:92: std::string device_id_; |device_id_| and |root_path_| can be const. https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:196: void MTPDeviceDelegateImplMac::CancelPendingTasksAndDeleteDelegate() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:299: if (next_file == FilePath()) You can just write this as: next_file.empty() https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:306: if (position_ <= 0 || done_) size_t position can't ever be < 0, ditto below. https://codereview.chromium.org/11416089/diff/74001/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_file_system_config.h (right): https://codereview.chromium.org/11416089/diff/74001/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_file_system_config.h:10: // Support MTP device file system for Windows, Linux and ChromeOS. Note that Forgot to mention Mac.
sail, want to take a look for ObjC? I think this one is less headache-inducing than the other CL... https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:92: std::string device_id_; On 2013/01/25 23:51:20, Lei Zhang wrote: > |device_id_| and |root_path_| can be const. Done. https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:196: void MTPDeviceDelegateImplMac::CancelPendingTasksAndDeleteDelegate() { On 2013/01/25 23:51:20, Lei Zhang wrote: > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Done. https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:299: if (next_file == FilePath()) On 2013/01/25 23:51:20, Lei Zhang wrote: > You can just write this as: next_file.empty() Done. https://codereview.chromium.org/11416089/diff/74001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:306: if (position_ <= 0 || done_) On 2013/01/25 23:51:20, Lei Zhang wrote: > size_t position can't ever be < 0, ditto below. Done. https://codereview.chromium.org/11416089/diff/74001/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_file_system_config.h (right): https://codereview.chromium.org/11416089/diff/74001/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_file_system_config.h:10: // Support MTP device file system for Windows, Linux and ChromeOS. Note that On 2013/01/25 23:51:20, Lei Zhang wrote: > Forgot to mention Mac. Done.
https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:118: no new line https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:267: else should remove https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:166: camera_ = [MockMTPICCameraDevice alloc]; camera_.reset([[MockMTPICCameraDevice alloc] init...]) https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:189: ICCameraDevice* camera_; scoped_nsobject?
https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/26 02:11:38, sail wrote: > 2013 Done. https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:118: On 2013/01/26 02:11:38, sail wrote: > no new line Done. https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:267: else On 2013/01/26 02:11:38, sail wrote: > should remove Done. https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:166: camera_ = [MockMTPICCameraDevice alloc]; On 2013/01/26 02:11:38, sail wrote: > camera_.reset([[MockMTPICCameraDevice alloc] init...]) This camera object is owned in the manager. Changed this to use the same mechanism as the test in the previous CL. https://codereview.chromium.org/11416089/diff/81002/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac_unittest.mm:189: ICCameraDevice* camera_; On 2013/01/26 02:11:38, sail wrote: > scoped_nsobject? Commented this to better reflect ownerships.
Objective-C code LGTM!
On 2013/01/29 19:05:40, sail wrote: > Objective-C code LGTM! Thanks! Adding kinuko and darin for OWNERS.
webkit/fileapi lgtm On 2013/01/25 23:51:20, Lei Zhang wrote: > lgtm, and hopefully we can remove a lot of the complexity once the fileapi > becomes async. +1, now the backend code supports async modules so I hope this happens shortly. Let us hear if we could do anything besides that.
https://codereview.chromium.org/11416089/diff/91001/chrome/browser/media_gall... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/11416089/diff/91001/chrome/browser/media_gall... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: 2012 -> 2013 https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_delegate.h (right): https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:5: #ifndef WEBKIT_FILEAPI_MEDIA_MTP_DEVICE_DELEGATE_H_ I'm apparently quite late to the party, but I don't understand why any code related to media gallery support should live under webkit/fileapi/. This directory is about supporting the fileapi backend. It seems like code pertaining to apps APIs should live at a higher level and perhaps plugin to the fileapi system as needed. Also, "media" as a sub-directory is usually reserved for directories containing src/media related code.
https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_delegate.h (right): https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_delegate.h:5: #ifndef WEBKIT_FILEAPI_MEDIA_MTP_DEVICE_DELEGATE_H_ On 2013/01/30 05:51:03, darin wrote: > I'm apparently quite late to the party, but I don't understand why any code > related to media gallery support should live under webkit/fileapi/. This > directory is about supporting the fileapi backend. It seems like code > pertaining to apps APIs should live at a higher level and perhaps plugin to the > fileapi system as needed. Agreed, we're in a way of refactoring this part (and mount-point system) more pluggable & extensible so we should be able to get rid of this fixture code soon (e.g. in Q1). > Also, "media" as a sub-directory is usually reserved for directories containing > src/media related code. Ah... my bad, I named this subdirectory. Should we rename this?
On Tue, Jan 29, 2013 at 11:12 PM, <kinuko@chromium.org> wrote: > > https://codereview.chromium.**org/11416089/diff/91001/** > webkit/fileapi/media/mtp_**device_delegate.h<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h> > File webkit/fileapi/media/mtp_**device_delegate.h (right): > > https://codereview.chromium.**org/11416089/diff/91001/** > webkit/fileapi/media/mtp_**device_delegate.h#newcode5<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h#newcode5> > webkit/fileapi/media/mtp_**device_delegate.h:5: #ifndef > WEBKIT_FILEAPI_MEDIA_MTP_**DEVICE_DELEGATE_H_ > On 2013/01/30 05:51:03, darin wrote: > >> I'm apparently quite late to the party, but I don't understand why any >> > code > >> related to media gallery support should live under webkit/fileapi/. >> > This > >> directory is about supporting the fileapi backend. It seems like code >> pertaining to apps APIs should live at a higher level and perhaps >> > plugin to the > >> fileapi system as needed. >> > > Agreed, we're in a way of refactoring this part (and mount-point system) > more pluggable & extensible so we should be able to get rid of this > fixture code soon (e.g. in Q1). > > Cool > > Also, "media" as a sub-directory is usually reserved for directories >> > containing > >> src/media related code. >> > > Ah... my bad, I named this subdirectory. Should we rename this? > > It would probably be a good idea. I know it's long, but it looks like "media_gallery" is used elsewhere. -Darin > https://codereview.chromium.**org/11416089/<https://codereview.chromium.org/1... >
On 2013/01/30 08:03:35, darin wrote: > On Tue, Jan 29, 2013 at 11:12 PM, <mailto:kinuko@chromium.org> wrote: > > > > > https://codereview.chromium.**org/11416089/diff/91001/** > > > webkit/fileapi/media/mtp_**device_delegate.h<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h> > > File webkit/fileapi/media/mtp_**device_delegate.h (right): > > > > https://codereview.chromium.**org/11416089/diff/91001/** > > > webkit/fileapi/media/mtp_**device_delegate.h#newcode5<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h#newcode5> > > webkit/fileapi/media/mtp_**device_delegate.h:5: #ifndef > > WEBKIT_FILEAPI_MEDIA_MTP_**DEVICE_DELEGATE_H_ > > On 2013/01/30 05:51:03, darin wrote: > > > >> I'm apparently quite late to the party, but I don't understand why any > >> > > code > > > >> related to media gallery support should live under webkit/fileapi/. > >> > > This > > > >> directory is about supporting the fileapi backend. It seems like code > >> pertaining to apps APIs should live at a higher level and perhaps > >> > > plugin to the > > > >> fileapi system as needed. > >> > > > > Agreed, we're in a way of refactoring this part (and mount-point system) > > more pluggable & extensible so we should be able to get rid of this > > fixture code soon (e.g. in Q1). > > > > > Cool > > > > > > Also, "media" as a sub-directory is usually reserved for directories > >> > > containing > > > >> src/media related code. > >> > > > > Ah... my bad, I named this subdirectory. Should we rename this? > > > > > It would probably be a good idea. I know it's long, but it looks like > "media_gallery" is used elsewhere. The same MTP module code is going to be used in ChromeOS's file browser to show the device contents, so the name "media_gallery" sounds a bit too narrow to me. "media_file_system" sounds better to me but is way more too long... well but on the second look the actual MTP access code is in "chrome/browser/media_gallery", so probably media_gallery should be the way to go. Greg/Lei do you mind changing the directory name? Or I may have a chance to create the patch before tomorrow in your time. > -Darin > > > > > https://codereview.chromium.**org/11416089/%3Chttps://codereview.chromium.org...> > >
On 2013/01/30 08:41:16, kinuko wrote: > On 2013/01/30 08:03:35, darin wrote: > > On Tue, Jan 29, 2013 at 11:12 PM, <mailto:kinuko@chromium.org> wrote: > > > > > > > > https://codereview.chromium.**org/11416089/diff/91001/** > > > > > > webkit/fileapi/media/mtp_**device_delegate.h<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h> > > > File webkit/fileapi/media/mtp_**device_delegate.h (right): > > > > > > https://codereview.chromium.**org/11416089/diff/91001/** > > > > > > webkit/fileapi/media/mtp_**device_delegate.h#newcode5<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h#newcode5> > > > webkit/fileapi/media/mtp_**device_delegate.h:5: #ifndef > > > WEBKIT_FILEAPI_MEDIA_MTP_**DEVICE_DELEGATE_H_ > > > On 2013/01/30 05:51:03, darin wrote: > > > > > >> I'm apparently quite late to the party, but I don't understand why any > > >> > > > code > > > > > >> related to media gallery support should live under webkit/fileapi/. > > >> > > > This > > > > > >> directory is about supporting the fileapi backend. It seems like code > > >> pertaining to apps APIs should live at a higher level and perhaps > > >> > > > plugin to the > > > > > >> fileapi system as needed. > > >> > > > > > > Agreed, we're in a way of refactoring this part (and mount-point system) > > > more pluggable & extensible so we should be able to get rid of this > > > fixture code soon (e.g. in Q1). > > > > > > > > Cool > > > > > > > > > > Also, "media" as a sub-directory is usually reserved for directories > > >> > > > containing > > > > > >> src/media related code. > > >> > > > > > > Ah... my bad, I named this subdirectory. Should we rename this? > > > > > > > > It would probably be a good idea. I know it's long, but it looks like > > "media_gallery" is used elsewhere. > > The same MTP module code is going to be used in ChromeOS's file browser to show > the device contents, so the name "media_gallery" sounds a bit too narrow to me. > "media_file_system" sounds better to me but is way more too long... well but on > the second look the actual MTP access code is in "chrome/browser/media_gallery", > so probably media_gallery should be the way to go. > > Greg/Lei do you mind changing the directory name? Or I may have a chance to > create the patch before tomorrow in your time. > > > -Darin > > > > > > > > > > https://codereview.chromium.**org/11416089/%253Chttps://codereview.chromium.o...> > > > Let's move this code under chrome/browser/media_gallery somewhere. But in the meantime, can we get this checked in? It'll be no significant difference to move this along with neighbors once we figure out what to do. Kinuko, if async support is now in place, please give Kausalya and I details so we can start to update to use the new interface. That's a priority for us. Thanks!
LGTM Definitely OK to consider refactorings in a separate CL. -Darin On Wed, Jan 30, 2013 at 9:03 AM, <gbillock@chromium.org> wrote: > On 2013/01/30 08:41:16, kinuko wrote: > >> On 2013/01/30 08:03:35, darin wrote: >> > On Tue, Jan 29, 2013 at 11:12 PM, <mailto:kinuko@chromium.org> wrote: >> > >> > > >> > > https://codereview.chromium.****org/11416089/diff/91001/** >> > > >> > >> > > webkit/fileapi/media/mtp_****device_delegate.h<https://** > codereview.chromium.org/**11416089/diff/91001/webkit/** > fileapi/media/mtp_device_**delegate.h<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h> > > > >> > > File webkit/fileapi/media/mtp_****device_delegate.h (right): >> > > >> > > https://codereview.chromium.****org/11416089/diff/91001/** >> > > >> > >> > > webkit/fileapi/media/mtp_****device_delegate.h#newcode5<htt** > ps://codereview.chromium.org/**11416089/diff/91001/webkit/** > fileapi/media/mtp_device_**delegate.h#newcode5<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h#newcode5> > > > >> > > webkit/fileapi/media/mtp_****device_delegate.h:5: #ifndef >> > > WEBKIT_FILEAPI_MEDIA_MTP_****DEVICE_DELEGATE_H_ >> > > On 2013/01/30 05:51:03, darin wrote: >> > > >> > >> I'm apparently quite late to the party, but I don't understand why >> any >> > >> >> > > code >> > > >> > >> related to media gallery support should live under webkit/fileapi/. >> > >> >> > > This >> > > >> > >> directory is about supporting the fileapi backend. It seems like >> code >> > >> pertaining to apps APIs should live at a higher level and perhaps >> > >> >> > > plugin to the >> > > >> > >> fileapi system as needed. >> > >> >> > > >> > > Agreed, we're in a way of refactoring this part (and mount-point >> system) >> > > more pluggable & extensible so we should be able to get rid of this >> > > fixture code soon (e.g. in Q1). >> > > >> > > >> > Cool >> > >> > >> > > >> > > Also, "media" as a sub-directory is usually reserved for directories >> > >> >> > > containing >> > > >> > >> src/media related code. >> > >> >> > > >> > > Ah... my bad, I named this subdirectory. Should we rename this? >> > > >> > > >> > It would probably be a good idea. I know it's long, but it looks like >> > "media_gallery" is used elsewhere. >> > > The same MTP module code is going to be used in ChromeOS's file browser to >> > show > >> the device contents, so the name "media_gallery" sounds a bit too narrow >> to >> > me. > >> "media_file_system" sounds better to me but is way more too long... well >> but >> > on > >> the second look the actual MTP access code is in >> > "chrome/browser/media_gallery"**, > >> so probably media_gallery should be the way to go. >> > > Greg/Lei do you mind changing the directory name? Or I may have a chance >> to >> create the patch before tomorrow in your time. >> > > > -Darin >> > >> > >> > > >> > >> > > https://codereview.chromium.****org/11416089/%253Chttps://code** > review.chromium.org/11416089/ <http://codereview.chromium.org/11416089/>> > >> > > >> > > Let's move this code under chrome/browser/media_gallery somewhere. > > But in the meantime, can we get this checked in? It'll be no significant > difference to move this along with neighbors once we figure out what to do. > > Kinuko, if async support is now in place, please give Kausalya and I > details so > we can start to update to use the new interface. That's a priority for us. > Thanks! > > https://codereview.chromium.**org/11416089/<https://codereview.chromium.org/1... >
On 2013/01/30 17:03:34, Greg Billock wrote: > On 2013/01/30 08:41:16, kinuko wrote: > > On 2013/01/30 08:03:35, darin wrote: > > > On Tue, Jan 29, 2013 at 11:12 PM, <mailto:kinuko@chromium.org> wrote: > > > > > > > > > > > https://codereview.chromium.**org/11416089/diff/91001/** > > > > > > > > > > webkit/fileapi/media/mtp_**device_delegate.h<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h> > > > > File webkit/fileapi/media/mtp_**device_delegate.h (right): > > > > > > > > https://codereview.chromium.**org/11416089/diff/91001/** > > > > > > > > > > webkit/fileapi/media/mtp_**device_delegate.h#newcode5<https://codereview.chromium.org/11416089/diff/91001/webkit/fileapi/media/mtp_device_delegate.h#newcode5> > > > > webkit/fileapi/media/mtp_**device_delegate.h:5: #ifndef > > > > WEBKIT_FILEAPI_MEDIA_MTP_**DEVICE_DELEGATE_H_ > > > > On 2013/01/30 05:51:03, darin wrote: > > > > > > > >> I'm apparently quite late to the party, but I don't understand why any > > > >> > > > > code > > > > > > > >> related to media gallery support should live under webkit/fileapi/. > > > >> > > > > This > > > > > > > >> directory is about supporting the fileapi backend. It seems like code > > > >> pertaining to apps APIs should live at a higher level and perhaps > > > >> > > > > plugin to the > > > > > > > >> fileapi system as needed. > > > >> > > > > > > > > Agreed, we're in a way of refactoring this part (and mount-point system) > > > > more pluggable & extensible so we should be able to get rid of this > > > > fixture code soon (e.g. in Q1). > > > > > > > > > > > Cool > > > > > > > > > > > > > > Also, "media" as a sub-directory is usually reserved for directories > > > >> > > > > containing > > > > > > > >> src/media related code. > > > >> > > > > > > > > Ah... my bad, I named this subdirectory. Should we rename this? > > > > > > > > > > > It would probably be a good idea. I know it's long, but it looks like > > > "media_gallery" is used elsewhere. > > > > The same MTP module code is going to be used in ChromeOS's file browser to > show > > the device contents, so the name "media_gallery" sounds a bit too narrow to > me. > > "media_file_system" sounds better to me but is way more too long... well but > on > > the second look the actual MTP access code is in > "chrome/browser/media_gallery", > > so probably media_gallery should be the way to go. > > > > Greg/Lei do you mind changing the directory name? Or I may have a chance to > > create the patch before tomorrow in your time. > > > > > -Darin > > > > > > > > > > > > > > > > https://codereview.chromium.**org/11416089/%25253Chttps://codereview.chromium...> > > > > > > Let's move this code under chrome/browser/media_gallery somewhere. > > But in the meantime, can we get this checked in? It'll be no significant > difference to move this along with neighbors once we figure out what to do. > > Kinuko, if async support is now in place, please give Kausalya and I details so > we can start to update to use the new interface. That's a priority for us. > Thanks! Sure... Kausalya should now some details, and I also cc'ed you to the related issue: http://crbug.com/154835 hope this new architecture works.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/91001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/84015
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/81008
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/83031
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
I've disabled a Mac registry test. It basically ends up wanting to create the MTP delegate deep in the registry guts. I'm not sure that's intentional or not, but we need to clear that up.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/124004
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/124008
Retried try job too often on mac_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11416089/130005
Message was sent while issue was closed.
Change committed as 181532 |
