Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12255023:
[Media Galleries] Switch Mac MTP delegate to async interface. (Closed)
|
Created:
7 years, 10 months ago by Greg Billock Modified:
7 years, 8 months ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, sail+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Media Galleries] Switch Mac MTP delegate to async interface.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192631
Patch Set 1 #Patch Set 2 : Merge to head and add test adaptation (still needs work) #Patch Set 3 : All tests passing #Patch Set 4 : No log lines #
Total comments: 36
Patch Set 5 : Tweaks and thread asserts #Patch Set 6 : Reorder some functions, add some more comments #
Total comments: 6
Patch Set 7 : Rebase #Patch Set 8 : Use Owned #Patch Set 9 : Handle multiple async calls #
Total comments: 27
Patch Set 10 : Rebase #Patch Set 11 : More asynciness, etc. #Patch Set 12 : comment #Patch Set 13 : Rebase #Patch Set 14 : Rebase #Patch Set 15 : Support overlapped dir reads #
Total comments: 8
Patch Set 16 : Repair erase #
Total comments: 2
Patch Set 17 : Iterator #Patch Set 18 : Add test for overlapped ReadDir #
Total comments: 10
Patch Set 19 : Constants &c #
Total comments: 6
Patch Set 20 : Rebase #Patch Set 21 : nits #
Messages
Total messages: 35 (0 generated)
This is ready for review. All tests passing.
I wasn't keeping up with this code so I don't think I can provide my feedback. Removing my self from reviewers.
https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:14: #include "base/sequenced_task_runner_helpers.h" This header file is not required. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:15: #include "base/synchronization/lock.h" Remove this include statement. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:16: #include "base/synchronization/waitable_event.h" Remove this include statement. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:17: #include "webkit/fileapi/file_system_file_util.h" Can you include this header file in .cc file? https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:21: class SequencedTaskRunner; No longer used. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:33: MTPDeviceDelegateImplMac(const std::string& device_id, This can be in private section. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:36: // MTPDeviceAsyncDelegate: You can declare these virtual functions in the private section. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:64: friend class base::DeleteHelper<MTPDeviceDelegateImplMac>; This is no longer required. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:79: void DownloadFile( Please document the new member functions. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:21: GetFileInfoSuccessCallback; Can you declare these type definitions in an anonymous namespace? https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:53: virtual void ResetDelegate(); OVERRIDE? https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:121: Please add DCHECKs in all member functions to make sure they are called on the right thread. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:137: void ForwardGetFileInfo( Can this go to the anonymous namespace? https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:163: base::Bind(&ForwardGetFileInfo, Why this closure is not owning |error| and |info|? If you make this closure own |error| and |info|, you can remove |info_deleter| and |error_deleter| from line 142 and 143 respectively. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:182: // Called on the IO thread by the async MTP file util. Please remove the comment and add a DCHECK in function definition. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:189: base::Unretained(this), By using "base::Unretained(this)" in a bind call, you are promising that object will be alive at the time of the call. From my understanding, MTPDeviceDelegateImplMac can be destructed before the callback is called. We should use a weak pointer here. Please correct me if I am wrong. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:193: void MTPDeviceDelegateImplMac::ReadDirectoryImpl( This class is currently accessed on multiple threads. E.g: MTPDeviceDelegateImplMac::ReadDirectory is called on the IO thread and MTPDeviceDelegateImplMac::ReadDirectoryImpl is called on the UI thread. Can we make MTPDeviceDelegateImplMac as a single threaded class? Sample: namespace chrome { namespace { void ReadDirectoryOnUIThread(...) { DCHECK(Called on UI thread); } } // namespace void MTPDeviceDelegateImplMac::ReadDirectory(...) { DCHECK(Called on IO thread); PostTaskOnUIThread(&ReadDirectoryOnUIThread, callback); } } // namespace https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:263: delete this; This class object is constructed on the IO thread and destructed on the UI thread. We should avoid this. This class should completely live on the IO thread and should do a call-and-reply to the UI thread to get the required device details.
https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:14: #include "base/sequenced_task_runner_helpers.h" On 2013/02/26 17:59:15, kmadhusu wrote: > This header file is not required. Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:15: #include "base/synchronization/lock.h" On 2013/02/26 17:59:15, kmadhusu wrote: > Remove this include statement. Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:16: #include "base/synchronization/waitable_event.h" On 2013/02/26 17:59:15, kmadhusu wrote: > Remove this include statement. Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:17: #include "webkit/fileapi/file_system_file_util.h" On 2013/02/26 17:59:15, kmadhusu wrote: > Can you include this header file in .cc file? Dropped. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:21: class SequencedTaskRunner; On 2013/02/26 17:59:15, kmadhusu wrote: > No longer used. Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:33: MTPDeviceDelegateImplMac(const std::string& device_id, On 2013/02/26 17:59:15, kmadhusu wrote: > This can be in private section. Create method needs to call it. It'd require more friend statements. I think this is OK. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:36: // MTPDeviceAsyncDelegate: On 2013/02/26 17:59:15, kmadhusu wrote: > You can declare these virtual functions in the private section. Technically, yes, but since the interface makes them public, they are accessible, and the unit tests call them. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:64: friend class base::DeleteHelper<MTPDeviceDelegateImplMac>; On 2013/02/26 17:59:15, kmadhusu wrote: > This is no longer required. Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h:79: void DownloadFile( On 2013/02/26 17:59:15, kmadhusu wrote: > Please document the new member functions. Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:21: GetFileInfoSuccessCallback; On 2013/02/26 17:59:15, kmadhusu wrote: > Can you declare these type definitions in an anonymous namespace? Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:53: virtual void ResetDelegate(); On 2013/02/26 17:59:15, kmadhusu wrote: > OVERRIDE? No, this isn't an ImageCaptureDeviceListener method. It's around for destruction behavior. I'll add a note about that. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:121: On 2013/02/26 17:59:15, kmadhusu wrote: > Please add DCHECKs in all member functions to make sure they are called on the > right thread. The important ones are the state-modifying ones. I added them there. The others are not needed. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:137: void ForwardGetFileInfo( On 2013/02/26 17:59:15, kmadhusu wrote: > Can this go to the anonymous namespace? Done. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:163: base::Bind(&ForwardGetFileInfo, On 2013/02/26 17:59:15, kmadhusu wrote: > Why this closure is not owning |error| and |info|? If you make this closure own > |error| and |info|, you can remove |info_deleter| and |error_deleter| from line > 142 and 143 respectively. There aren't good copy constructors for PlatformFileInfo. Since I needed to pass that one for deletion, I decided to do it for the other one too so it wouldn't create a question. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:182: // Called on the IO thread by the async MTP file util. On 2013/02/26 17:59:15, kmadhusu wrote: > Please remove the comment and add a DCHECK in function definition. Added DCHECK to the UI thread functions. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:189: base::Unretained(this), On 2013/02/26 17:59:15, kmadhusu wrote: > By using "base::Unretained(this)" in a bind call, you are promising that object > will be alive at the time of the call. > > From my understanding, MTPDeviceDelegateImplMac can be destructed before the > callback is called. We should use a weak pointer here. Please correct me if I am > wrong. Yes. I added a note in the header that the owner is basically guaranteeing lifetime past response times. If that's not accurate, then yes, we need weak pointers, but I think that's a fair thread-compatibility contract for the filesystem API. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:193: void MTPDeviceDelegateImplMac::ReadDirectoryImpl( On 2013/02/26 17:59:15, kmadhusu wrote: > This class is currently accessed on multiple threads. > > E.g: MTPDeviceDelegateImplMac::ReadDirectory is called on the IO thread and > MTPDeviceDelegateImplMac::ReadDirectoryImpl is called on the UI thread. > > Can we make MTPDeviceDelegateImplMac as a single threaded class? > > Sample: > > namespace chrome { > namespace { > > void ReadDirectoryOnUIThread(...) { > DCHECK(Called on UI thread); > } > > } // namespace > > void MTPDeviceDelegateImplMac::ReadDirectory(...) { > DCHECK(Called on IO thread); > PostTaskOnUIThread(&ReadDirectoryOnUIThread, callback); > } > > } // namespace The IO-called methods don't do any state mutation -- they just forward directly on the UI thread, so that keeps the class thread-compatible -- it can be created by anyone, but thereafter only works on the UI thread. https://codereview.chromium.org/12255023/diff/5002/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:263: delete this; On 2013/02/26 17:59:15, kmadhusu wrote: > This class object is constructed on the IO thread and destructed on the UI > thread. We should avoid this. This class should completely live on the IO thread > and should do a call-and-reply to the UI thread to get the required device > details. I don't see that as a problem. The object is thread-compatible, meaning that it guards itself from state corruption as long as a creator doesn't use it uninitialized (which they shouldn't be able to do by using the offered constructor). We could bounce creation to the UI thread, but I don't see that as being a win. It'd keep it fully on one thread, but since it wants to handle inbound IO-thread calls anyway, we'd either need a pair of objects that have identical scopes and lifetimes, or use a dummy. I don't think either is clearer than using forwarding continuations.
Adding thestig@ as reviewer. I am currently busy. Sorry, I don't have time to do a detailed review of this CL. I am going to defer this review to thestig@ who is familiar about MTPDeviceAsyncDelegate code. I will be happy to be in the reviewers list to provide drive-by comments. Thanks.
https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:168: content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI, You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl returns a pair with the info and error instead. Alternatively, make |info| Owned(info) and similarly for |error| in the ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need to have the scoped deleters. https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:225: read_directory_success_callback_ = success_callback; Isn't it possible for this delegate to receive multiple read directory calls? i.e. there can be more than 1 request in flight. https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:226: read_directory_error_callback_ = error_callback; |read_directory_error_callback_| never gets called.
Added kinuko. Can you tell me about the parallel invocation of the async methods? I've been assuming that there won't be any. Is that true? Or do we want to enable the async interface to be able to call re-entrant into the interface? https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:168: content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI, On 2013/03/04 22:45:01, Lei Zhang wrote: > You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl returns a pair > with the info and error instead. > > Alternatively, make |info| Owned(info) and similarly for |error| in the > ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need to have the > scoped deleters. Used Owned() here and in ReadDir. https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:225: read_directory_success_callback_ = success_callback; On 2013/03/04 22:45:01, Lei Zhang wrote: > Isn't it possible for this delegate to receive multiple read directory calls? > i.e. there can be more than 1 request in flight. I'm not sure. I'll ask. https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:226: read_directory_error_callback_ = error_callback; On 2013/03/04 22:45:01, Lei Zhang wrote: > |read_directory_error_callback_| never gets called. Yes. I'm not sure what to do about that. Schedule a timeout and call it if we don't have metadata by that time?
On 2013/03/06 00:28:29, Greg Billock wrote: > On 2013/03/04 22:45:01, Lei Zhang wrote: > > Isn't it possible for this delegate to receive multiple read directory calls? > > i.e. there can be more than 1 request in flight. > > I'm not sure. I'll ask. Kausalya has been the Windows/Linux version as though multiple operations can be issued before the in-flight operation gets retired. Thus there is a queue of pending tasks. Either those implementations contain too much complexity, or this CL does not have enough.
On 2013/03/06 00:28:29, Greg Billock wrote: > Added kinuko. Can you tell me about the parallel invocation of the async > methods? I've been assuming that there won't be any. Is that true? Or do we want > to enable the async interface to be able to call re-entrant into the interface? The current implementation allows multiple async methods to be called in parallel. In a simple architecture where all entries are read at once in a single task that runs on a single thread or on a single SequencedTaskRunner there's no need to worry about the re-entrancy, but I guessed it may not be the case in your code. > https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... > File chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm (right): > > https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:168: > content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI, > On 2013/03/04 22:45:01, Lei Zhang wrote: > > You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl returns a > pair > > with the info and error instead. > > > > Alternatively, make |info| Owned(info) and similarly for |error| in the > > ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need to have > the > > scoped deleters. > > Used Owned() here and in ReadDir. > > https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:225: > read_directory_success_callback_ = success_callback; > On 2013/03/04 22:45:01, Lei Zhang wrote: > > Isn't it possible for this delegate to receive multiple read directory calls? > > i.e. there can be more than 1 request in flight. > > I'm not sure. I'll ask. > > https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_galle... > chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm:226: > read_directory_error_callback_ = error_callback; > On 2013/03/04 22:45:01, Lei Zhang wrote: > > |read_directory_error_callback_| never gets called. > > Yes. I'm not sure what to do about that. Schedule a timeout and call it if we > don't have metadata by that time?
Thanks, Kinuko. My mental image (calls within a sequenced task runner) was wrong, or perhaps we want to maintain flexibility. I'll change the implementation to support simultaneous calls. There's another issue with the mac implementation that is an impedance mismatch. The API wants to read directories/files driven by the caller. The Mac PTP implementation wants to push those events from the library upstream. So somewhere (namely, my adapter), there's got to be a wait state that waits for pushed results from the OS before replying to the pull request from the filesystem API. So the question is, what do we do about that? Shall I set a timeout in the code and eventually just bail out on any pending directory read? On Tue, Mar 5, 2013 at 8:12 PM, <kinuko@chromium.org> wrote: > On 2013/03/06 00:28:29, Greg Billock wrote: > >> Added kinuko. Can you tell me about the parallel invocation of the async >> methods? I've been assuming that there won't be any. Is that true? Or do >> we >> > want > >> to enable the async interface to be able to call re-entrant into the >> > interface? > > The current implementation allows multiple async methods to be called in > parallel. In a simple architecture where all entries are read at once in a > single task that runs on a single thread or on a single SequencedTaskRunner > there's no need to worry about the re-entrancy, but I guessed it may not > be the > case in your code. > > > > https://codereview.chromium.**org/12255023/diff/7004/chrome/** > browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm> > >> File chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm<http://mtp_device_delegate_impl_mac.mm>(right): >> > > > https://codereview.chromium.**org/12255023/diff/7004/chrome/** > browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode168<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode168> > >> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm:168<http://mtp_device_delegate_impl_mac.mm:168> >> : >> content::BrowserThread::**PostTaskAndReply(content::**BrowserThread::UI, >> On 2013/03/04 22:45:01, Lei Zhang wrote: >> > You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl >> returns a >> pair >> > with the info and error instead. >> > >> > Alternatively, make |info| Owned(info) and similarly for |error| in the >> > ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need to >> have >> the >> > scoped deleters. >> > > Used Owned() here and in ReadDir. >> > > > https://codereview.chromium.**org/12255023/diff/7004/chrome/** > browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode225<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode225> > >> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm:225<http://mtp_device_delegate_impl_mac.mm:225> >> : >> read_directory_success_**callback_ = success_callback; >> On 2013/03/04 22:45:01, Lei Zhang wrote: >> > Isn't it possible for this delegate to receive multiple read directory >> > calls? > >> > i.e. there can be more than 1 request in flight. >> > > I'm not sure. I'll ask. >> > > > https://codereview.chromium.**org/12255023/diff/7004/chrome/** > browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode226<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode226> > >> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm:226<http://mtp_device_delegate_impl_mac.mm:226> >> : >> read_directory_error_callback_ = error_callback; >> On 2013/03/04 22:45:01, Lei Zhang wrote: >> > |read_directory_error_**callback_| never gets called. >> > > Yes. I'm not sure what to do about that. Schedule a timeout and call it >> if we >> don't have metadata by that time? >> > > > > https://codereview.chromium.**org/12255023/<https://codereview.chromium.org/1... >
On Thu, Mar 7, 2013 at 2:49 AM, Greg Billock <gbillock@chromium.org> wrote: > Thanks, Kinuko. My mental image (calls within a sequenced task runner) was > wrong, or perhaps we want to maintain flexibility. I'll change the > implementation to support simultaneous calls. > I think you could probably simply queue up requests (callbacks) and handle them one by one. > > There's another issue with the mac implementation that is an impedance > mismatch. The API wants to read directories/files driven by the caller. The > Mac PTP implementation wants to push those events from the library > upstream. So somewhere (namely, my adapter), there's got to be a wait state > that waits for pushed results from the OS before replying to the pull > request from the filesystem API. So the question is, what do we do about > that? Shall I set a timeout in the code and eventually just bail out on any > pending directory read? > Is there a way to tell if it has finished reading all entries? (Looks like there is) Or are there any error callbacks it could get? Anyway setting a reasonable timeout sounds good to me in general. We should not wait forever there. Related question-- when we have hundreds of files we might end up waiting for very long time and return with error? Or maybe just returning what it has at the point would be better. Also it sounds like we'd better support reading directories in multiple chunks (so that we can return first 20 entries and continue and so on) at some time in the future? On Tue, Mar 5, 2013 at 8:12 PM, <kinuko@chromium.org> wrote: > >> On 2013/03/06 00:28:29, Greg Billock wrote: >> >>> Added kinuko. Can you tell me about the parallel invocation of the async >>> methods? I've been assuming that there won't be any. Is that true? Or do >>> we >>> >> want >> >>> to enable the async interface to be able to call re-entrant into the >>> >> interface? >> >> The current implementation allows multiple async methods to be called in >> parallel. In a simple architecture where all entries are read at once in a >> single task that runs on a single thread or on a single >> SequencedTaskRunner >> there's no need to worry about the re-entrancy, but I guessed it may not >> be the >> case in your code. >> >> >> >> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm> >> >>> File chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_** >>> mac.mm <http://mtp_device_delegate_impl_mac.mm> (right): >>> >> >> >> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode168<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode168> >> >>> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm:168<http://mtp_device_delegate_impl_mac.mm:168> >>> : >>> content::BrowserThread::**PostTaskAndReply(content::**BrowserThread::UI, >>> On 2013/03/04 22:45:01, Lei Zhang wrote: >>> > You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl >>> returns a >>> pair >>> > with the info and error instead. >>> > >>> > Alternatively, make |info| Owned(info) and similarly for |error| in the >>> > ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need to >>> have >>> the >>> > scoped deleters. >>> >> >> Used Owned() here and in ReadDir. >>> >> >> >> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode225<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode225> >> >>> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm:225<http://mtp_device_delegate_impl_mac.mm:225> >>> : >>> read_directory_success_**callback_ = success_callback; >>> On 2013/03/04 22:45:01, Lei Zhang wrote: >>> > Isn't it possible for this delegate to receive multiple read directory >>> >> calls? >> >>> > i.e. there can be more than 1 request in flight. >>> >> >> I'm not sure. I'll ask. >>> >> >> >> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode226<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode226> >> >>> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_**mac.mm:226<http://mtp_device_delegate_impl_mac.mm:226> >>> : >>> read_directory_error_callback_ = error_callback; >>> On 2013/03/04 22:45:01, Lei Zhang wrote: >>> > |read_directory_error_**callback_| never gets called. >>> >> >> Yes. I'm not sure what to do about that. Schedule a timeout and call it >>> if we >>> don't have metadata by that time? >>> >> >> >> >> https://codereview.chromium.**org/12255023/<https://codereview.chromium.org/1... >> > >
On Wed, Mar 6, 2013 at 10:07 PM, Kinuko Yasuda <kinuko@chromium.org> wrote: > On Thu, Mar 7, 2013 at 2:49 AM, Greg Billock <gbillock@chromium.org>wrote: > >> Thanks, Kinuko. My mental image (calls within a sequenced task runner) >> was wrong, or perhaps we want to maintain flexibility. I'll change the >> implementation to support simultaneous calls. >> > > I think you could probably simply queue up requests (callbacks) and handle > them one by one. > For most queries, I'm just handling them in continuations. The ReadDir call is the one I need to fix up. > >> > There's another issue with the mac implementation that is an impedance >> mismatch. The API wants to read directories/files driven by the caller. The >> Mac PTP implementation wants to push those events from the library >> upstream. So somewhere (namely, my adapter), there's got to be a wait state >> that waits for pushed results from the OS before replying to the pull >> request from the filesystem API. So the question is, what do we do about >> that? Shall I set a timeout in the code and eventually just bail out on any >> pending directory read? >> > > Is there a way to tell if it has finished reading all entries? (Looks like > there is) Or are there any error callbacks it could get? Anyway setting a > reasonable timeout sounds good to me in general. We should not wait > forever there. > Related question-- when we have hundreds of files we might end up waiting > for very long time and return with error? Or maybe just returning what it > has at the point would be better. Also it sounds like we'd better support > reading directories in multiple chunks (so that we can return first 20 > entries and continue and so on) at some time in the future? > I think there's an option for that (or at least I thought there was -- I remember adding a TODO for it). > > On Tue, Mar 5, 2013 at 8:12 PM, <kinuko@chromium.org> wrote: >> >>> On 2013/03/06 00:28:29, Greg Billock wrote: >>> >>>> Added kinuko. Can you tell me about the parallel invocation of the async >>>> methods? I've been assuming that there won't be any. Is that true? Or >>>> do we >>>> >>> want >>> >>>> to enable the async interface to be able to call re-entrant into the >>>> >>> interface? >>> >>> The current implementation allows multiple async methods to be called in >>> parallel. In a simple architecture where all entries are read at once in >>> a >>> single task that runs on a single thread or on a single >>> SequencedTaskRunner >>> there's no need to worry about the re-entrancy, but I guessed it may not >>> be the >>> case in your code. >>> >>> >>> >>> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >>> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm> >>> >>>> File chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_** >>>> mac.mm <http://mtp_device_delegate_impl_mac.mm> (right): >>>> >>> >>> >>> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >>> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode168<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode168> >>> >>>> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_** >>>> mac.mm:168 <http://mtp_device_delegate_impl_mac.mm:168>: >>>> content::BrowserThread::**PostTaskAndReply(content::** >>>> BrowserThread::UI, >>>> On 2013/03/04 22:45:01, Lei Zhang wrote: >>>> > You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl >>>> returns a >>>> pair >>>> > with the info and error instead. >>>> > >>>> > Alternatively, make |info| Owned(info) and similarly for |error| in >>>> the >>>> > ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need >>>> to have >>>> the >>>> > scoped deleters. >>>> >>> >>> Used Owned() here and in ReadDir. >>>> >>> >>> >>> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >>> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode225<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode225> >>> >>>> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_** >>>> mac.mm:225 <http://mtp_device_delegate_impl_mac.mm:225>: >>>> read_directory_success_**callback_ = success_callback; >>>> On 2013/03/04 22:45:01, Lei Zhang wrote: >>>> > Isn't it possible for this delegate to receive multiple read directory >>>> >>> calls? >>> >>>> > i.e. there can be more than 1 request in flight. >>>> >>> >>> I'm not sure. I'll ask. >>>> >>> >>> >>> https://codereview.chromium.**org/12255023/diff/7004/chrome/** >>> browser/media_gallery/mac/mtp_**device_delegate_impl_mac.mm#**newcode226<https://codereview.chromium.org/12255023/diff/7004/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm#newcode226> >>> >>>> chrome/browser/media_gallery/**mac/mtp_device_delegate_impl_** >>>> mac.mm:226 <http://mtp_device_delegate_impl_mac.mm:226>: >>>> read_directory_error_callback_ = error_callback; >>>> On 2013/03/04 22:45:01, Lei Zhang wrote: >>>> > |read_directory_error_**callback_| never gets called. >>>> >>> >>> Yes. I'm not sure what to do about that. Schedule a timeout and call it >>>> if we >>>> don't have metadata by that time? >>>> >>> >>> >>> >>> https://codereview.chromium.**org/12255023/<https://codereview.chromium.org/1... >>> >> >> >
OK. Updated to handle multiple simultaneous async calls.
On 2013/03/07 23:53:57, Greg Billock wrote: > OK. Updated to handle multiple simultaneous async calls. Any more comments here?
On 2013/03/11 15:52:21, Greg Billock wrote: > On 2013/03/07 23:53:57, Greg Billock wrote: > > OK. Updated to handle multiple simultaneous async calls. > > Any more comments here? Sorry if you were waiting for my comments, this one slipped under my radar. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:228: read_dir_transactions_[root.value()] = What happens if we get parallel ReadDirectory requests for the same path? (And if there's already an entry for the path we probably want to batch requests) https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:231: if (received_all_files_) { Does this flag indicate it received all files (not only for the requested path) from the device? ...It looks so. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:236: // Schedule a timeout in case the directory read doesn't complete nit: '.' at the end of comment
https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:61: void CancelAndDelete(); I think this can be private? https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:109: std::vector<base::FilePath> file_paths_; Do we still need this? Does it help keep the files from the canera in order? https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:169: content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI, I think you can PostTaskAndReplyWithResult here and get rid of |error|. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:330: // Note: This trampoline is used because Bind can't attach curried I thought you can do this in NotifyReadDir(): for (...) { fileapi::AsyncFileUtil::EntryList entry_list; for (...) { ... } content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(iter->second.first, entry_list, false)); https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:370: DCHECK(iter != read_file_transactions_.end()); These three lines can be written as: if (iter == read_file_transactions_.end()) { NOTREACHED(); return; } https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:382: base::PlatformFileInfo info = file_info_[path.value()]; Isn't path.value() just |name| ? https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:392: base::FilePath(device_location).BaseName().value(); nit: fits on the previous line. https://codereview.chromium.org/12255023/diff/32001/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_async_delegate.h (right): https://codereview.chromium.org/12255023/diff/32001/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_async_delegate.h:25: // Note: caller must ensure that the deleting call is not made while there are I don't think the caller to CancelPendingTasksAndDeleteDelegate() ensures this condition. Can't you just run all the error callbacks in |read_dir_transactions_| and |read_file_transactions_| when we are deleting the delegate?
https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:61: void CancelAndDelete(); On 2013/03/18 09:25:55, Lei Zhang wrote: > I think this can be private? Done. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:109: std::vector<base::FilePath> file_paths_; On 2013/03/18 09:25:55, Lei Zhang wrote: > Do we still need this? Does it help keep the files from the canera in order? Hmm. I think we could work without it, but when I switch to support hierarchical directories, something basically equivalent will come back. Shall I keep it for now? https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:169: content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI, On 2013/03/18 09:25:55, Lei Zhang wrote: > I think you can PostTaskAndReplyWithResult here and get rid of |error|. Yeah, I considered this, but thought it was more easily understood and less fragile to just pass both, since one needs ownership passed. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:228: read_dir_transactions_[root.value()] = On 2013/03/18 03:40:37, kinuko wrote: > What happens if we get parallel ReadDirectory requests for the same path? (And > if there's already an entry for the path we probably want to batch requests) I think I'm covering this now. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:231: if (received_all_files_) { On 2013/03/18 03:40:37, kinuko wrote: > Does this flag indicate it received all files (not only for the requested path) > from the device? ...It looks so. The mac image capture just sends all files, then a message that they're all done. There's no (documented) order guarantee, so we don't know any path is complete until we get this. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:236: // Schedule a timeout in case the directory read doesn't complete On 2013/03/18 03:40:37, kinuko wrote: > nit: '.' at the end of comment Done. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:330: // Note: This trampoline is used because Bind can't attach curried On 2013/03/18 09:25:55, Lei Zhang wrote: > I thought you can do this in NotifyReadDir(): > > for (...) { > fileapi::AsyncFileUtil::EntryList entry_list; > for (...) { ... } > content::BrowserThread::PostTask(content::BrowserThread::IO, > FROM_HERE, > base::Bind(iter->second.first, entry_list, false)); Yeah, you can. I'm doing it elsewhere, so I'm not sure what's up here. I probably misread an error. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:370: DCHECK(iter != read_file_transactions_.end()); On 2013/03/18 09:25:55, Lei Zhang wrote: > These three lines can be written as: > if (iter == read_file_transactions_.end()) { > NOTREACHED(); > return; > } Done. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:382: base::PlatformFileInfo info = file_info_[path.value()]; On 2013/03/18 09:25:55, Lei Zhang wrote: > Isn't path.value() just |name| ? switched this about https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:392: base::FilePath(device_location).BaseName().value(); On 2013/03/18 09:25:55, Lei Zhang wrote: > nit: fits on the previous line. Done. https://codereview.chromium.org/12255023/diff/32001/webkit/fileapi/media/mtp_... File webkit/fileapi/media/mtp_device_async_delegate.h (right): https://codereview.chromium.org/12255023/diff/32001/webkit/fileapi/media/mtp_... webkit/fileapi/media/mtp_device_async_delegate.h:25: // Note: caller must ensure that the deleting call is not made while there are Uggh. I wasn't sure about this. OK, I'll switch it up. On 2013/03/18 09:25:55, Lei Zhang wrote: > I don't think the caller to CancelPendingTasksAndDeleteDelegate() ensures this > condition. > > Can't you just run all the error callbacks in |read_dir_transactions_| and > |read_file_transactions_| when we are deleting the delegate?
https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:109: std::vector<base::FilePath> file_paths_; On 2013/03/20 23:43:55, Greg Billock wrote: > On 2013/03/18 09:25:55, Lei Zhang wrote: > > Do we still need this? Does it help keep the files from the canera in order? > > Hmm. I think we could work without it, but when I switch to support hierarchical > directories, something basically equivalent will come back. Shall I keep it for > now? Sure. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:228: read_dir_transactions_[root.value()] = On 2013/03/20 23:43:55, Greg Billock wrote: > On 2013/03/18 03:40:37, kinuko wrote: > > What happens if we get parallel ReadDirectory requests for the same path? (And > > if there's already an entry for the path we probably want to batch requests) > > I think I'm covering this now. Can you elaborate? If two calls to ReadDirectory() comes in, wouldn't the second call's callbacks overwrite the first's. https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:370: DCHECK(iter != read_file_transactions_.end()); On 2013/03/20 23:43:55, Greg Billock wrote: > On 2013/03/18 09:25:55, Lei Zhang wrote: > > These three lines can be written as: > > if (iter == read_file_transactions_.end()) { > > NOTREACHED(); > > return; > > } > > Done. Did you want to add a NOTREACHED() to retain the same meaning as the DCHECK() you had before?
https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:228: read_dir_transactions_[root.value()] = On 2013/03/21 22:34:58, Lei Zhang wrote: > On 2013/03/20 23:43:55, Greg Billock wrote: > > On 2013/03/18 03:40:37, kinuko wrote: > > > What happens if we get parallel ReadDirectory requests for the same path? > (And > > > if there's already an entry for the path we probably want to batch requests) > > > > I think I'm covering this now. > > Can you elaborate? If two calls to ReadDirectory() comes in, wouldn't the second > call's callbacks overwrite the first's. Oh, I see what you mean. I've been assuming the caller won't run duplicate calls. (The snapshot impl code makes the same assumption.) Shall I protect this? https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:370: DCHECK(iter != read_file_transactions_.end()); On 2013/03/21 22:34:58, Lei Zhang wrote: > On 2013/03/20 23:43:55, Greg Billock wrote: > > On 2013/03/18 09:25:55, Lei Zhang wrote: > > > These three lines can be written as: > > > if (iter == read_file_transactions_.end()) { > > > NOTREACHED(); > > > return; > > > } > > > > Done. > > Did you want to add a NOTREACHED() to retain the same meaning as the DCHECK() > you had before? I realized that since CancelDownloads, this is an expected code path that may be executed; there shouldn't have been a dcheck.
On 2013/03/25 16:33:25, Greg Billock wrote: > https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... > File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): > > https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... > chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:228: > read_dir_transactions_[root.value()] = > On 2013/03/21 22:34:58, Lei Zhang wrote: > > On 2013/03/20 23:43:55, Greg Billock wrote: > > > On 2013/03/18 03:40:37, kinuko wrote: > > > > What happens if we get parallel ReadDirectory requests for the same path? > > (And > > > > if there's already an entry for the path we probably want to batch > requests) > > > > > > I think I'm covering this now. > > > > Can you elaborate? If two calls to ReadDirectory() comes in, wouldn't the > second > > call's callbacks overwrite the first's. > > Oh, I see what you mean. I've been assuming the caller won't run duplicate > calls. (The snapshot impl code makes the same assumption.) Shall I protect this? I'm pretty sure it can happen. When I run the MGA test app on Linux against a tablet, the caller is issuing ReadDirectory calls much faster than they are being fulfilled. The pending calls on Linux go into a queue. I don't think there's anything preventing an app from issuing multiple calls for the same file.
On 2013/03/26 01:55:01, Lei Zhang wrote: > On 2013/03/25 16:33:25, Greg Billock wrote: > > > https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... > > File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm > (right): > > > > > https://codereview.chromium.org/12255023/diff/32001/chrome/browser/media_gall... > > chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:228: > > read_dir_transactions_[root.value()] = > > On 2013/03/21 22:34:58, Lei Zhang wrote: > > > On 2013/03/20 23:43:55, Greg Billock wrote: > > > > On 2013/03/18 03:40:37, kinuko wrote: > > > > > What happens if we get parallel ReadDirectory requests for the same > path? > > > (And > > > > > if there's already an entry for the path we probably want to batch > > requests) > > > > > > > > I think I'm covering this now. > > > > > > Can you elaborate? If two calls to ReadDirectory() comes in, wouldn't the > > second > > > call's callbacks overwrite the first's. > > > > Oh, I see what you mean. I've been assuming the caller won't run duplicate > > calls. (The snapshot impl code makes the same assumption.) Shall I protect > this? > > I'm pretty sure it can happen. When I run the MGA test app on Linux against a > tablet, the caller is issuing ReadDirectory calls much faster than they are > being fulfilled. The pending calls on Linux go into a queue. I don't think > there's anything preventing an app from issuing multiple calls for the same > file. Done
lgtm if you fix the std::list::erase() issue. https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:130: typedef std::list<ReadDirectoryRequest> ReadDirTransactionList; I thought you were going to make this a map of lists, so you don't have to traverse the entire list every time a read directory finishes. But this does make the Cancel operation easier. Ok with me either way, since it's not obvious how much impact this has. https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:254: read_dir_transactions_.erase(iter); This doesn't work. The moment you erase |iter|, you invalidate it. Instead, you need to write: for (ReadDirTransactionList::iterator iter = read_dir_transactions_.begin(); iter != read_dir_transactions_.end();) { ... iter = read_dir_transactions_.erase(iter); } https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:373: if (iter == read_file_transactions_.end()) { nit: no need for braces. https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:394: : directory(dir), nit: indent out 2 more spaces here and for the 2 lines below.
https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h (right): https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h:130: typedef std::list<ReadDirectoryRequest> ReadDirTransactionList; On 2013/04/02 01:28:18, Lei Zhang wrote: > I thought you were going to make this a map of lists, so you don't have to > traverse the entire list every time a read directory finishes. But this does > make the Cancel operation easier. Ok with me either way, since it's not obvious > how much impact this has. I typed that in, but t was pretty complicated. :-) I expect overlapped requests is an edge case at present -- we'll have to beef this up for non-flattened hierarchies, but let's deal with it then. https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:254: read_dir_transactions_.erase(iter); Oh, drat. You're right. Thanks. On 2013/04/02 01:28:18, Lei Zhang wrote: > This doesn't work. The moment you erase |iter|, you invalidate it. Instead, you > need to write: > > for (ReadDirTransactionList::iterator iter = read_dir_transactions_.begin(); > iter != read_dir_transactions_.end();) { > ... > iter = read_dir_transactions_.erase(iter); > } https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:373: if (iter == read_file_transactions_.end()) { On 2013/04/02 01:28:18, Lei Zhang wrote: > nit: no need for braces. Done. https://codereview.chromium.org/12255023/diff/63001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:394: : directory(dir), On 2013/04/02 01:28:18, Lei Zhang wrote: > nit: indent out 2 more spaces here and for the 2 lines below. Done.
https://codereview.chromium.org/12255023/diff/71001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/71001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:250: iter != read_dir_transactions_.end(); ++iter) { You still have the extra ++iter here.
https://codereview.chromium.org/12255023/diff/71001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/71001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:250: iter != read_dir_transactions_.end(); ++iter) { On 2013/04/03 00:05:02, Lei Zhang wrote: > You still have the extra ++iter here. Done, but I'm going to add a test for overlapped dir reads.
On 2013/04/03 00:19:27, Greg Billock wrote: > https://codereview.chromium.org/12255023/diff/71001/chrome/browser/media_gall... > File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): > > https://codereview.chromium.org/12255023/diff/71001/chrome/browser/media_gall... > chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:250: iter != > read_dir_transactions_.end(); ++iter) { > On 2013/04/03 00:05:02, Lei Zhang wrote: > > You still have the extra ++iter here. > > Done, but I'm going to add a test for overlapped dir reads. OK, added. Want to have a look?
lgtm https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:185: // Need a waitable event on io thread startup? remove? https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:193: delegate_ = new chrome::MTPDeviceDelegateImplMac("id", "/ic:id"); there's already a const for "id", and constify "/ic:id" ? https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:205: } nit: separate with newline https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:240: base::WaitableEvent wait(true, false); IWYU for WaitableEvent. https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:251: wait.Wait(); Can we just check IsSignaled() instead? If we already ran the message loop, the signal should have fired.
https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:185: // Need a waitable event on io thread startup? On 2013/04/04 03:46:21, Lei Zhang wrote: > remove? Done. https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:193: delegate_ = new chrome::MTPDeviceDelegateImplMac("id", "/ic:id"); On 2013/04/04 03:46:21, Lei Zhang wrote: > there's already a const for "id", and constify "/ic:id" ? Done. https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:205: } On 2013/04/04 03:46:21, Lei Zhang wrote: > nit: separate with newline Done. https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:240: base::WaitableEvent wait(true, false); On 2013/04/04 03:46:21, Lei Zhang wrote: > IWYU for WaitableEvent. It's there. https://codereview.chromium.org/12255023/diff/83001/chrome/browser/media_gall... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:251: wait.Wait(); On 2013/04/04 03:46:21, Lei Zhang wrote: > Can we just check IsSignaled() instead? If we already ran the message loop, the > signal should have fired. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/12255023/106002
Presubmit check for 12255023-106002 failed and returned exit status 1.
INFO:root:Found 4 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
webkit/fileapi/media/mtp_device_file_system_config.h
Presubmit checks took 1.5s to calculate.
On 2013/04/04 17:01:16, I haz the power (commit-bot) wrote: > Presubmit check for 12255023-106002 failed and returned exit status 1. > > INFO:root:Found 4 file(s). > > Running presubmit commit checks ... > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > webkit/fileapi/media/mtp_device_file_system_config.h > > Presubmit checks took 1.5s to calculate. Kinuko, can you look for webkit/fileapi owners?
lgtm https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:157: } nit: no need of { } for one-line body in chromium https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:182: content::BrowserThread::FILE, &message_loop_)); nit: indent https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:481: DownloadFile(base::FilePath("/ic:id/nonexist"), nit: indent
https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm (right): https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm:157: } On 2013/04/05 07:51:21, kinuko wrote: > nit: no need of { } for one-line body in chromium Done. https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... File chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm (right): https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:182: content::BrowserThread::FILE, &message_loop_)); On 2013/04/05 07:51:21, kinuko wrote: > nit: indent Done. https://codereview.chromium.org/12255023/diff/106002/chrome/browser/media_gal... chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm:481: DownloadFile(base::FilePath("/ic:id/nonexist"), On 2013/04/05 07:51:21, kinuko wrote: > nit: indent Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/12255023/118001
Message was sent while issue was closed.
Change committed as 192631 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
