Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(664)

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.

Description

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}

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… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+327 lines, -191 lines) Patch
M content/browser/browser_main_loop.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +8 lines, -1 line 0 comments Download
M content/browser/download/download_manager_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +16 lines, -0 lines 0 comments Download
M content/browser/download/download_manager_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +68 lines, -8 lines 0 comments Download
M content/browser/download/download_resource_handler.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +10 lines, -0 lines 0 comments Download
M content/browser/download/download_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +9 lines, -0 lines 0 comments Download
M content/browser/download/save_file_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +55 lines, -3 lines 0 comments Download
M content/browser/loader/DEPS View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 9 chunks +56 lines, -24 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 10 chunks +96 lines, -147 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +9 lines, -6 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 136 (82 generated)
ananta
Please take an initial glance before I add other reviewers.
4 years, 4 months ago (2016-08-17 01:45:28 UTC) #3
Randy Smith (Not in Mondays)
Quick drive by review. I'll try to come back and do a more thorough review ...
4 years, 4 months ago (2016-08-17 15:53:54 UTC) #17
Randy Smith (Not in Mondays)
(Also: Yay for doing this! I very much approve of the goal, I just know ...
4 years, 4 months ago (2016-08-17 15:54:51 UTC) #18
ananta
https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc#newcode68 content/browser/download/download_manager_impl.cc:68: bool must_download) { On 2016/08/17 15:53:54, Randy Smith - ...
4 years, 4 months ago (2016-08-17 20:31:36 UTC) #20
ananta
https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc#newcode109 content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest( On 2016/08/17 15:53:54, Randy Smith ...
4 years, 4 months ago (2016-08-18 01:53:25 UTC) #24
Randy Smith (Not in Mondays)
https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc#newcode109 content/browser/download/download_manager_impl.cc:109: DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest( On 2016/08/18 01:53:25, ananta wrote: ...
4 years, 4 months ago (2016-08-18 15:33:57 UTC) #29
mmenke
Driveby comments https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode655 content/browser/loader/resource_dispatcher_host_impl.cc:655: return std::unique_ptr<ResourceHandler>(); Returning a nullptr looks like ...
4 years, 4 months ago (2016-08-18 17:35:44 UTC) #31
scottmg
https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/2251643003/diff/100001/content/browser/loader/resource_dispatcher_host_impl.cc#oldcode43 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/resource_dispatcher_host_impl.h File content/browser/loader/resource_dispatcher_host_impl.h (left): ...
4 years, 4 months ago (2016-08-18 17:38:08 UTC) #32
Randy Smith (Not in Mondays)
More detailed review. The biggest issue is that I'm not sure I'm ok with the ...
4 years, 4 months ago (2016-08-18 17:38:58 UTC) #34
ananta
On 2016/08/18 15:33:57, Randy Smith - Not in Fridays wrote: > https://codereview.chromium.org/2251643003/diff/60001/content/browser/download/download_manager_impl.cc > File content/browser/download/download_manager_impl.cc ...
4 years, 4 months ago (2016-08-18 21:50:21 UTC) #35
ananta
https://codereview.chromium.org/2251643003/diff/100001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/download/download_manager_impl.cc#newcode73 content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/18 17:38:57, Randy Smith - ...
4 years, 4 months ago (2016-08-18 22:27:10 UTC) #36
svaldez
Some comments from a quick pass. https://codereview.chromium.org/2251643003/diff/240001/content/browser/browser_main_loop.cc File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/browser_main_loop.cc#newcode1297 content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); Consider migrating ...
4 years, 4 months ago (2016-08-19 17:05:14 UTC) #52
ananta
https://codereview.chromium.org/2251643003/diff/240001/content/browser/browser_main_loop.cc File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2251643003/diff/240001/content/browser/browser_main_loop.cc#newcode1297 content/browser/browser_main_loop.cc:1297: DownloadManagerImpl::ResourceDispatcherHostCreated(); On 2016/08/19 17:05:14, svaldez wrote: > Consider migrating ...
4 years, 4 months ago (2016-08-19 19:02:14 UTC) #53
ananta
I moved the PostTask for the InitializeDownloadTabInfoOnUIThread function to the DownloadResourceHandler::OnResponseStarted function. OnWillStarted does not ...
4 years, 4 months ago (2016-08-19 22:30:43 UTC) #54
ananta
On 2016/08/19 22:30:43, ananta wrote: > I moved the PostTask for the InitializeDownloadTabInfoOnUIThread function to ...
4 years, 4 months ago (2016-08-19 23:44:10 UTC) #59
Randy Smith (Not in Mondays)
On 2016/08/18 21:50:21, ananta wrote: > On 2016/08/18 15:33:57, Randy Smith - Not in Fridays ...
4 years, 4 months ago (2016-08-21 22:58:20 UTC) #64
Randy Smith (Not in Mondays)
Next round of comments. I think we're moving closer to a meeting of minds around ...
4 years, 4 months ago (2016-08-21 23:32:57 UTC) #65
mmenke
Oh, and don't wait for a signoff from me on this review - I defer ...
4 years, 4 months ago (2016-08-22 14:40:08 UTC) #67
ananta
https://codereview.chromium.org/2251643003/diff/100001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/download/download_manager_impl.cc#newcode73 content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/21 23:32:57, Randy Smith - ...
4 years, 4 months ago (2016-08-22 19:14:58 UTC) #68
ananta
On 2016/08/21 22:58:20, Randy Smith - Not in Fridays wrote: > On 2016/08/18 21:50:21, ananta ...
4 years, 4 months ago (2016-08-22 19:16:00 UTC) #69
ananta
+jam for content owners
4 years, 4 months ago (2016-08-22 22:00:51 UTC) #75
jam
This looks fine from my end, makes sense to split it up into small changes ...
4 years, 4 months ago (2016-08-22 22:29:46 UTC) #76
Randy Smith (Not in Mondays)
On 2016/08/22 22:29:46, jam wrote: > This looks fine from my end, makes sense to ...
4 years, 4 months ago (2016-08-22 22:34:29 UTC) #77
ananta
On 2016/08/22 22:34:29, Randy Smith - Not in Fridays wrote: > On 2016/08/22 22:29:46, jam ...
4 years, 4 months ago (2016-08-23 01:50:33 UTC) #84
ananta
https://codereview.chromium.org/2251643003/diff/100001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/100001/content/browser/download/download_manager_impl.cc#newcode73 content/browser/download/download_manager_impl.cc:73: if (ResourceDispatcherHostImpl::Get()->delegate()) { On 2016/08/22 19:14:57, ananta wrote: > ...
4 years, 4 months ago (2016-08-23 04:27:38 UTC) #87
Randy Smith (Not in Mondays)
Response to comments. As noted below, I think we should talk interactively; I'm not sure ...
4 years, 4 months ago (2016-08-23 19:54:45 UTC) #88
Randy Smith (Not in Mondays)
On 2016/08/23 01:50:33, ananta wrote: > On 2016/08/22 22:34:29, Randy Smith - Not in Fridays ...
4 years, 4 months ago (2016-08-23 20:02:57 UTC) #89
Randy Smith (Not in Mondays)
Quick summary of an offline conversation between ananta@ and I; ananta@ please correct if I ...
4 years, 4 months ago (2016-08-23 21:22:14 UTC) #90
mmenke
I think we should step back for a moment, and think about what API we ...
4 years, 4 months ago (2016-08-23 21:35:03 UTC) #91
mmenke
On 2016/08/23 21:35:03, mmenke wrote: > I think we should step back for a moment, ...
4 years, 4 months ago (2016-08-23 21:42:52 UTC) #92
ananta
On 2016/08/23 21:42:52, mmenke wrote: > On 2016/08/23 21:35:03, mmenke wrote: > > I think ...
4 years, 4 months ago (2016-08-23 21:49:41 UTC) #93
Randy Smith (Not in Mondays)
On 2016/08/23 21:35:03, mmenke wrote: > I think we should step back for a moment, ...
4 years, 4 months ago (2016-08-23 21:51:28 UTC) #94
Randy Smith (Not in Mondays)
Offline conversation updates: Matt and I had a long conversation offline, the upshot of which ...
4 years, 4 months ago (2016-08-23 22:53:47 UTC) #95
ananta
On 2016/08/23 22:53:47, Randy Smith - Not in Fridays wrote: > Offline conversation updates: Matt ...
4 years, 4 months ago (2016-08-24 01:57:18 UTC) #99
Randy Smith (Not in Mondays)
This basic approach looks good to me. I still need to review the tests in ...
4 years, 4 months ago (2016-08-24 14:34:21 UTC) #102
mmenke
I don't think I'm going to have time to look at this today. For now, ...
4 years, 4 months ago (2016-08-24 14:47:35 UTC) #103
mmenke
On 2016/08/24 14:47:35, mmenke wrote: > I don't think I'm going to have time to ...
4 years, 4 months ago (2016-08-24 14:50:15 UTC) #104
Randy Smith (Not in Mondays)
Ananta: I'm on bug triage today and tomorrow, and that plus the chance that Matt ...
4 years, 4 months ago (2016-08-24 16:56:45 UTC) #105
ananta
https://codereview.chromium.org/2251643003/diff/410001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/download/download_manager_impl.cc#newcode585 content/browser/download/download_manager_impl.cc:585: } On 2016/08/24 14:34:21, Randy Smith - Not in ...
4 years, 4 months ago (2016-08-24 18:52:03 UTC) #106
ananta
https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode2654 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 ...
4 years, 4 months ago (2016-08-24 18:56:07 UTC) #107
mmenke
On 2016/08/24 14:47:35, mmenke (busy) wrote: > I don't think I'm going to have time ...
4 years, 4 months ago (2016-08-24 20:37:04 UTC) #108
mmenke
On 2016/08/24 20:37:04, mmenke (busy) wrote: > On 2016/08/24 14:47:35, mmenke (busy) wrote: > > ...
4 years, 3 months ago (2016-08-25 14:44:08 UTC) #109
mmenke
Ok, now that the crash has mysteriously vanished, I'm fine with landing this (Modulo layering ...
4 years, 3 months ago (2016-08-25 19:10:25 UTC) #110
Randy Smith (Not in Mondays)
https://codereview.chromium.org/2251643003/diff/410001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/download/download_manager_impl.cc#newcode585 content/browser/download/download_manager_impl.cc:585: } On 2016/08/25 19:10:25, mmenke (busy) wrote: > On ...
4 years, 3 months ago (2016-08-25 19:12:38 UTC) #111
ananta
https://codereview.chromium.org/2251643003/diff/410001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/410001/content/browser/download/download_manager_impl.cc#newcode585 content/browser/download/download_manager_impl.cc:585: } On 2016/08/25 19:10:25, mmenke (busy) wrote: > On ...
4 years, 3 months ago (2016-08-25 20:03:01 UTC) #112
Randy Smith (Not in Mondays)
Other than the test misunderstanding (:-{) and a couple of nits I'm fine with this. ...
4 years, 3 months ago (2016-08-25 20:58:50 UTC) #115
mmenke
LGTM, rubberstamp, deferring to Randy. https://codereview.chromium.org/2251643003/diff/430001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/download/download_manager_impl.cc#newcode573 content/browser/download/download_manager_impl.cc:573: } Seems like this ...
4 years, 3 months ago (2016-08-25 21:03:09 UTC) #116
Randy Smith (Not in Mondays)
https://codereview.chromium.org/2251643003/diff/430001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/download/download_manager_impl.cc#newcode573 content/browser/download/download_manager_impl.cc:573: } On 2016/08/25 21:03:09, mmenke (busy) wrote: > Seems ...
4 years, 3 months ago (2016-08-25 21:17:45 UTC) #119
ananta
https://codereview.chromium.org/2251643003/diff/430001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2251643003/diff/430001/content/browser/download/download_manager_impl.cc#newcode33 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 ...
4 years, 3 months ago (2016-08-25 22:20:50 UTC) #122
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251643003/450001
4 years, 3 months ago (2016-08-26 01:05:05 UTC) #127
commit-bot: I haz the power
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_presubmit/builds/246672)
4 years, 3 months ago (2016-08-26 01:12:52 UTC) #129
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251643003/450001
4 years, 3 months ago (2016-08-26 01:17:14 UTC) #132
commit-bot: I haz the power
Committed patchset #24 (id:450001)
4 years, 3 months ago (2016-08-26 01:22:13 UTC) #134
commit-bot: I haz the power
4 years, 3 months ago (2016-08-26 01:25:48 UTC) #136
Message was sent while issue was closed.
Patchset 24 (id:??) landed as
https://crrev.com/c82dd5be1176dd5c8220f7dfd43afb369a585215
Cr-Commit-Position: refs/heads/master@{#414614}

Powered by Google App Engine
This is Rietveld 408576698