Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251643003:
Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl (Closed)
|
Created:
4 years, 4 months ago by ananta Modified:
4 years, 3 months ago CC:
chromium-reviews, asanka, jam, rginda+watch_chromium.org, darin-cc_chromium.org, loading-reviews_chromium.org, svaldez, Łukasz Anforowicz Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl
This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h
not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload
methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the
old BeginSaveFile and BeginDownload methods. These callers directly call the new
ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal).
Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before
BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure.
I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function
is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the
DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out
to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor.
I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function.
Did not want to break those tests in this patchset.
Added TODO's in the code
However the RDH interface is still a work in progress. There are other annoyances in
ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and
BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch.
BUG=598073
TBR=jam
Committed: https://crrev.com/c82dd5be1176dd5c8220f7dfd43afb369a585215
Cr-Commit-Position: refs/heads/master@{#414614}
Patch Set 1 #Patch Set 2 : Fix comments #Patch Set 3 : Fix compile errors #Patch Set 4 : Fix content_unittests compile failures. The BeginDownloadHelper function is now a static public fun… #
Total comments: 7
Patch Set 5 : Fix bot redness and add DCHECKs for thread id. #Patch Set 6 : Replace typedef with using #
Total comments: 36
Patch Set 7 : Address review comments and bring back the BeginRequestInternal function #Patch Set 8 : Fix compile failures and make the CreateRequestInfo function private again. #Patch Set 9 : Fix compile failures and make the CreateRequestInfo function private again. #Patch Set 10 : Fix compile failures and make the CreateRequestInfo function private again. #Patch Set 11 : Fix test crashes by invoking the ResourceDispatcherHostDelegate::DownloadStarting function after we… #Patch Set 12 : git cl format #Patch Set 13 : Fix browser test redness by ensuring that the ResourceDispatcherHostDelegate is notified in CreateR… #
Total comments: 8
Patch Set 14 : Address review comments #Patch Set 15 : Initialize the DownloadTabInfo structure in DownloadResourceHandler::OnResponseStarted to ensure th… #Patch Set 16 : Initialize tab_info_ in OnStart instead of OnResponseStart because we need a central place where al… #
Total comments: 16
Patch Set 17 : Next round of review comments #Patch Set 18 : Move the download tests to download_manager_impl_unittest.cc #Patch Set 19 : Remove download code from resource_dispatcher_host_unittest.cc #Patch Set 20 : Fix bot redness in ResourceDispatcherHostTests by notifying the DownloadManagerImpl about ResourceD… #Patch Set 21 : Added a function to Initialize the URLRequest in RDHI. #Patch Set 22 : Remove unnecessary function declaration #
Total comments: 12
Patch Set 23 : Move the blob code to RDHI::BeginURLRequest #
Total comments: 18
Patch Set 24 : Address next round of review comments and keep the download test in resource_dispatcher_host_unitte… #Depends on Patchset: Messages
Total messages: 136 (82 generated)
Description was changed from ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function CreateResourceHandlerForDownload which is registered with ResourceDispatcherHostImpl via the new ResourceDispatcherHostImpl::RegisterCreateDownloadHandlerInterceptor function. I agree that this is a touch ugly. However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 ========== to ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function CreateResourceHandlerForDownload which is registered with ResourceDispatcherHostImpl via the new ResourceDispatcherHostImpl::RegisterCreateDownloadHandlerInterceptor function. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 ==========
ananta@chromium.org changed reviewers: + scottmg@chromium.org
Please take an initial glance before I add other reviewers.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
Quick drive by review. I'll try to come back and do a more thorough review later today. +svaldez, lukasz FTI. https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... content/browser/download/download_manager_impl.cc:68: bool must_download) { I believe this function needs to be called on the IO thread; willing to add a DCHECK() to that effect? (Also for the other functions in this file.) (The download code runs on four different threads, so I'm a bit paranoid about being very clear what thread code is supposed to be called on.) https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest( If this is the only place this function is called (which fits my memory and my quick scan of this CL), can it be moved to a file static function in this file? Or even just hoist the guts into this function? I don't have any major objections to a static function on DMI that's called on the IO thread (the class lives on the UI thread) but I think if it doesn't need to be on DMI it shouldn't be. https://codereview.chromium.org/2251643003/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:83: CreateDownloadHandlerIntercept; nit: I think "using X = " is the new hotness?
(Also: Yay for doing this! I very much approve of the goal, I just know how easy it is to make download code uglier and how hard it is to make it cleaner, so I want someone who knows the download flow to give it a once over before it lands.)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... content/browser/download/download_manager_impl.cc:68: bool must_download) { On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > I believe this function needs to be called on the IO thread; willing to add a > DCHECK() to that effect? (Also for the other functions in this file.) > > (The download code runs on four different threads, so I'm a bit paranoid about > being very clear what thread code is supposed to be called on.) Added DCHECK's
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest( On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > If this is the only place this function is called (which fits my memory and my > quick scan of this CL), can it be moved to a file static function in this file? > Or even just hoist the guts into this function? I don't have any major > objections to a static function on DMI that's called on the IO thread (the class > lives on the UI thread) but I think if it doesn't need to be on DMI it shouldn't > be. This function is also called from resource_dispatcher_host_unittest.cc in the download tests. I agree with the static function ugliness. However for this use case we need a way for the function to be accessible outside this file. https://codereview.chromium.org/2251643003/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:83: CreateDownloadHandlerIntercept; On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > nit: I think "using X = " is the new hotness? Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest( On 2016/08/18 01:53:25, ananta wrote: > On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > > If this is the only place this function is called (which fits my memory and my > > quick scan of this CL), can it be moved to a file static function in this > file? > > Or even just hoist the guts into this function? I don't have any major > > objections to a static function on DMI that's called on the IO thread (the > class > > lives on the UI thread) but I think if it doesn't need to be on DMI it > shouldn't > > be. > > This function is also called from resource_dispatcher_host_unittest.cc in the > download tests. I agree with the static function ugliness. However for this use > case we need a way for the function to be accessible outside this file. Given that the function is no longer in rdhi.cc, shouldn't the unit test for it be moved out of rdhu.cc? (I'm not sure how the unit test affects making it static to this file, I'll look at that more closely in the next round of review.) The merging of the two functions is probably reasonable to leave to the downloads folks if it requires test re-evaluations, but moving the test makes sense to me?
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Driveby comments https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:655: return std::unique_ptr<ResourceHandler>(); Returning a nullptr looks like it will result in a crash. Maybe we should just not let create_download_handler_intercept_ be null at this point? If we're going to crash anyways, we don't get anything by the extra logic here. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1514: request_data.referrer_policy), Moving where we set the referrer seems concerning - we've already passed the URLRequest to BlobProtocolHandler, ServiceWorkerRequestHandler, ForeignFetchRequestHandler, AppCacheInterceptor, CrossSiteResourceHandler, and all the standard handlers at this point. Some certainly rely on first_party_for_cookies. Not sure if any depend on referrer or referrer policy, but it seems like a bad idea to set anything on the request after we've already let the world have a looksee at it.
https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:43: #include "content/browser/download/save_file_resource_handler.h" And this one. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (left): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:27: #include "content/browser/download/download_resource_handler.h" Remove the DEPS entry in c/b/loader for this file.
rdsmith@chromium.org changed reviewers: - mmenke@chromium.org
More detailed review. The biggest issue is that I'm not sure I'm ok with the handling of CreateRequestHandlerForDownload (I know you want to look for something better to do in future CLs, but this one doesn't strike me as being a step in the right direction). I remember seeing something about why we're not pushing stuff into the ResourceDispatcherHostDelegate, but it wasn't on this CL and I didn't retain it--remind me of the issues around that? https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { I don't think it makes sense to move this call and the below delegate call out of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you move CreateResourceHandlerForDownload out you should also move these methods off ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate (though that's problematic because the DownloadManager is a UI thread object *and* it's not clear the RDHI can figure out the correct DownloadManager at the point this function is called). All of which brings me back to "Why can't we just push all this logic into the ResourceDispatcherHostDelegate?" https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:576: if (ResourceDispatcherHostImpl::Get()->is_shutdown()) Can this be pulled down into RDHI::BeginURLRequest, maybe by making that function have a return value? That would avoid accessing what's arguably a variable that should be local to RDHI. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:604: resource_context); Can these simply be added to the interface for BeginURLRequest? ResourceRequestInfoImpl usage is currently basically restricted to content/browser/loader, and widening its exposure seems like something to avoid if we can. Just the fact that it's an Impl class suggests that :-J. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:623: url_request.get(), is_content_initiated, true)); It feels broken for download code to delegate creation of the download resource handler to another class which just calls back into a function in this file in order to pass the created handler into BeginURLRequest. Would you add a TODO comment to clean this up? If we leave CreateResourceHandlerForDownload() in this file (which I still think is a mistake) I'd vote in favor of just calling it directly. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/save_file_manager.cc:302: return; Save question as previous--can this be lowered into BeginURLRequest? https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/save_file_manager.cc:327: extra_info->AssociateWithRequest(request.get()); // Request takes ownership. Same question as elsewhere: Can we pass this into BeginURLRequest and avoid access to RRII outside of content/browser/loader? https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:651: return DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST; This check seems to have been removed in the refactoring. Why? https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:655: return std::unique_ptr<ResourceHandler>(); I'm uncomfortable with this change. Either we're in a situation where this "shouldn't happen" (in which case let's have a DCHECK() and document in the header that the interceptor must be set before any requests can be handled) or if might happen, in which case I think we need to handle it better. Unless you're convinced that all the functions that call CreateResourceHandlerForDownload() can handle a null resource handler? If they can't, then what we're doing is moving the failure from a place where it's relatively clear what's happening (here) to one where it'll be less clear. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:312: CreateDownloadHandlerIntercept intercept); Why can't we handle this through the current ResourceDispatcherHostDelegate? I remember seeing something go by about this, but it was on another CL I wasn't reviewing; remind me or give me a pointer? My thoughts about the overall architectural question of downloads vs. content/browser/loader: I think the MimeTypeResourceHandler needs to have some conceptual knowledge of downloads, because of the attachment Content-Disposition. (It also knows about downloads as a "default handler" for things it can't figure out any other way to handle, which might be a useful conceptual shift, but isn't worth worrying about given the attachment content disposition.) However, MimeTypeResourceHandlers come and go and the RDHI stays around, so at least at first glance it makes sense to me to go through RDHI for the knowledge of what to do with downloads (i.e. how to create a DownloadResourceHandler). And given the desire to separate content/browser/loader from the rest of content, some kind of callout makes sense. But I don't yet understand why we can't do that through the existing delegate.
On 2016/08/18 15:33:57, Randy Smith - Not in Fridays wrote: > https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... > File content/browser/download/download_manager_impl.cc (right): > > https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... > content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason > reason = DownloadManagerImpl::BeginDownloadRequest( > On 2016/08/18 01:53:25, ananta wrote: > > On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > > > If this is the only place this function is called (which fits my memory and > my > > > quick scan of this CL), can it be moved to a file static function in this > > file? > > > Or even just hoist the guts into this function? I don't have any major > > > objections to a static function on DMI that's called on the IO thread (the > > class > > > lives on the UI thread) but I think if it doesn't need to be on DMI it > > shouldn't > > > be. > > > > This function is also called from resource_dispatcher_host_unittest.cc in the > > download tests. I agree with the static function ugliness. However for this > use > > case we need a way for the function to be accessible outside this file. > > Given that the function is no longer in rdhi.cc, shouldn't the unit test for it > be moved out of rdhu.cc? (I'm not sure how the unit test affects making it > static to this file, I'll look at that more closely in the next round of > review.) > > The merging of the two functions is probably reasonable to leave to the > downloads folks if it requires test re-evaluations, but moving the test makes > sense to me? I have added a TODO in the test to move it out in a later patchset if that is ok with you. We can move the method out of the DownloadManagerImpl class then.
https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > I don't think it makes sense to move this call and the below delegate call out > of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you move > CreateResourceHandlerForDownload out you should also move these methods off > ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate (though > that's problematic because the DownloadManager is a UI thread object *and* it's > not clear the RDHI can figure out the correct DownloadManager at the point this > function is called). All of which brings me back to "Why can't we just push all > this logic into the ResourceDispatcherHostDelegate?" Please correct me if I am wrong here. The ResourceDispatcherHostDelegate takes on a variety of forms, one of them being in Chrome. The RDH delegate mechanism allows content to get into the embedder (chrome). The embedder can only get to content via public interfaces. Till now downloads, save file requests were fully handled in content. I don't see how we can push the CreateResourceHandlerForDownload onto the delegate. While the interceptor approach is hacky at best, it is not intended by a live for ever thing. Once we hash out how the public interface to RDH is going to look like after it moves out of content, we can revisit this. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:576: if (ResourceDispatcherHostImpl::Get()->is_shutdown()) On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > Can this be pulled down into RDHI::BeginURLRequest, maybe by making that > function have a return value? That would avoid accessing what's arguably a > variable that should be local to RDHI. Done. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:604: resource_context); On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > Can these simply be added to the interface for BeginURLRequest? > ResourceRequestInfoImpl usage is currently basically restricted to > content/browser/loader, and widening its exposure seems like something to avoid > if we can. Just the fact that it's an Impl class suggests that :-J. Done. I left the core of the logic in BeginURLRequest in the old BeginRequestInternal function which has come back. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:623: url_request.get(), is_content_initiated, true)); On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > It feels broken for download code to delegate creation of the download resource > handler to another class which just calls back into a function in this file in > order to pass the created handler into BeginURLRequest. Would you add a TODO > comment to clean this up? If we leave CreateResourceHandlerForDownload() in > this file (which I still think is a mistake) I'd vote in favor of just calling > it directly. Added a TODO here. The CreateResourceHandlerForDownload function in this file only creates the download handler. The rest of the logic of notifying the delegate and creating a throttled handler etc has moved back to the caller in ResourceDispatcherHostImpl. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/save_file_manager.cc:302: return; On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > Save question as previous--can this be lowered into BeginURLRequest? Done. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/save_file_manager.cc:327: extra_info->AssociateWithRequest(request.get()); // Request takes ownership. On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > Same question as elsewhere: Can we pass this into BeginURLRequest and avoid > access to RRII outside of content/browser/loader? Done. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:43: #include "content/browser/download/save_file_resource_handler.h" On 2016/08/18 17:38:08, scottmg wrote: > And this one. Done. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:651: return DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST; On 2016/08/18 17:38:58, Randy Smith - Not in Fridays wrote: > This check seems to have been removed in the refactoring. Why? Thanks for catching this. Fixed https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:655: return std::unique_ptr<ResourceHandler>(); On 2016/08/18 17:35:43, mmenke wrote: > Returning a nullptr looks like it will result in a crash. Maybe we should just > not let create_download_handler_intercept_ be null at this point? If we're > going to crash anyways, we don't get anything by the extra logic here. Sure thing. Added a DCHECK here https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:655: return std::unique_ptr<ResourceHandler>(); On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > I'm uncomfortable with this change. Either we're in a situation where this > "shouldn't happen" (in which case let's have a DCHECK() and document in the > header that the interceptor must be set before any requests can be handled) or > if might happen, in which case I think we need to handle it better. Unless > you're convinced that all the functions that call > CreateResourceHandlerForDownload() can handle a null resource handler? If they > can't, then what we're doing is moving the failure from a place where it's > relatively clear what's happening (here) to one where it'll be less clear. Added a DCHECK here https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1514: request_data.referrer_policy), On 2016/08/18 17:35:43, mmenke wrote: > Moving where we set the referrer seems concerning - we've already passed the > URLRequest to BlobProtocolHandler, ServiceWorkerRequestHandler, > ForeignFetchRequestHandler, AppCacheInterceptor, CrossSiteResourceHandler, and > all the standard handlers at this point. Some certainly rely on > first_party_for_cookies. Not sure if any depend on referrer or referrer policy, > but it seems like a bad idea to set anything on the request after we've already > let the world have a looksee at it. Thanks. Fixed this by bringing back the old BeginRequestInternal function which is called elsewhere in this file and from the new BeginURLRequest function. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (left): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:27: #include "content/browser/download/download_resource_handler.h" On 2016/08/18 17:38:08, scottmg wrote: > Remove the DEPS entry in c/b/loader for this file. Done. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:312: CreateDownloadHandlerIntercept intercept); On 2016/08/18 17:38:58, Randy Smith - Not in Fridays wrote: > Why can't we handle this through the current ResourceDispatcherHostDelegate? I > remember seeing something go by about this, but it was on another CL I wasn't > reviewing; remind me or give me a pointer? > > My thoughts about the overall architectural question of downloads vs. > content/browser/loader: I think the MimeTypeResourceHandler needs to have some > conceptual knowledge of downloads, because of the attachment > Content-Disposition. (It also knows about downloads as a "default handler" for > things it can't figure out any other way to handle, which might be a useful > conceptual shift, but isn't worth worrying about given the attachment content > disposition.) However, MimeTypeResourceHandlers come and go and the RDHI stays > around, so at least at first glance it makes sense to me to go through RDHI for > the knowledge of what to do with downloads (i.e. how to create a > DownloadResourceHandler). And given the desire to separate > content/browser/loader from the rest of content, some kind of callout makes > sense. But I don't yet understand why we can't do that through the existing > delegate. The RDH delegate lives in multiple forms, one of them in Chrome. https://cs.chromium.org/chromium/src/chrome/browser/renderer_host/chrome_reso... I am not sure pushing the creation onto the delegate is correct in this context. I have added a TODO here and places where it is used to look into this, once we are ready to move the RDH and loader code out of content.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
svaldez@chromium.org changed reviewers: + svaldez@chromium.org
Some comments from a quick pass. https://codereview.chromium.org/2251643003/diff/240001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/browse... content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); Consider migrating or copying this comment to the download_manager_impl function. https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... content/browser/download/download_resource_handler.cc:136: // Do UI thread initialization for tab_info_ asap after Comment should be updated. https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... content/browser/download/save_file_manager.cc:303: if (!known_proto) { Combine these two lines. https://codereview.chromium.org/2251643003/diff/240001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2297: if (is_shutdown()) Why does this not just use is_shutdown_?
https://codereview.chromium.org/2251643003/diff/240001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/browse... content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); On 2016/08/19 17:05:14, svaldez wrote: > Consider migrating or copying this comment to the download_manager_impl > function. Done. https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... content/browser/download/download_resource_handler.cc:136: // Do UI thread initialization for tab_info_ asap after On 2016/08/19 17:05:14, svaldez wrote: > Comment should be updated. Done. https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/downlo... content/browser/download/save_file_manager.cc:303: if (!known_proto) { On 2016/08/19 17:05:14, svaldez wrote: > Combine these two lines. Done. https://codereview.chromium.org/2251643003/diff/240001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2297: if (is_shutdown()) On 2016/08/19 17:05:14, svaldez wrote: > Why does this not just use is_shutdown_? Done.
I moved the PostTask for the InitializeDownloadTabInfoOnUIThread function to the DownloadResourceHandler::OnResponseStarted function. OnWillStarted does not get called in all cases. Also changed the code to allocate the DownloadTabInfo pointer just before the PostTask. Verified with a debug run of downloading multiple files in the browser and save as. Works correctly.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/08/19 22:30:43, ananta wrote: > I moved the PostTask for the InitializeDownloadTabInfoOnUIThread function to > the DownloadResourceHandler::OnResponseStarted function. OnWillStarted does not > get called in all cases. Also changed the code to allocate the DownloadTabInfo > pointer just before the PostTask. > > Verified with a debug run of downloading multiple files in the browser and save > as. Works correctly. Please scratch the previous comment. I moved the code to initalize the tab_info_ in OnStart() and updated the comments for the tasks here. OnStart() always gets invoked regardless of success or failure.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/18 21:50:21, ananta wrote: > On 2016/08/18 15:33:57, Randy Smith - Not in Fridays wrote: > > > https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... > > File content/browser/download/download_manager_impl.cc (right): > > > > > https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... > > content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason > > reason = DownloadManagerImpl::BeginDownloadRequest( > > On 2016/08/18 01:53:25, ananta wrote: > > > On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > > > > If this is the only place this function is called (which fits my memory > and > > my > > > > quick scan of this CL), can it be moved to a file static function in this > > > file? > > > > Or even just hoist the guts into this function? I don't have any major > > > > objections to a static function on DMI that's called on the IO thread (the > > > class > > > > lives on the UI thread) but I think if it doesn't need to be on DMI it > > > shouldn't > > > > be. > > > > > > This function is also called from resource_dispatcher_host_unittest.cc in > the > > > download tests. I agree with the static function ugliness. However for this > > use > > > case we need a way for the function to be accessible outside this file. > > > > Given that the function is no longer in rdhi.cc, shouldn't the unit test for > it > > be moved out of rdhu.cc? (I'm not sure how the unit test affects making it > > static to this file, I'll look at that more closely in the next round of > > review.) > > > > The merging of the two functions is probably reasonable to leave to the > > downloads folks if it requires test re-evaluations, but moving the test makes > > sense to me? > > I have added a TODO in the test to move it out in a later patchset if that is ok > with you. > We can move the method out of the DownloadManagerImpl class then. Help me understand why it doesn't belong in this CL? I'm on board with the goal of making CLs as small as possible for review, maintenance, and revert/reland granularity purposes, but I also want CLs to atomically move the tree from/to clean states. Having a function in one file and it's unit test in the unit test grouping for a different files seems wrong. What am I missing? (Having said that, you probably want to wait until we've settled the various architectural issues before moving it, if you do--let's make sure it won't be wasted work.)
Next round of comments. I think we're moving closer to a meeting of minds around the basic architectural issue; I certainly see why we can't use the existing delegate. Still exploring alternatives, though :-}. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/18 22:27:09, ananta wrote: > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > I don't think it makes sense to move this call and the below delegate call out > > of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you move > > CreateResourceHandlerForDownload out you should also move these methods off > > ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate (though > > that's problematic because the DownloadManager is a UI thread object *and* > it's > > not clear the RDHI can figure out the correct DownloadManager at the point > this > > function is called). All of which brings me back to "Why can't we just push > all > > this logic into the ResourceDispatcherHostDelegate?" > > Please correct me if I am wrong here. The ResourceDispatcherHostDelegate takes > on > a variety of forms, one of them being in Chrome. The RDH delegate mechanism > allows content to get into the embedder (chrome). The embedder can only get to > content via public interfaces. Till now downloads, save file requests were fully > handled in content. I don't see how we can push the > CreateResourceHandlerForDownload onto the > delegate. While the interceptor approach is hacky at best, it is not intended by > a live for ever thing. Once we hash out how the public interface to RDH is going > to look like after it moves out of content, we can revisit this. > > > No, that's right. Another way of putting this is that ResourceHandler isn't a class that's visible to Chrome, so Chrome can't create an instance of it. Sorry I missed this on my first pass. But I'd still like to have a clear sense of the overall plan in this space. As I understand it, the short term goal that this CL is driving towards is moving content/browser/loader out of content and into a component, with the goal beyond that one being to wrap content/browser/loader in a mojo interface. Is your thought that we'll maintain the interceptor registration architecture through the componentization of content/browser/loader, and then change to something else for the mojo wrapping? If so, could you give me a sense as to what the something else would look like? It seems as if ResourceHandlers are going to stay a core part of the content/browser/loader architecture, so I'd think that (after mojoization) anything that created a ResourceHandler would have to be *inside* content/browser/loader. So that would argue for pulling the information about downloads and the download resource handler into content/browser/loader, and finding some other place within the downloads system to make the architectural cut. Do you have a different image of what mojoization might look like? ("The interceptor is what is planned for componentization and I don't yet know what we'll do for mojoization" is a fine response, I just want to make sure that's the status rather than assuming it.) https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:576: if (ResourceDispatcherHostImpl::Get()->is_shutdown()) On 2016/08/18 22:27:09, ananta wrote: > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > Can this be pulled down into RDHI::BeginURLRequest, maybe by making that > > function have a return value? That would avoid accessing what's arguably a > > variable that should be local to RDHI. > > Done. I apologize, but I'd like to take this request back. I'd rather have a "is_shutdown()" boolean on RDHI than create a new type to be filled in with a single enum (shutdown). I had imagined we could return a boolean from the BeginURLRequest function, but an actual whole enum I think is uglier than just having this accessor. Sorry/thanks ... (Keeping RRII inside of RDHI strikes me as goodness I'd like to keep, but I think that's orthogonal to whether BeginURLRequest returns an enum.) https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1514: request_data.referrer_policy), On 2016/08/18 22:27:09, ananta wrote: > On 2016/08/18 17:35:43, mmenke wrote: > > Moving where we set the referrer seems concerning - we've already passed the > > URLRequest to BlobProtocolHandler, ServiceWorkerRequestHandler, > > ForeignFetchRequestHandler, AppCacheInterceptor, CrossSiteResourceHandler, and > > all the standard handlers at this point. Some certainly rely on > > first_party_for_cookies. Not sure if any depend on referrer or referrer > policy, > > but it seems like a bad idea to set anything on the request after we've > already > > let the world have a looksee at it. > > Thanks. Fixed this by bringing back the old BeginRequestInternal function which > is called elsewhere in this file and from the new BeginURLRequest function. This looks to me to still have been changed on the BeginDownloadRequest path. In the old code, BeginDownload() set the referrer at the beginning of the function and then called into the BlobStorageContext and protocol handler with the request. Now, DownloadManager::BeginDownloadRequest doesn't set the referrer but still has the calls into the BlobStorageContext & etc. Am I missing something? (Thanks for catching this, Matt.) https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); Why not change the RDHI constructor rather than register an interceptor? We're putting in a DCHECK that basically says that the interceptor must always be there, and passing it into the constructor seems a slightly better way to enforce that than a DCHECK. It means this code needs to know the name and signature of the function used to create the resource handler, but that's basically |ResourceHandler *func(URLRequest*)|, which doesn't seem too bad to make visible here. I'll note that I've convinced myself in reading this CL that a) content/browser/loader *does* need to have knowledge of downloads, and that b) while we could find a way to restrict that knowledge to the resource handlers in c/b/l and keep it out of RDHI, RDHI is the only obvious place to store any information long-term; the other objects are not persistent. So I think RDHI is likely to need to have knowledge of downloads long-term. I.e. something in this space is likely to be what we do long-term, and we may as well make it as clean as possible. Please let me know if you see flaws in this logic; it's not directly relevant to this CL, but it will affect how I review it (in the whole "I want to see where we're going to make sure the individual steps move us in that direction" sense). https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); I have a slight preference for this reference being to DownloadResourceHandler instead of DownloadManager{Impl}; DRH is an IO thread object, and DMI a UI thread object, and the RDH is much closer to DRH architecturally than it is to DMI. DMI is what represents the download system to the higher levels, and RDH is pretty low level. Having said that, I don't feel strongly about it--if you really want the reference to be to DMI, I'll accept it (and maybe circle back with Asanka and jam@ after Asanka gets back). https://codereview.chromium.org/2251643003/diff/300001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/downlo... content/browser/download/download_resource_handler.cc:192: // tasks. I'm sorry, I don't understand what drove moving this. Was there a problem with where it was? The problem with moving it is that it gives more time for the user (or malicious code) to navigate the tab away and thus make the tab_info_ invalid/unuseful. So it's better to get the information as quickly as possible when the download starts (i.e. triggered from the constructor). The "This is safe" in the original comment was about why it was ok to pass a raw pointer to a member variable of an object that lives on the IO thread to a different thread (i.e. why that raw pointer would stay valid no matter what thread races occurred). It wasn't about why navigating away was safe; navigating away will just result in a null web_contents, which will result in the tab information not being set. https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2303: ResourceDispatcherHostImpl::Get()->CreateRequestInfo( Why not just call CreateRequestInfo directly? https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2315: is_content_initiated, true); Won't this already have been called? If the request is a download, then at some point (before or after this) the download resource handler must have been put on the chain, and if it was, HandleDownloadStarted() was called in CreateResourceHandlerForDownload(). What am I missing? https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:320: bool is_shutdown() const { return is_shutdown_; } I don't think this function is needed anymore?
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Oh, and don't wait for a signoff from me on this review - I defer to rdsmith. https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:297: enum URLRequestStatus { Given that we already have a net::URLRequestState class that the loader/ directory uses extensively, I don't think we want an enum by that same name. Do we even need an enum, or could we just return a bool indicating if starting the request suceeded or failed? If it failed for some other reason, I'd prefer we go through the normal failure path, anyways (Create a request and then fail it with an error). Shutdown is a bit magical, in that creating a request at that point in time may be problematic.
https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > On 2016/08/18 22:27:09, ananta wrote: > > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > > I don't think it makes sense to move this call and the below delegate call > out > > > of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you > move > > > CreateResourceHandlerForDownload out you should also move these methods off > > > ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate (though > > > that's problematic because the DownloadManager is a UI thread object *and* > > it's > > > not clear the RDHI can figure out the correct DownloadManager at the point > > this > > > function is called). All of which brings me back to "Why can't we just push > > all > > > this logic into the ResourceDispatcherHostDelegate?" > > > > Please correct me if I am wrong here. The ResourceDispatcherHostDelegate takes > > on > > a variety of forms, one of them being in Chrome. The RDH delegate mechanism > > allows content to get into the embedder (chrome). The embedder can only get to > > content via public interfaces. Till now downloads, save file requests were > fully > > handled in content. I don't see how we can push the > > CreateResourceHandlerForDownload onto the > > delegate. While the interceptor approach is hacky at best, it is not intended > by > > a live for ever thing. Once we hash out how the public interface to RDH is > going > > to look like after it moves out of content, we can revisit this. > > > > > > > > No, that's right. Another way of putting this is that ResourceHandler isn't a > class that's visible to Chrome, so Chrome can't create an instance of it. Sorry > I missed this on my first pass. > > But I'd still like to have a clear sense of the overall plan in this space. As > I understand it, the short term goal that this CL is driving towards is moving > content/browser/loader out of content and into a component, with the goal beyond > that one being to wrap content/browser/loader in a mojo interface. Is your > thought that we'll maintain the interceptor registration architecture through > the componentization of content/browser/loader, and then change to something > else for the mojo wrapping? If so, could you give me a sense as to what the > something else would look like? > > It seems as if ResourceHandlers are going to stay a core part of the > content/browser/loader architecture, so I'd think that (after mojoization) > anything that created a ResourceHandler would have to be *inside* > content/browser/loader. So that would argue for pulling the information about > downloads and the download resource handler into content/browser/loader, and > finding some other place within the downloads system to make the architectural > cut. Do you have a different image of what mojoization might look like? > > ("The interceptor is what is planned for componentization and I don't yet know > what we'll do for mojoization" is a fine response, I just want to make sure > that's the status rather than assuming it.) It does seem plausible that we may want to have DownloadResourceHandler or a variant of it live inside the content/browser/loader folder. The download_resource_handler.h/.cc files pull in other files from the content/browser/download folder. If it is ok with you we can do this in a second pass once we have the code in content/browser/loader ready to be moved out. In any case the interface of RDHI or whatever we chose to call it once it moves it is by no means ironed out. The interceptor is just an initial step to help us move the loader code out. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:576: if (ResourceDispatcherHostImpl::Get()->is_shutdown()) On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > On 2016/08/18 22:27:09, ananta wrote: > > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > > Can this be pulled down into RDHI::BeginURLRequest, maybe by making that > > > function have a return value? That would avoid accessing what's arguably a > > > variable that should be local to RDHI. > > > > Done. > > I apologize, but I'd like to take this request back. I'd rather have a > "is_shutdown()" boolean on RDHI than create a new type to be filled in with a > single enum (shutdown). I had imagined we could return a boolean from the > BeginURLRequest function, but an actual whole enum I think is uglier than just > having this accessor. Sorry/thanks ... > > (Keeping RRII inside of RDHI strikes me as goodness I'd like to keep, but I > think that's orthogonal to whether BeginURLRequest returns an enum.) Sure thing. Reverted to BeginURLRequest being a void function. https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1514: request_data.referrer_policy), On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > On 2016/08/18 22:27:09, ananta wrote: > > On 2016/08/18 17:35:43, mmenke wrote: > > > Moving where we set the referrer seems concerning - we've already passed the > > > URLRequest to BlobProtocolHandler, ServiceWorkerRequestHandler, > > > ForeignFetchRequestHandler, AppCacheInterceptor, CrossSiteResourceHandler, > and > > > all the standard handlers at this point. Some certainly rely on > > > first_party_for_cookies. Not sure if any depend on referrer or referrer > > policy, > > > but it seems like a bad idea to set anything on the request after we've > > already > > > let the world have a looksee at it. > > > > Thanks. Fixed this by bringing back the old BeginRequestInternal function > which > > is called elsewhere in this file and from the new BeginURLRequest function. > > This looks to me to still have been changed on the BeginDownloadRequest path. > In the old code, BeginDownload() set the referrer at the beginning of the > function and then called into the BlobStorageContext and protocol handler with > the request. Now, DownloadManager::BeginDownloadRequest doesn't set the > referrer but still has the calls into the BlobStorageContext & etc. Am I > missing something? > > (Thanks for catching this, Matt.) Yes this has changed. The current code for context is here: https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch... GetRequestBlobDataHandle does this https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_request_jo... SetRequestedBlobDataHandle does this https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_request_jo... The code is not inspecting the referrer. I think it should be safe? https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > I have a slight preference for this reference being to DownloadResourceHandler > instead of DownloadManager{Impl}; DRH is an IO thread object, and DMI a UI > thread object, and the RDH is much closer to DRH architecturally than it is to > DMI. DMI is what represents the download system to the higher levels, and RDH > is pretty low level. > > Having said that, I don't feel strongly about it--if you really want the > reference to be to DMI, I'll accept it (and maybe circle back with Asanka and > jam@ after Asanka gets back). This is a temporary change to get us to the end goal of moving the loader code out. I think your points about having the DownloadResourceHandler live in the loader folder may be what we end up doing anyways once we are at that point. We can leave this as is for now? https://codereview.chromium.org/2251643003/diff/300001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/downlo... content/browser/download/download_resource_handler.cc:192: // tasks. On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > I'm sorry, I don't understand what drove moving this. Was there a problem with > where it was? > > The problem with moving it is that it gives more time for the user (or malicious > code) to navigate the tab away and thus make the tab_info_ invalid/unuseful. So > it's better to get the information as quickly as possible when the download > starts (i.e. triggered from the constructor). > > The "This is safe" in the original comment was about why it was ok to pass a raw > pointer to a member variable of an object that lives on the IO thread to a > different thread (i.e. why that raw pointer would stay valid no matter what > thread races occurred). It wasn't about why navigating away was safe; > navigating away will just result in a null web_contents, which will result in > the tab information not being set. The ResourceRequestInfoImpl instance which is associated with the original request is not available here in all cases. For normal downloads which are detected via the mime type, yes. However for downloads initiated via the context menu for e.g. Save As, or Save image as, etc the request is initiated eventually via the DownloadUrl function in download_manager_impl.cc. OnStart gets called in all cases, success/failure and hence seemed like the right place to do this. https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2303: ResourceDispatcherHostImpl::Get()->CreateRequestInfo( On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > Why not just call CreateRequestInfo directly? Done. https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2315: is_content_initiated, true); On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > Won't this already have been called? If the request is a download, then at some > point (before or after this) the download resource handler must have been put on > the chain, and if it was, HandleDownloadStarted() was called in > CreateResourceHandlerForDownload(). What am I missing? The DownloadResourceHandler is created and is put on the chain for normal downloads. However it is not for downloads initiated via Save As for e.g. https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:297: enum URLRequestStatus { On 2016/08/22 14:40:08, mmenke wrote: > Given that we already have a net::URLRequestState class that the loader/ > directory uses extensively, I don't think we want an enum by that same name. Do > we even need an enum, or could we just return a bool indicating if starting the > request suceeded or failed? If it failed for some other reason, I'd prefer we > go through the normal failure path, anyways (Create a request and then fail it > with an error). Shutdown is a bit magical, in that creating a request at that > point in time may be problematic. Left it as void. It does not return anything useful at the moment. https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:320: bool is_shutdown() const { return is_shutdown_; } On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > I don't think this function is needed anymore? Needed. We check the shutdown state in the DownloadManagerImpl as in one of the previous patchsets.
On 2016/08/21 22:58:20, Randy Smith - Not in Fridays wrote: > On 2016/08/18 21:50:21, ananta wrote: > > On 2016/08/18 15:33:57, Randy Smith - Not in Fridays wrote: > > > > > > https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... > > > File content/browser/download/download_manager_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2251643003/diff/60001/content/browser/downloa... > > > content/browser/download/download_manager_impl.cc:109: > DownloadInterruptReason > > > reason = DownloadManagerImpl::BeginDownloadRequest( > > > On 2016/08/18 01:53:25, ananta wrote: > > > > On 2016/08/17 15:53:54, Randy Smith - Not in Fridays wrote: > > > > > If this is the only place this function is called (which fits my memory > > and > > > my > > > > > quick scan of this CL), can it be moved to a file static function in > this > > > > file? > > > > > Or even just hoist the guts into this function? I don't have any major > > > > > objections to a static function on DMI that's called on the IO thread > (the > > > > class > > > > > lives on the UI thread) but I think if it doesn't need to be on DMI it > > > > shouldn't > > > > > be. > > > > > > > > This function is also called from resource_dispatcher_host_unittest.cc in > > the > > > > download tests. I agree with the static function ugliness. However for > this > > > use > > > > case we need a way for the function to be accessible outside this file. > > > > > > Given that the function is no longer in rdhi.cc, shouldn't the unit test for > > it > > > be moved out of rdhu.cc? (I'm not sure how the unit test affects making it > > > static to this file, I'll look at that more closely in the next round of > > > review.) > > > > > > The merging of the two functions is probably reasonable to leave to the > > > downloads folks if it requires test re-evaluations, but moving the test > makes > > > sense to me? > > > > I have added a TODO in the test to move it out in a later patchset if that is > ok > > with you. > > We can move the method out of the DownloadManagerImpl class then. > > Help me understand why it doesn't belong in this CL? I'm on board with the goal > of making CLs as small as possible for review, maintenance, and revert/reland > granularity purposes, but I also want CLs to atomically move the tree from/to > clean states. Having a function in one file and it's unit test in the unit test > grouping for a different files seems wrong. What am I missing? > > (Having said that, you probably want to wait until we've settled the various > architectural issues before moving it, if you do--let's make sure it won't be > wasted work.) Sure thing.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ananta@chromium.org changed reviewers: + jam@chromium.org
+jam for content owners
This looks fine from my end, makes sense to split it up into small changes with the eventual goal of removing all the download methods from RDH (but not in this cl). Randy: do you have any outstanding comments (there were too many comments here for me to go through all of them)? Would a sync up over VC be useful here (not just for this patch, but for this overall work)? In general, the goal is to separate downloads from the eventual network service code. The reasoning is that downloads code can be thought of as a general consumer of networking, just like blink, extensions, plugins etc... Layering it on top of networking keeps separation of concerns which will force us to keep networking service generic but also capable of supporting different use cases. Also, the download code in content/browser/downloads and chrome/browser/download is pretty big and it makes sense to keep them all together in a future download service.
On 2016/08/22 22:29:46, jam wrote: > This looks fine from my end, makes sense to split it up into small changes with > the eventual goal of removing all the download methods from RDH (but not in this > cl). > > Randy: do you have any outstanding comments (there were too many comments here > for me to go through all of them)? > > Would a sync up over VC be useful here (not just for this patch, but for this > overall work)? In general, the goal is to separate downloads from the eventual > network service code. The reasoning is that downloads code can be thought of as > a general consumer of networking, just like blink, extensions, plugins etc... > Layering it on top of networking keeps separation of concerns which will force > us to keep networking service generic but also capable of supporting different > use cases. Also, the download code in content/browser/downloads and > chrome/browser/download is pretty big and it makes sense to keep them all > together in a future download service. John: I haven't had a chance to go through Ananta's responses to my comments yet, but my comments are still at the substantive level, so I doubt I'm ready to stamp yet. I would value a sync up over VC, but I don't think it's needed for this CL; it's more that I'd like to understand better how you and she are thinking about moving forward in this space to inform future review. But maybe Ananta's cleared up all my confusion in her comments and I just don't know that yet :-}. I'm good with evaluating this CL in terms of componentization and not worrying about mojoization for the moment, and I think I see how the choices being made are driven by the componentization question. I'll respond to your questions in more detail after I've had a chance to read Ananta's responses.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/22 22:34:29, Randy Smith - Not in Fridays wrote: > On 2016/08/22 22:29:46, jam wrote: > > This looks fine from my end, makes sense to split it up into small changes > with > > the eventual goal of removing all the download methods from RDH (but not in > this > > cl). > > > > Randy: do you have any outstanding comments (there were too many comments here > > for me to go through all of them)? > > > > Would a sync up over VC be useful here (not just for this patch, but for this > > overall work)? In general, the goal is to separate downloads from the eventual > > network service code. The reasoning is that downloads code can be thought of > as > > a general consumer of networking, just like blink, extensions, plugins etc... > > Layering it on top of networking keeps separation of concerns which will force > > us to keep networking service generic but also capable of supporting different > > use cases. Also, the download code in content/browser/downloads and > > chrome/browser/download is pretty big and it makes sense to keep them all > > together in a future download service. > > John: I haven't had a chance to go through Ananta's responses to my comments > yet, but my comments are still at the substantive level, so I doubt I'm ready to > stamp yet. I would value a sync up over VC, but I don't think it's needed for > this CL; it's more that I'd like to understand better how you and she are > thinking about moving forward in this space to inform future review. But > maybe Ananta's cleared up all my confusion in her comments and I just don't know > that yet :-}. I'm good with evaluating this CL in terms of componentization and > not worrying about mojoization for the moment, and I think I see how the choices > being made are driven by the componentization question. > > I'll respond to your questions in more detail after I've had a chance to read > Ananta's responses. I moved the TransferResponseStartedDownload and TransferRequestRedirectedDownload tests to download_manager_impl_unittest.cc. There are 5 other tests in ResourceDispatcherHostTests which are related to download. They are as below: ResourceDispatcherHostTest.CancelRequestsForContext ResourceDispatcherHostTest.ForbiddenDownload ResourceDispatcherHostTest.IgnoreCancelForDownloads I will move those out in a separate patchset if that is ok with you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/22 19:14:57, ananta wrote: > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > On 2016/08/18 22:27:09, ananta wrote: > > > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > > > I don't think it makes sense to move this call and the below delegate call > > out > > > > of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you > > move > > > > CreateResourceHandlerForDownload out you should also move these methods > off > > > > ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate > (though > > > > that's problematic because the DownloadManager is a UI thread object *and* > > > it's > > > > not clear the RDHI can figure out the correct DownloadManager at the point > > > this > > > > function is called). All of which brings me back to "Why can't we just > push > > > all > > > > this logic into the ResourceDispatcherHostDelegate?" > > > > > > Please correct me if I am wrong here. The ResourceDispatcherHostDelegate > takes > > > on > > > a variety of forms, one of them being in Chrome. The RDH delegate mechanism > > > allows content to get into the embedder (chrome). The embedder can only get > to > > > content via public interfaces. Till now downloads, save file requests were > > fully > > > handled in content. I don't see how we can push the > > > CreateResourceHandlerForDownload onto the > > > delegate. While the interceptor approach is hacky at best, it is not > intended > > by > > > a live for ever thing. Once we hash out how the public interface to RDH is > > going > > > to look like after it moves out of content, we can revisit this. > > > > > > > > > > > > > No, that's right. Another way of putting this is that ResourceHandler isn't a > > class that's visible to Chrome, so Chrome can't create an instance of it. > Sorry > > I missed this on my first pass. > > > > But I'd still like to have a clear sense of the overall plan in this space. > As > > I understand it, the short term goal that this CL is driving towards is moving > > content/browser/loader out of content and into a component, with the goal > beyond > > that one being to wrap content/browser/loader in a mojo interface. Is your > > thought that we'll maintain the interceptor registration architecture through > > the componentization of content/browser/loader, and then change to something > > else for the mojo wrapping? If so, could you give me a sense as to what the > > something else would look like? > > > > It seems as if ResourceHandlers are going to stay a core part of the > > content/browser/loader architecture, so I'd think that (after mojoization) > > anything that created a ResourceHandler would have to be *inside* > > content/browser/loader. So that would argue for pulling the information about > > downloads and the download resource handler into content/browser/loader, and > > finding some other place within the downloads system to make the architectural > > cut. Do you have a different image of what mojoization might look like? > > > > ("The interceptor is what is planned for componentization and I don't yet know > > what we'll do for mojoization" is a fine response, I just want to make sure > > that's the status rather than assuming it.) > > It does seem plausible that we may want to have DownloadResourceHandler or a > variant of it live inside the content/browser/loader folder. The > download_resource_handler.h/.cc files pull in other files from the > content/browser/download folder. If it is ok with you we can do this in a second > pass once we have the code in content/browser/loader ready to be moved out. In > any case the interface of RDHI or whatever we chose to call it once it moves it > is by no means ironed out. The interceptor is just an initial step to help us > move the loader code out. Discussed this a bit with jam. The goal of the network service work is to come up with a higher level API for issuing/handling network requests. Clients of this service could be chrome, blink, content, etc. In this context downloads could be a client of this network service. Additionally the downloads code today spans content and chrome and it may make sense for this code to live in the same process (browser) for performance reasons. The network service eventually will move to its own process.
Response to comments. As noted below, I think we should talk interactively; I'm not sure we're making progress via CL comments. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/22 19:14:57, ananta wrote: > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > On 2016/08/18 22:27:09, ananta wrote: > > > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > > > I don't think it makes sense to move this call and the below delegate call > > out > > > > of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you > > move > > > > CreateResourceHandlerForDownload out you should also move these methods > off > > > > ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate > (though > > > > that's problematic because the DownloadManager is a UI thread object *and* > > > it's > > > > not clear the RDHI can figure out the correct DownloadManager at the point > > > this > > > > function is called). All of which brings me back to "Why can't we just > push > > > all > > > > this logic into the ResourceDispatcherHostDelegate?" > > > > > > Please correct me if I am wrong here. The ResourceDispatcherHostDelegate > takes > > > on > > > a variety of forms, one of them being in Chrome. The RDH delegate mechanism > > > allows content to get into the embedder (chrome). The embedder can only get > to > > > content via public interfaces. Till now downloads, save file requests were > > fully > > > handled in content. I don't see how we can push the > > > CreateResourceHandlerForDownload onto the > > > delegate. While the interceptor approach is hacky at best, it is not > intended > > by > > > a live for ever thing. Once we hash out how the public interface to RDH is > > going > > > to look like after it moves out of content, we can revisit this. > > > > > > > > > > > > > No, that's right. Another way of putting this is that ResourceHandler isn't a > > class that's visible to Chrome, so Chrome can't create an instance of it. > Sorry > > I missed this on my first pass. > > > > But I'd still like to have a clear sense of the overall plan in this space. > As > > I understand it, the short term goal that this CL is driving towards is moving > > content/browser/loader out of content and into a component, with the goal > beyond > > that one being to wrap content/browser/loader in a mojo interface. Is your > > thought that we'll maintain the interceptor registration architecture through > > the componentization of content/browser/loader, and then change to something > > else for the mojo wrapping? If so, could you give me a sense as to what the > > something else would look like? > > > > It seems as if ResourceHandlers are going to stay a core part of the > > content/browser/loader architecture, so I'd think that (after mojoization) > > anything that created a ResourceHandler would have to be *inside* > > content/browser/loader. So that would argue for pulling the information about > > downloads and the download resource handler into content/browser/loader, and > > finding some other place within the downloads system to make the architectural > > cut. Do you have a different image of what mojoization might look like? > > > > ("The interceptor is what is planned for componentization and I don't yet know > > what we'll do for mojoization" is a fine response, I just want to make sure > > that's the status rather than assuming it.) > > It does seem plausible that we may want to have DownloadResourceHandler or a > variant of it live inside the content/browser/loader folder. The > download_resource_handler.h/.cc files pull in other files from the > content/browser/download folder. If it is ok with you we can do this in a second > pass once we have the code in content/browser/loader ready to be moved out. In > any case the interface of RDHI or whatever we chose to call it once it moves it > is by no means ironed out. The interceptor is just an initial step to help us > move the loader code out. It's ok with me--I just want to have a clear idea where we're going. https://codereview.chromium.org/2251643003/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/23 04:27:38, ananta wrote: > On 2016/08/22 19:14:57, ananta wrote: > > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > > On 2016/08/18 22:27:09, ananta wrote: > > > > On 2016/08/18 17:38:57, Randy Smith - Not in Fridays wrote: > > > > > I don't think it makes sense to move this call and the below delegate > call > > > out > > > > > of RDHI--the delegate is *for* RDHI to call, and no-one else. So if you > > > move > > > > > CreateResourceHandlerForDownload out you should also move these methods > > off > > > > > ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate > > (though > > > > > that's problematic because the DownloadManager is a UI thread object > *and* > > > > it's > > > > > not clear the RDHI can figure out the correct DownloadManager at the > point > > > > this > > > > > function is called). All of which brings me back to "Why can't we just > > push > > > > all > > > > > this logic into the ResourceDispatcherHostDelegate?" > > > > > > > > Please correct me if I am wrong here. The ResourceDispatcherHostDelegate > > takes > > > > on > > > > a variety of forms, one of them being in Chrome. The RDH delegate > mechanism > > > > allows content to get into the embedder (chrome). The embedder can only > get > > to > > > > content via public interfaces. Till now downloads, save file requests were > > > fully > > > > handled in content. I don't see how we can push the > > > > CreateResourceHandlerForDownload onto the > > > > delegate. While the interceptor approach is hacky at best, it is not > > intended > > > by > > > > a live for ever thing. Once we hash out how the public interface to RDH is > > > going > > > > to look like after it moves out of content, we can revisit this. > > > > > > > > > > > > > > > > > > No, that's right. Another way of putting this is that ResourceHandler isn't > a > > > class that's visible to Chrome, so Chrome can't create an instance of it. > > Sorry > > > I missed this on my first pass. > > > > > > But I'd still like to have a clear sense of the overall plan in this space. > > As > > > I understand it, the short term goal that this CL is driving towards is > moving > > > content/browser/loader out of content and into a component, with the goal > > beyond > > > that one being to wrap content/browser/loader in a mojo interface. Is your > > > thought that we'll maintain the interceptor registration architecture > through > > > the componentization of content/browser/loader, and then change to something > > > else for the mojo wrapping? If so, could you give me a sense as to what the > > > something else would look like? > > > > > > It seems as if ResourceHandlers are going to stay a core part of the > > > content/browser/loader architecture, so I'd think that (after mojoization) > > > anything that created a ResourceHandler would have to be *inside* > > > content/browser/loader. So that would argue for pulling the information > about > > > downloads and the download resource handler into content/browser/loader, and > > > finding some other place within the downloads system to make the > architectural > > > cut. Do you have a different image of what mojoization might look like? > > > > > > ("The interceptor is what is planned for componentization and I don't yet > know > > > what we'll do for mojoization" is a fine response, I just want to make sure > > > that's the status rather than assuming it.) > > > > It does seem plausible that we may want to have DownloadResourceHandler or a > > variant of it live inside the content/browser/loader folder. The > > download_resource_handler.h/.cc files pull in other files from the > > content/browser/download folder. If it is ok with you we can do this in a > second > > pass once we have the code in content/browser/loader ready to be moved out. In > > any case the interface of RDHI or whatever we chose to call it once it moves > it > > is by no means ironed out. The interceptor is just an initial step to help us > > move the loader code out. > > Discussed this a bit with jam. The goal of the network service work is to come > up with a higher level API for issuing/handling network requests. Clients of > this service could be chrome, blink, content, etc. In this context downloads > could be a client of this network service. Additionally the downloads code today > spans content and chrome and it may make sense for this code to live in the same > process (browser) for performance reasons. The network service eventually will > move to its own process. So this strikes me as orthogonal to the issues we were discussing. I'm not saying that the entire downloads system should be included in content/browser/loader, just that content/browser/loader needs to have some *concept* of downloads; that (given Content-Disposition: attachment) there's no other choice. In a mojo context, this might mean that there's some downloads service that the content/browser/loader has a reference to. The reason I raise this is that if content/browser/loader needs to have some concept of downloads, having an interceptor to call is a fine way to manage it; it's naturally parallel to having a reference to a service in mojo context (AIUI). If this *doesn't* fit with your image of where we're eventually going to go with the c/b/l <-> downloads interface, could you sketch out how you think we'll handle Content-Disposition: attachment + Content-Type that there's no registered plugin for? Just at a high level. (This issue doesn't need to be resolved for this CL.) https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1514: request_data.referrer_policy), On 2016/08/22 19:14:57, ananta wrote: > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > On 2016/08/18 22:27:09, ananta wrote: > > > On 2016/08/18 17:35:43, mmenke wrote: > > > > Moving where we set the referrer seems concerning - we've already passed > the > > > > URLRequest to BlobProtocolHandler, ServiceWorkerRequestHandler, > > > > ForeignFetchRequestHandler, AppCacheInterceptor, CrossSiteResourceHandler, > > and > > > > all the standard handlers at this point. Some certainly rely on > > > > first_party_for_cookies. Not sure if any depend on referrer or referrer > > > policy, > > > > but it seems like a bad idea to set anything on the request after we've > > > already > > > > let the world have a looksee at it. > > > > > > Thanks. Fixed this by bringing back the old BeginRequestInternal function > > which > > > is called elsewhere in this file and from the new BeginURLRequest function. > > > > This looks to me to still have been changed on the BeginDownloadRequest path. > > In the old code, BeginDownload() set the referrer at the beginning of the > > function and then called into the BlobStorageContext and protocol handler with > > the request. Now, DownloadManager::BeginDownloadRequest doesn't set the > > referrer but still has the calls into the BlobStorageContext & etc. Am I > > missing something? > > > > (Thanks for catching this, Matt.) > > Yes this has changed. The current code for context is here: > https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch... > GetRequestBlobDataHandle does this > https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_request_jo... > > SetRequestedBlobDataHandle does this > https://cs.chromium.org/chromium/src/storage/browser/blob/blob_url_request_jo... > > The code is not inspecting the referrer. I think it should be safe? The issn't isn't about whether the code is currently inspecting the referrer, it's whether we are making changes to the request after the point at which we give external sections of the cod visibility into the request. Another way to think about it is that this change makes the blob code more fragile, because now people making changes in the future need to know that they can't trust the referrer that is visible in {Get,Set}RequestBlobHandle. Maybe that's ok--it's not obvious what blob handlers would care about the referrer. But it's better to keep dependency relationships like this to a minimum; the more we have the more likely are bugs down the road. Can you just promote SetReferrerForRequest to a (static) method on the RDHI and call it from the initiating code sections where it used to be called? https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/browse... content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); On 2016/08/22 19:14:57, ananta wrote: > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > I have a slight preference for this reference being to DownloadResourceHandler > > instead of DownloadManager{Impl}; DRH is an IO thread object, and DMI a UI > > thread object, and the RDH is much closer to DRH architecturally than it is to > > DMI. DMI is what represents the download system to the higher levels, and RDH > > is pretty low level. > > > > Having said that, I don't feel strongly about it--if you really want the > > reference to be to DMI, I'll accept it (and maybe circle back with Asanka and > > jam@ after Asanka gets back). > > This is a temporary change to get us to the end goal of moving the loader code > out. I think your points about having the DownloadResourceHandler live in the > loader folder may be what we end up doing anyways once we are at that point. We > can leave this as is for now? I'm not sure we're communicating; would you maybe catch me on IM? Here's a summary of my perspective, but as I say, I'm not sure we're communicating, so interactive communication would probably be better. * I understand that the short term goal is moving the loading code out into its own component. * I'm a big believer in "temporary solutions aren't", and as a result I'm rarely willing to allow landing code in the code base that I wouldn't be comfortable being there forever. Sometimes I'll do it, but I need a good argument as to why it's worth doing. I haven't heard such an argument yet; I totally get that we have a goal of moving loader code out into a component, but I don't have any understanding of why that should change my code review evaluations. * I'm not trying to create major work for you, I'm trying to explore what the best options are given the short-term goal. My perception (which may be incorrect--please let me know if you think it is) is that the changes I'm suggesting are fairly small ones (+). So I keep being confused as to why you push back with "but this is only temporary". What am I missing? * More specifically, because I've convinced myself that something like an interceptor will be necessary long term (in mojo terms, a reference to an external service) I want to make the interceptor addition as clean as possible (it's much more likely we'll go in and rip out something that'll need to be changed than that we'll tweak something that doesn't need to be changed). Is there some reason why the interceptor shouldn't be passed to the constructor instead of doing through a separate setter? * Is there some reason why we can't use a static function on the DownloadResourceHandler instead of a static function on the DownloadManagerImpl? Again, that seems like a fairly small change? (+) The exception here is moving the tests. I understand that that's real work, and I'm sorry about that, but what I don't understand is why, if we agree that work needs to be done at some point, it shouldn't be done now to keep the results of this CL more cohesive. Again, it feels like there's some context I'm missing. Do you have a sense what it is? https://codereview.chromium.org/2251643003/diff/300001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/downlo... content/browser/download/download_resource_handler.cc:192: // tasks. On 2016/08/22 19:14:57, ananta wrote: > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > I'm sorry, I don't understand what drove moving this. Was there a problem > with > > where it was? > > > > The problem with moving it is that it gives more time for the user (or > malicious > > code) to navigate the tab away and thus make the tab_info_ invalid/unuseful. > So > > it's better to get the information as quickly as possible when the download > > starts (i.e. triggered from the constructor). > > > > The "This is safe" in the original comment was about why it was ok to pass a > raw > > pointer to a member variable of an object that lives on the IO thread to a > > different thread (i.e. why that raw pointer would stay valid no matter what > > thread races occurred). It wasn't about why navigating away was safe; > > navigating away will just result in a null web_contents, which will result in > > the tab information not being set. > > The ResourceRequestInfoImpl instance which is associated with the original > request is not available here in all cases. For normal downloads which are > detected via the mime type, yes. However for downloads initiated via the context > menu for e.g. Save As, or Save image as, etc the request is initiated eventually > via the DownloadUrl function in download_manager_impl.cc. OnStart gets called in > all cases, success/failure and hence seemed like the right place to do this. So the code is squirrelly, so I may be mistaken, but I just spent some time tracing code, and DownloadUrl in download_manager_impl.cc will never go through this code. It does have anything to do with the DownloadResourceHandler, and instead creates a UrlDownloader to handle the job of getting bits from the URLRequest to the download system. So by my reading Save As/Save Image as will never go through this code, so there's no point moving it for them. What am I missing? https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2315: is_content_initiated, true); On 2016/08/22 19:14:57, ananta wrote: > On 2016/08/21 23:32:57, Randy Smith - Not in Fridays wrote: > > Won't this already have been called? If the request is a download, then at > some > > point (before or after this) the download resource handler must have been put > on > > the chain, and if it was, HandleDownloadStarted() was called in > > CreateResourceHandlerForDownload(). What am I missing? > > The DownloadResourceHandler is created and is put on the chain for normal > downloads. However it is not for downloads initiated via Save As for e.g. Right, but I don't believe that this code is called in that case; UrlDownloader calls request_->Start() directly. So I believe that in all cases where the code goes through this path, the handler passed in will have been created through CreateResourceHandlerForDownload(). If you disagree, can you give me the specific code path that you think goes through here without going through CreateResourceHandlerForDownload()? (Part of my disbelief is existential; if we're doing a download and we have a ResourceHandler, it *must* be a DownloadResourceHandler (or have one at the end of the chain), and the only place that's created is in CreateResourceHandlerForDownload().)
On 2016/08/23 01:50:33, ananta wrote: > On 2016/08/22 22:34:29, Randy Smith - Not in Fridays wrote: > > On 2016/08/22 22:29:46, jam wrote: > > > This looks fine from my end, makes sense to split it up into small changes > > with > > > the eventual goal of removing all the download methods from RDH (but not in > > this > > > cl). > > > > > > Randy: do you have any outstanding comments (there were too many comments > here > > > for me to go through all of them)? > > > > > > Would a sync up over VC be useful here (not just for this patch, but for > this > > > overall work)? In general, the goal is to separate downloads from the > eventual > > > network service code. The reasoning is that downloads code can be thought of > > as > > > a general consumer of networking, just like blink, extensions, plugins > etc... > > > Layering it on top of networking keeps separation of concerns which will > force > > > us to keep networking service generic but also capable of supporting > different > > > use cases. Also, the download code in content/browser/downloads and > > > chrome/browser/download is pretty big and it makes sense to keep them all > > > together in a future download service. > > > > John: I haven't had a chance to go through Ananta's responses to my comments > > yet, but my comments are still at the substantive level, so I doubt I'm ready > to > > stamp yet. I would value a sync up over VC, but I don't think it's needed for > > this CL; it's more that I'd like to understand better how you and she are > > thinking about moving forward in this space to inform future review. But > > maybe Ananta's cleared up all my confusion in her comments and I just don't > know > > that yet :-}. I'm good with evaluating this CL in terms of componentization > and > > not worrying about mojoization for the moment, and I think I see how the > choices > > being made are driven by the componentization question. > > > > I'll respond to your questions in more detail after I've had a chance to read > > Ananta's responses. > > I moved the TransferResponseStartedDownload and > TransferRequestRedirectedDownload tests to > download_manager_impl_unittest.cc. Thanks! I'll review those moves, but the basic fact of moving them is goodness. > There are 5 other tests in Presuming you meant 5 total. > ResourceDispatcherHostTests which > are related to download. They are as below: > > ResourceDispatcherHostTest.CancelRequestsForContext > ResourceDispatcherHostTest.ForbiddenDownload > ResourceDispatcherHostTest.IgnoreCancelForDownloads It's fine not to move those in this CL. I am currently of the opinion that they should not be moved period, because they test things that are about the conceptual understanding of downloads that I think we'll need to keep in content/browser/loader, but I'm happy to have that discussion later. But please do let me have it :-}. > > I will move those out in a separate patchset if that is ok with you.
Quick summary of an offline conversation between ananta@ and I; ananta@ please correct if I get any of this wrong: * The interceptor will be passed to the constructor instead of by setter. * The interceptor will be a function on the DownloadResourceHandler, not the DownloadManagerImpl. * We went over the implications of keeping ResourceRequestInfoImpl within ResourceDispatcherHostImpl, and I'll use that information to do a re-review of my points around InitializeDownloadTabInfoOnUIThread and HandleDownloadStarted. * mmenke@ is going to do a look at the CL and make suggestions as to the proper architecture for SetReferrerForRequest().
I think we should step back for a moment, and think about what API we want to expose. Via the main path, we create the URLRequest, create the ResourceRequestInfo, set its referrer, and then expose it to the world in all its glory when creating the throttles and handlers (Which can access the URLRequest, ResourceRequestInfo, and the referrer), and then start the request. Via the download path, we create the URLRequest, expose it to the world when we create its handlers, and only then create the ResourceRequestInfo and set its referrer. In a Mojo world, what should this look like? ResourceHandlers, ResourceThrottles, ResourceRequestInfo, and mime sniffing will still live in the loading stack...But what about downloads? Should those also be part of the loading stack, or should they exist on the other side of the Mojo divide, to take over the data channel (Or create their own channel, if we know something's a download in advance)? Post-Mojo, are we really going to want to have multiple different endpoints for ResourceHandler stack, or just one? If we plan keep the download system as it is, I think we should try and decide how we should hook up a DownloadResourceHandler to mime sniffing, if we move the DownloadResourceHandler outside of the component that contains RDHI before we proceed with this CL. (In addition to figuring out how to make creation of the loader stack more consistent in the two paths)
On 2016/08/23 21:35:03, mmenke wrote: > I think we should step back for a moment, and think about what API we want to > expose. > > Via the main path, we create the URLRequest, create the ResourceRequestInfo, set > its referrer, and then expose it to the world in all its glory when creating the > throttles and handlers (Which can access the URLRequest, ResourceRequestInfo, > and the referrer), and then start the request. > > Via the download path, we create the URLRequest, expose it to the world when we > create its handlers, and only then create the ResourceRequestInfo and set its > referrer. > > In a Mojo world, what should this look like? ResourceHandlers, > ResourceThrottles, ResourceRequestInfo, and mime sniffing will still live in the > loading stack...But what about downloads? Should those also be part of the > loading stack, or should they exist on the other side of the Mojo divide, to > take over the data channel (Or create their own channel, if we know something's > a download in advance)? Post-Mojo, are we really going to want to have multiple > different endpoints for ResourceHandler stack, or just one? > > If we plan keep the download system as it is, I think we should try and decide > how we should hook up a DownloadResourceHandler to mime sniffing, if we move the > DownloadResourceHandler outside of the component that contains RDHI before we > proceed with this CL. (In addition to figuring out how to make creation of the > loader stack more consistent in the two paths) One way you could imagine downloads working in a Mojo world is that we pass a MojoURLRequestThingy to a download service. The way <a download> works would be the renderer creates the MojoThingy itself, and then passes it over to the download system in the browser process. The way that loader/ sniffs that a request is a download, and creates a new MojoURLRequestThingy, which is passes to the download service. Or the consumer of the original MojoURLRequestThingy could find out that it was sniffed as a download via a mojo call, and then pass it along to the download service. Of course, we could still keep things as they are. Either way, think we should decide on where we'd like to going now, instead of just blindly trying to remove dependencies.
On 2016/08/23 21:42:52, mmenke wrote: > On 2016/08/23 21:35:03, mmenke wrote: > > I think we should step back for a moment, and think about what API we want to > > expose. > > > > Via the main path, we create the URLRequest, create the ResourceRequestInfo, > set > > its referrer, and then expose it to the world in all its glory when creating > the > > throttles and handlers (Which can access the URLRequest, ResourceRequestInfo, > > and the referrer), and then start the request. > > > > Via the download path, we create the URLRequest, expose it to the world when > we > > create its handlers, and only then create the ResourceRequestInfo and set its > > referrer. > > > > In a Mojo world, what should this look like? ResourceHandlers, > > ResourceThrottles, ResourceRequestInfo, and mime sniffing will still live in > the > > loading stack...But what about downloads? Should those also be part of the > > loading stack, or should they exist on the other side of the Mojo divide, to > > take over the data channel (Or create their own channel, if we know > something's > > a download in advance)? Post-Mojo, are we really going to want to have > multiple > > different endpoints for ResourceHandler stack, or just one? > > > > If we plan keep the download system as it is, I think we should try and decide > > how we should hook up a DownloadResourceHandler to mime sniffing, if we move > the > > DownloadResourceHandler outside of the component that contains RDHI before we > > proceed with this CL. (In addition to figuring out how to make creation of > the > > loader stack more consistent in the two paths) > > One way you could imagine downloads working in a Mojo world is that we pass a > MojoURLRequestThingy to a download service. The way <a download> works would be > the renderer creates the MojoThingy itself, and then passes it over to the > download system in the browser process. The way that loader/ sniffs that a > request is a download, and creates a new MojoURLRequestThingy, which is passes > to the download service. Or the consumer of the original MojoURLRequestThingy > could find out that it was sniffed as a download via a mojo call, and then pass > it along to the download service. > > Of course, we could still keep things as they are. Either way, think we should > decide on where we'd like to going now, instead of just blindly trying to remove > dependencies. Perhaps one way to think of this is to imagine the downloads system as a client of the NetworkService. The clients we can imagine now include blink, downloads, possibly extensions, chrome. The network service on a higher level should possibly be in charge of issuing network requests and after some processing delivering responses to its clients who then decide what they want to do with it. That may be over simplifying it I guess, but once we have the network service interface ironed out we can revisit this.
On 2016/08/23 21:35:03, mmenke wrote: > I think we should step back for a moment, and think about what API we want to > expose. > > Via the main path, we create the URLRequest, create the ResourceRequestInfo, set > its referrer, and then expose it to the world in all its glory when creating the > throttles and handlers (Which can access the URLRequest, ResourceRequestInfo, > and the referrer), and then start the request. > > Via the download path, we create the URLRequest, expose it to the world when we > create its handlers, and only then create the ResourceRequestInfo and set its > referrer. > > In a Mojo world, what should this look like? ResourceHandlers, > ResourceThrottles, ResourceRequestInfo, and mime sniffing will still live in the > loading stack...But what about downloads? Should those also be part of the > loading stack, or should they exist on the other side of the Mojo divide, to > take over the data channel (Or create their own channel, if we know something's > a download in advance)? Post-Mojo, are we really going to want to have multiple > different endpoints for ResourceHandler stack, or just one? I've thought about trying to make the downloads service a special kind of plugin (since we pick out plugins by mime type as well). Presumably plugins aren't part of the loading stack. I'll note that downloads are special (and not in a good way) in that they aren't associated with a renderer process, and in an ideal world (i.e. the refactor that davidben@'s talked about) they would get "taken away" from the RDHI and handled separately. I think the model for that was that a ResourceLoader would be run separately from the RDHI. Unfortunately, I don't think that sheds light on the issue you're raising; at least for me, it increases the feeling of "here be dragons" and means I don't have a clear image of where we're going with downloads and RDHI in the long term, and don't think I'll get it without a fair amount of design work. Putting these two things together, I've been aiming for minimal change from what we currently have that's compatible with componetization, and assuming that content/browser/loader will in the medium term have some conceptual knowledge of downloads but the handling of the data will be outside of content/browser/loader, and aiming my review to be a step in that direction. > If we plan keep the download system as it is, I think we should try and decide > how we should hook up a DownloadResourceHandler to mime sniffing, if we move the > DownloadResourceHandler outside of the component that contains RDHI before we > proceed with this CL. (In addition to figuring out how to make creation of the > loader stack more consistent in the two paths) I think for componentization, this CL is in at least basic alignment with the that goal: The RDHI component needs to know how to do something with downloads, but we don't want a lot of intelligence around that within RDHI, so it takes an interceptor. That interceptor is stored on RDHI (rather than something having to do with the MimeTypeResourceHandler) because the RDHI is persistent and the MimeTypeResourceHandler isn't.
Offline conversation updates: Matt and I had a long conversation offline, the upshot of which was that the code in BeginDownload and BeginSaveFile, despite the names, looked much more closely tied to the ResourceDispatcherHost than the systems it was acting on behalf of, and we weren't sure it should be moved out of RDHI at all. Ananta and I talked, and decided that the easy way to handle that was an InitializeRequest() method on RDHI that could be called by external consumers that would do the common stuff, after which child process security checks and handler creation could be done, followed by BeginUrlRequest. (Removing the child process security checks is an explicit goal of this effort.) Calling the ResourceDispatcherHostImpl delegate would be handled as it is now, but an |if (is_download)| test inside of BeginUrlRequest, which if it was a download would call the delegate and add the throttles. Ananta's going to put together a PS with that configuration. Matt, please raise a flag if this doesn't make sense to you.
Description was changed from ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function CreateResourceHandlerForDownload which is registered with ResourceDispatcherHostImpl via the new ResourceDispatcherHostImpl::RegisterCreateDownloadHandlerInterceptor function. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 ========== to ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 ==========
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/23 22:53:47, Randy Smith - Not in Fridays wrote: > Offline conversation updates: Matt and I had a long conversation offline, the > upshot of which was that the code in BeginDownload and BeginSaveFile, despite > the names, looked much more closely tied to the ResourceDispatcherHost than the > systems it was acting on behalf of, and we weren't sure it should be moved out > of RDHI at all. Ananta and I talked, and decided that the easy way to handle > that was an InitializeRequest() method on RDHI that could be called by external > consumers that would do the common stuff, after which child process security > checks and handler creation could be done, followed by BeginUrlRequest. > (Removing the child process security checks is an explicit goal of this effort.) > > Calling the ResourceDispatcherHostImpl delegate would be handled as it is now, > but an |if (is_download)| test inside of BeginUrlRequest, which if it was a > download would call the delegate and add the throttles. > > Ananta's going to put together a PS with that configuration. > > Matt, please raise a flag if this doesn't make sense to you. Randy, I made the changes we discussed over IM. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This basic approach looks good to me. I still need to review the tests in detail. Matt, do you want to take a look? https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... content/browser/download/download_manager_impl.cc:585: } Is this not common code with the other initiation pathways? Can it be moved into the Initialize function above,or into the BeginURLRequest function below? https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2654: request_info->GetRouteID(), is_content_initiated, true, &throttles); I'm inclined to think this interface and its uses would be a bit cleaner if it was renamed to something like GetThrottlesForRequest, called for all requests, and the download test pushed into the delegate. No need to do that in this CL; just a thought. https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:235: // This method should be removed or moved outside this class. I suspect you could remove this method at this point and just have places that need downloads make sure to set is_download and attach a download handler via DownloadResourceHandler::Create, and rely on the conditional in BeginUrlRequest to setup the throttles. Up to you, though. (And it won't help your long-term goal since that'll just push the download reference into MimeTypeSniffingHandler.)
I don't think I'm going to have time to look at this today. For now, this NOT LGTM, because I don't want this landed until dev blocker https://crbug.com/640545 is resolved, since it's pointing at the download code as the cause.
On 2016/08/24 14:47:35, mmenke wrote: > I don't think I'm going to have time to look at this today. For now, this NOT > LGTM, because I don't want this landed until dev blocker > https://crbug.com/640545 is resolved, since it's pointing at the download code > as the cause. Oh, and I do want to take a look at this, once that's resolved.
Ananta: I'm on bug triage today and tomorrow, and that plus the chance that Matt may want some large scale change means I'm going to lower the priority of doing the tests + include files review (but I'll leave it on my plate--I'm not blocking my next review on Matt). Just letting you know what to expect.
https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... content/browser/download/download_manager_impl.cc:585: } On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > Is this not common code with the other initiation pathways? Can it be moved > into the Initialize function above,or into the BeginURLRequest function below? The code which is common between the SaveFileManager and DownloadManagerImpl is the call to ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(). We want to get rid of references to ChildProcessSecurityPolicyImpl from the loader code and instead look into replacing it with a lightweight policy cache mechanism which can be updated with new policies/data as needed. We haven't thought this through fully yet.
https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2654: request_info->GetRouteID(), is_content_initiated, true, &throttles); On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > I'm inclined to think this interface and its uses would be a bit cleaner if it > was renamed to something like GetThrottlesForRequest, called for all requests, > and the download test pushed into the delegate. No need to do that in this CL; > just a thought. Sure thing. Please file a bug with your thoughts so we can address them in a future patch. https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:235: // This method should be removed or moved outside this class. On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > I suspect you could remove this method at this point and just have places that > need downloads make sure to set is_download and attach a download handler via > DownloadResourceHandler::Create, and rely on the conditional in BeginUrlRequest > to setup the throttles. Up to you, though. (And it won't help your long-term > goal since that'll just push the download reference into > MimeTypeSniffingHandler.) BeginURLRequest is used in the SaveFileManager and the Image SaveAs code paths in DownloadManagerImpl. It is not used from what I can see from the MimeTypeSniffingHandler codepath.
On 2016/08/24 14:47:35, mmenke (busy) wrote: > I don't think I'm going to have time to look at this today. For now, this NOT > LGTM, because I don't want this landed until dev blocker > https://crbug.com/640545 is resolved, since it's pointing at the download code > as the cause. To clarify, this reason I'm Not LGTMing this isn't that I don't have time, it's that there's an issue on Canary causing 30% of our browser crashers, and the ResourceThrottle stack is directly implicated in it (And, in particular, DownloadResourceThrottle). I don't think we should be landing any changes here until we figure it out and fix it. Landing this potentially interferes with investigation by changing the stack trace, or having other unexpected effects on the crash. If we land after branch, it also complicates landing the merge. If you guys want to help with the investigation, you're welcome to do so.
On 2016/08/24 20:37:04, mmenke (busy) wrote: > On 2016/08/24 14:47:35, mmenke (busy) wrote: > > I don't think I'm going to have time to look at this today. For now, this NOT > > LGTM, because I don't want this landed until dev blocker > > https://crbug.com/640545 is resolved, since it's pointing at the download code > > as the cause. > > To clarify, this reason I'm Not LGTMing this isn't that I don't have time, it's > that there's an issue on Canary causing 30% of our browser crashers, and the > ResourceThrottle stack is directly implicated in it (And, in particular, > DownloadResourceThrottle). I don't think we should be landing any changes here > until we figure it out and fix it. Landing this potentially interferes with > investigation by changing the stack trace, or having other unexpected effects on > the crash. If we land after branch, it also complicates landing the merge. > > If you guys want to help with the investigation, you're welcome to do so. Hrm... The crash mysteriously vanished today.
Ok, now that the crash has mysteriously vanished, I'm fine with landing this (Modulo layering issues) any time after branch. https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... content/browser/download/download_manager_impl.cc:585: } On 2016/08/24 18:52:03, ananta wrote: > On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > > Is this not common code with the other initiation pathways? Can it be moved > > into the Initialize function above,or into the BeginURLRequest function below? > > The code which is common between the SaveFileManager and DownloadManagerImpl is > the call to ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(). We > want to get rid of references to ChildProcessSecurityPolicyImpl from the loader > code and instead look into replacing it with a lightweight policy cache > mechanism which can be updated with new policies/data as needed. We haven't > thought this through fully yet. And what about blob logic? The download system really shouldn't know about that, either.
https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... content/browser/download/download_manager_impl.cc:585: } On 2016/08/25 19:10:25, mmenke (busy) wrote: > On 2016/08/24 18:52:03, ananta wrote: > > On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > > > Is this not common code with the other initiation pathways? Can it be moved > > > into the Initialize function above,or into the BeginURLRequest function > below? > > > > The code which is common between the SaveFileManager and DownloadManagerImpl > is > > the call to ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(). We > > want to get rid of references to ChildProcessSecurityPolicyImpl from the > loader > > code and instead look into replacing it with a lightweight policy cache > > mechanism which can be updated with new policies/data as needed. We haven't > > thought this through fully yet. > > And what about blob logic? The download system really shouldn't know about > that, either. Yes, I was specifically referring to the blob logic. Sorry, should have specified.
https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... content/browser/download/download_manager_impl.cc:585: } On 2016/08/25 19:10:25, mmenke (busy) wrote: > On 2016/08/24 18:52:03, ananta wrote: > > On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > > > Is this not common code with the other initiation pathways? Can it be moved > > > into the Initialize function above,or into the BeginURLRequest function > below? > > > > The code which is common between the SaveFileManager and DownloadManagerImpl > is > > the call to ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(). We > > want to get rid of references to ChildProcessSecurityPolicyImpl from the > loader > > code and instead look into replacing it with a lightweight policy cache > > mechanism which can be updated with new policies/data as needed. We haven't > > thought this through fully yet. > > And what about blob logic? The download system really shouldn't know about > that, either. Yes. It makes sense for that code to move back into BeginURLRequest(). I moved the check in the if (is_download) block to avoid changing behavior. Speaking to rdsmith, it seems like the blob logic should run for SaveAs as well. I added a TODO for that. https://codereview.chromium.org/2251643003/diff/410001/content/browser/downlo... content/browser/download/download_manager_impl.cc:585: } On 2016/08/25 19:12:38, Randy Smith - Not in Fridays wrote: > On 2016/08/25 19:10:25, mmenke (busy) wrote: > > On 2016/08/24 18:52:03, ananta wrote: > > > On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote: > > > > Is this not common code with the other initiation pathways? Can it be > moved > > > > into the Initialize function above,or into the BeginURLRequest function > > below? > > > > > > The code which is common between the SaveFileManager and DownloadManagerImpl > > is > > > the call to ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(). > We > > > want to get rid of references to ChildProcessSecurityPolicyImpl from the > > loader > > > code and instead look into replacing it with a lightweight policy cache > > > mechanism which can be updated with new policies/data as needed. We haven't > > > thought this through fully yet. > > > > And what about blob logic? The download system really shouldn't know about > > that, either. > > Yes, I was specifically referring to the blob logic. Sorry, should have > specified. Done.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Other than the test misunderstanding (:-{) and a couple of nits I'm fine with
this. I'm out in Fridays, so I'll stamp now (LGTM) conditional on the below
comments, but I'll leave interpreting them to your discretion--if you feel like
I've misunderstood code in my comments I'm ok with you explaining that and
committing rather than waiting for the round trip. Specifically, you're welcome
to use your own judgement on the includes.
Obviously, please do wait for Matt's stamp as well.
https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):
https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2654:
request_info->GetRouteID(), is_content_initiated, true, &throttles);
On 2016/08/24 18:56:06, ananta wrote:
> On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote:
> > I'm inclined to think this interface and its uses would be a bit cleaner if
it
> > was renamed to something like GetThrottlesForRequest, called for all
requests,
> > and the download test pushed into the delegate. No need to do that in this
> CL;
> > just a thought.
>
> Sure thing. Please file a bug with your thoughts so we can address them in a
> future patch.
Yeah, thinking this through more carefully, it wouldn't work for the reasons
mentioned on the other comment. For this idea to work, "is_download" must be
set on the RRII when the delegate method is called, and it isn't set on
downloads sniffed by MimeTypeSniffingHandler until after the RDHI has started to
feed data to the resource handler chain, at which point it's really well past
the point where it should be doing generic delegate calls on all requests.
Cleaning this up will take a more invasive refactor.
https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):
https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:235: // This method
should be removed or moved outside this class.
On 2016/08/24 18:56:06, ananta wrote:
> On 2016/08/24 14:34:21, Randy Smith - Not in Fridays wrote:
> > I suspect you could remove this method at this point and just have places
that
> > need downloads make sure to set is_download and attach a download handler
via
> > DownloadResourceHandler::Create, and rely on the conditional in
> BeginUrlRequest
> > to setup the throttles. Up to you, though. (And it won't help your
long-term
> > goal since that'll just push the download reference into
> > MimeTypeSniffingHandler.)
>
> BeginURLRequest is used in the SaveFileManager and the Image SaveAs code paths
> in DownloadManagerImpl. It is not used from what I can see from the
> MimeTypeSniffingHandler codepath.
Yes, I was thinking the throttle check and upcall could be moved down into
BeginRequestInternal. But you're right, a larger refactor would be required,
since while requests that go through MimeTypeSniffingHandler do go through
BeginRequestInternal, they aren't marked as downloads at that point. So the
throttle upcall would need to be moved much later, in fact moved past the point
at which RDHI thinks that information's started to be passed to the handlers (at
which point it really shouldn't be doing anything). Drat. Never mind.
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
File content/browser/download/download_manager_impl.cc (right):
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
content/browser/download/download_manager_impl.cc:33: #include
"content/browser/loader/throttling_resource_handler.h"
Are these still needed?
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
content/browser/download/download_manager_impl.cc:35: #include
"content/browser/resource_context_impl.h"
I think this could be replaced by a forward decl?
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
content/browser/download/download_manager_impl.cc:47: #include
"content/public/browser/resource_throttle.h"
Still needed?
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
File content/browser/download/download_manager_impl_unittest.cc (right):
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
content/browser/download/download_manager_impl_unittest.cc:1010:
Wow, I'm sorry, I guess I'm doing a poor job of expressing what I'm looking for.
The invariant I feel strongly about that I'd like this CL to keep is that code
in x.cc is tested in x_unittest.cc (or browsertest or ...), rather than some
other class' test file. I had thought based on what you said earlier in the
thread that there were unit tests for the functionality in BeginDownload. But
I'm now guessing that we have no unittests for BeginDownload (i.e. answering the
question "Did we correctly setup a URLRequest based on the arguments to
BeginDownload()"?), which makes me sad but isn't relevant to this CL. These
tests aren't testing code in BeginDownload; they are testing functionality
that's still in ResourceDispatcherHostImpl, specifically at
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
and
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch....
So they should be in resource_dispatcher_host_unittest.cc.
In a perfect world, those tests would/should probably be written in terms of
BeginUrlRequest rather than going through "helper functions" in other
compilation units, but that's the kind of thing I don't feel that strongly
about. But I do want to keep tests for code grouped with that code in the
corresponding ...tests.cc, and to my eye these tests are testing code in RDHI.
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
File content/browser/download/save_file_manager.cc (right):
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
content/browser/download/save_file_manager.cc:20: #include
"content/browser/loader/resource_request_info_impl.h"
Still needed?
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo...
content/browser/download/save_file_manager.cc:25: #include
"content/public/browser/resource_context.h"
Forward decl?
https://codereview.chromium.org/2251643003/diff/430001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):
https://codereview.chromium.org/2251643003/diff/430001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2324: // if yes then
move the code below outside the if block.
Looking at this code, if the blob logic should apply to SaveAs, I don't see why
it can't be in BeginRequestInternal rather than in all these feeder functions.
(Not relevant for this CL; just sharing the thought.)
https://codereview.chromium.org/2251643003/diff/430001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):
https://codereview.chromium.org/2251643003/diff/430001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:304: // This function
should be called for invoking the BeginURLRequest() function
nit, typo?: "for" -> "before"?
LGTM, rubberstamp, deferring to Randy. https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl.cc:573: } Seems like this check should probably also go in RDH, too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl.cc:573: } On 2016/08/25 21:03:09, mmenke (busy) wrote: > Seems like this check should probably also go in RDH, too. Yes, except that then BeginUrlRequest() would have to return the information about why the request was rejected, which would require some sort of enum, possibly created just for that function, enumerating the various reasons it could fail. That's why the is_shutdown() check is in this function as well; that struck me as cleaner than creating a new enum just to return back the failure information. WRT shutdown, I thought about repurposing net::Error, but that doesn't really have a "this failed due to shutdown" return. Though net::ERR_INVALID_URL might serve here.
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl.cc:33: #include "content/browser/loader/throttling_resource_handler.h" On 2016/08/25 20:58:49, Randy Smith - Not in Fridays wrote: > Are these still needed? Removed https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl.cc:35: #include "content/browser/resource_context_impl.h" On 2016/08/25 20:58:49, Randy Smith - Not in Fridays wrote: > I think this could be replaced by a forward decl? Removed https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl.cc:47: #include "content/public/browser/resource_throttle.h" On 2016/08/25 20:58:49, Randy Smith - Not in Fridays wrote: > Still needed? Removed https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl.cc:573: } On 2016/08/25 21:17:45, Randy Smith - Not in Fridays wrote: > On 2016/08/25 21:03:09, mmenke (busy) wrote: > > Seems like this check should probably also go in RDH, too. > > Yes, except that then BeginUrlRequest() would have to return the information > about why the request was rejected, which would require some sort of enum, > possibly created just for that function, enumerating the various reasons it > could fail. That's why the is_shutdown() check is in this function as well; > that struck me as cleaner than creating a new enum just to return back the > failure information. > > WRT shutdown, I thought about repurposing net::Error, but that doesn't really > have a "this failed due to shutdown" return. Though net::ERR_INVALID_URL might > serve here. Will look into this in a followup patch https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... File content/browser/download/download_manager_impl_unittest.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/download_manager_impl_unittest.cc:1010: On 2016/08/25 20:58:50, Randy Smith - Not in Fridays wrote: > Wow, I'm sorry, I guess I'm doing a poor job of expressing what I'm looking for. > > > The invariant I feel strongly about that I'd like this CL to keep is that code > in x.cc is tested in x_unittest.cc (or browsertest or ...), rather than some > other class' test file. I had thought based on what you said earlier in the > thread that there were unit tests for the functionality in BeginDownload. But > I'm now guessing that we have no unittests for BeginDownload (i.e. answering the > question "Did we correctly setup a URLRequest based on the arguments to > BeginDownload()"?), which makes me sad but isn't relevant to this CL. These > tests aren't testing code in BeginDownload; they are testing functionality > that's still in ResourceDispatcherHostImpl, specifically at > https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch... > and > https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch.... > So they should be in resource_dispatcher_host_unittest.cc. > > In a perfect world, those tests would/should probably be written in terms of > BeginUrlRequest rather than going through "helper functions" in other > compilation units, but that's the kind of thing I don't feel that strongly > about. But I do want to keep tests for code grouped with that code in the > corresponding ...tests.cc, and to my eye these tests are testing code in RDHI. Based on our discussion, keeping the test in resource_dispatcher_host_unittest.cc https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... File content/browser/download/save_file_manager.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/save_file_manager.cc:20: #include "content/browser/loader/resource_request_info_impl.h" On 2016/08/25 20:58:50, Randy Smith - Not in Fridays wrote: > Still needed? Thanks. Removed https://codereview.chromium.org/2251643003/diff/430001/content/browser/downlo... content/browser/download/save_file_manager.cc:25: #include "content/public/browser/resource_context.h" On 2016/08/25 20:58:50, Randy Smith - Not in Fridays wrote: > Forward decl? We cannot forward decl as code is calling the GetRequestContext() function in the ResourceContext class. https://codereview.chromium.org/2251643003/diff/430001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2324: // if yes then move the code below outside the if block. On 2016/08/25 20:58:50, Randy Smith - Not in Fridays wrote: > Looking at this code, if the blob logic should apply to SaveAs, I don't see why > it can't be in BeginRequestInternal rather than in all these feeder functions. > (Not relevant for this CL; just sharing the thought.) Thanks. Will keep that in mind. Please file a bug with your comments so we can address them going forward.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2251643003/#ps450001 (title: "Address next round of review comments and keep the download test in resource_dispatcher_host_unitte…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 ========== to ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 TBR=jam ==========
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 TBR=jam ========== to ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 TBR=jam ==========
Message was sent while issue was closed.
Committed patchset #24 (id:450001)
Message was sent while issue was closed.
Description was changed from ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 TBR=jam ========== to ========== Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl This is a continuation of the work to make files in content/browser/loader like resource_dispatcher_host_imp.cc/.h not depend on code outside of the loader folder. The callers of the BeginSaveFile and BeginDownload methods which are SaveFileManager and DownloadManagerImpl now implement the preamble code in the old BeginSaveFile and BeginDownload methods. These callers directly call the new ResourceDispatcherHostImpl::BeginURLRequest method (Previously ResourceDispatcherHostImpl::BeginRequestInternal). Additionally ResourceDispatcherHostImpl now has a new function InitializeURLRequest which needs to be called before BeginURLRequest. This function sets the referrer and associates the request with the ResourceRequestInfoImpl structure. I have left the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function as is for now. This function is invoked from content/browser/loader/mime_type_handler.cc file which requires an instance of the DownloadResourceHandler object to continue. The knowledge of download specific handlers has moved out to a callback function DownloadResourceHandler::Create which is registered with ResourceDispatcherHostImpl in the constructor. I agree that this is a touch ugly. There are tests which expect to override the ResourceDispatcherHostImpl::CreateResourceHandlerForDownload function. Did not want to break those tests in this patchset. Added TODO's in the code However the RDH interface is still a work in progress. There are other annoyances in ResourceDispatcherHostImpl today like the CreateRequestInfo function which is invoked in the BeginSaveFile and BeginDownload code path. I have left this as is for now. Will look into a better way in a subsequent patch. BUG=598073 TBR=jam Committed: https://crrev.com/c82dd5be1176dd5c8220f7dfd43afb369a585215 Cr-Commit-Position: refs/heads/master@{#414614} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/c82dd5be1176dd5c8220f7dfd43afb369a585215 Cr-Commit-Position: refs/heads/master@{#414614} |
