Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1418663010:
Adding WebContent-free Download (Closed)
DescriptionAdding WebContent-free Download
BUG=225901, 7648
Committed: https://crrev.com/6901c9042900fb14aa061369a1064cd293a628f6
Cr-Commit-Position: refs/heads/master@{#364414}
Patch Set 1 #Patch Set 2 : Fix header. #Patch Set 3 : Using DownloadResourceHandler directly. #Patch Set 4 : Cleaning up DRH. #Patch Set 5 : Cleaning up headers. #
Total comments: 30
Patch Set 6 : Fixing comments. #Patch Set 7 : Fixing test failures. #Patch Set 8 : Fix typo. #Patch Set 9 : Fixing Mock for GetBrowserContext. #
Total comments: 14
Patch Set 10 : Fixing ownership and comments. #Patch Set 11 : Migrating out to download_request_core. #Patch Set 12 : Rebase. #
Total comments: 30
Patch Set 13 : Removing Core. #
Total comments: 5
Patch Set 14 : Fixing nits. #Patch Set 15 : Fixing nits. #
Total comments: 1
Messages
Total messages: 32 (10 generated)
svaldez@chromium.org changed reviewers: + asanka@chromium.org
Forgot to send this last week. No need to review it yet, mostly just FYI. This CL currently supports downloads without a WebContents, however I still need to port more of the checks and metrics from ResourceLoader and DownloadResourceHandler over, as well as fixing bugs for other platforms.
Re-using some chunks of DownloadResourceHandler to reduce code re-use. This should be ready to review, currently it doesn't have any UMA monitoring for download resumption, but if we want to add that, we'd probably want to do it as a separate CL. We'll probably also want to add more testing, though that should probably be done once your Download browsertest CL lands.
Description was changed from ========== Adding WebContent-free Download BUG= ========== to ========== Adding WebContent-free Download BUG=225901 ==========
Another alternative is to split out the logic for handling the HTTP response for a download request from DownloadResourceHandler into a separate handler. Then the URLDownloader and DownloadResourceHandler can both call it to handle request events. This way you won't need to deal with the additional work of populating the ResourceResponse. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_item_impl.cc:511: In addition, let's check that the download URL SchemeIsHTTPOrHTTPS(). Those are the only ones we know how to resume. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_item_impl.cc:1708: GetBrowserContext()->GetResourceContext())); Do we need to worry about storage partitions here? https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_manager_impl.cc:56: DownloadManager* download_manager) { Since this is in the download guts, you can use DownloadManagerImpl. That way we can pass around a WeakPtr<>. The WeakPtr<> asserts that its dereferenced on the correct thread in addition to making it possible to account for shutdown races. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_manager_impl.cc:127: if (has_web_contents) { Rather than passing around this boolean, you can check the renderer/routing IDs to determine whether we should use RDH or not. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_manager_impl.cc:138: params->content_initiated(), params->resource_context(), For this case, content_initiated should always be false. do_not_prompt_for_login should be true. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_resource_handler.cc:71: download_manager = parent_download_manager; I'd do this the other way. I.e. if parent_download_manager is non-nullptr, then use it without poking at info->request_handle at all. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_resource_handler.h:84: void set_parent_download_manager(DownloadManager* download_manager) { Let's call this download_manager. The 'parent' relationship already sort-of exists between an off-the-record profile and its parent profile and their respective download managers. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:29: void SetReferrerForRequest(net::URLRequest* request, const Referrer& referrer) { For the URL downloader case, since the request is not originating from a document, it also doesn't have a source for referrer policy. We can require that the any required policies already be applied to the referrer_url and use it as-is. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:281: bool defer; Initialize. OnResponseStarted() isn't required to initialize this. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:285: } Probably want to DCHECK(!defer) if we don't expect it to happen. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:316: StartReading(true); // Read the next chunk. Shall we call this ReadNextChunk() ? https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:332: { Unnecessary { https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:377: { handler_->OnResponseCompleted(request_->status(), security_info, &defer); } Unnecessary { https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/url_downloader.h (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.h:38: void OnResponseStarted(net::URLRequest* request) override; Add a comment noting that this is the URLRequest::Delegate implementation. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.h:40: void OnReadCompleted(net::URLRequest* request, int bytes_read) override; We should also deal with OnReceivedRedirect() and prevent the request from redirecting. URLDownloader doesn't have all the logic necessary to sanitize and handle a redirect. Nor should we allow any redirects for a download resumption.
https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_item_impl.cc:511: On 2015/11/20 19:57:46, asanka wrote: > In addition, let's check that the download URL SchemeIsHTTPOrHTTPS(). Those are > the only ones we know how to resume. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_item_impl.cc:1708: GetBrowserContext()->GetResourceContext())); On 2015/11/20 19:57:46, asanka wrote: > Do we need to worry about storage partitions here? Not sure. I think this defaults in the correct way, but otherwise I'm not sure what storage partition we'd need to use, since we don't have an active site. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_manager_impl.cc:56: DownloadManager* download_manager) { On 2015/11/20 19:57:46, asanka wrote: > Since this is in the download guts, you can use DownloadManagerImpl. That way we > can pass around a WeakPtr<>. The WeakPtr<> asserts that its dereferenced on the > correct thread in addition to making it possible to account for shutdown races. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_manager_impl.cc:127: if (has_web_contents) { On 2015/11/20 19:57:46, asanka wrote: > Rather than passing around this boolean, you can check the renderer/routing IDs > to determine whether we should use RDH or not. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_manager_impl.cc:138: params->content_initiated(), params->resource_context(), On 2015/11/20 19:57:46, asanka wrote: > For this case, content_initiated should always be false. do_not_prompt_for_login > should be true. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_resource_handler.cc:71: download_manager = parent_download_manager; On 2015/11/20 19:57:46, asanka wrote: > I'd do this the other way. I.e. if parent_download_manager is non-nullptr, then > use it without poking at info->request_handle at all. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/download_resource_handler.h:84: void set_parent_download_manager(DownloadManager* download_manager) { On 2015/11/20 19:57:46, asanka wrote: > Let's call this download_manager. The 'parent' relationship already sort-of > exists between an off-the-record profile and its parent profile and their > respective download managers. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:29: void SetReferrerForRequest(net::URLRequest* request, const Referrer& referrer) { On 2015/11/20 19:57:46, asanka wrote: > For the URL downloader case, since the request is not originating from a > document, it also doesn't have a source for referrer policy. We can require that > the any required policies already be applied to the referrer_url and use it > as-is. > Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:281: bool defer; On 2015/11/20 19:57:46, asanka wrote: > Initialize. OnResponseStarted() isn't required to initialize this. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:285: } On 2015/11/20 19:57:46, asanka wrote: > Probably want to DCHECK(!defer) if we don't expect it to happen. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:316: StartReading(true); // Read the next chunk. On 2015/11/20 19:57:46, asanka wrote: > Shall we call this ReadNextChunk() ? Using ResourceLoader naming scheme. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:332: { On 2015/11/20 19:57:46, asanka wrote: > Unnecessary { Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.cc:377: { handler_->OnResponseCompleted(request_->status(), security_info, &defer); } On 2015/11/20 19:57:46, asanka wrote: > Unnecessary { Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... File content/browser/download/url_downloader.h (right): https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.h:38: void OnResponseStarted(net::URLRequest* request) override; On 2015/11/20 19:57:46, asanka wrote: > Add a comment noting that this is the URLRequest::Delegate implementation. Done. https://codereview.chromium.org/1418663010/diff/80001/content/browser/downloa... content/browser/download/url_downloader.h:40: void OnReadCompleted(net::URLRequest* request, int bytes_read) override; On 2015/11/20 19:57:46, asanka wrote: > We should also deal with OnReceivedRedirect() and prevent the request from > redirecting. URLDownloader doesn't have all the logic necessary to sanitize and > handle a redirect. Nor should we allow any redirects for a download resumption. Done.
So haha. Funny story. All this sounded very familiar and it turns out I wrote this back in 2013 but never landed. Here's the CL https://codereview.chromium.org/23496076 The code probably doesn't apply any more (note that's still using an SVN base URL). But the DownloadUrl implementation there is still largely valid and follows the approach I was describing (i.e. rip out the guts of DRH so that we can use its functionality without being burdened by its dependencies). https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_item_impl.cc:508: return IsDownloadResumptionEnabled() && GetURL().SchemeIsHTTPOrHTTPS() && Nit: You can move the SchemeIs.. check into it's own line above the GetResumeMode() call and return early. That way you can document why we are limiting ourselves to http/s. We may revisit this decision since we still have the option to completely restart the request for non http/s downloads, but that'll be done separately once we have http/s working. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_item_impl.cc:1704: GetOriginalUrl()); Oh boy. I just noticed that we are calling GetOriginalUrl() :-(. The validators we are setting below (ETag and Last-Modified) only apply to the final URL (i.e. GetURL()). This should be addressed in a separate CL, so don't worry about it for this one. I'll push one out that's dependent on the test refactor so that the test is easier to write. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_item_impl_unittest.cc:64: return NULL; Nit: nullptr for new code. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_manager_impl.cc:131: params->render_view_host_routing_id(), We'd need to check what the status is for downloads using these IDs directly. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_resource_handler.cc:255: download_manager_.get(), Can't de-reference a WeakPtr on a thread other that its home thread. We'd need to pass in the WeakPtr as-is to StartOnUIThread and have StartOnUIThread dereference it on the UI thread. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/url_downloader.cc:164: ResourceRequestInfoImpl* extra_info = new ResourceRequestInfoImpl( I'm still not too happy about creating this ResourceRequestInfoImpl here. I know we are putting a stop-gap in place, but it still feels like we should pull out the bits in DownloadResourceHandler that deal with request driving and response handling and invoke it directly rather than try to wrap the DRH itself. Have you checked with davidben about what he thinks about this? That said, I'm willing to yield to //content owner opinion on this.
https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_item_impl.cc:508: return IsDownloadResumptionEnabled() && GetURL().SchemeIsHTTPOrHTTPS() && On 2015/11/25 15:53:57, asanka wrote: > Nit: You can move the SchemeIs.. check into it's own line above the > GetResumeMode() call and return early. That way you can document why we are > limiting ourselves to http/s. We may revisit this decision since we still have > the option to completely restart the request for non http/s downloads, but > that'll be done separately once we have http/s working. Done. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_item_impl.cc:1704: GetOriginalUrl()); On 2015/11/25 15:53:57, asanka wrote: > Oh boy. I just noticed that we are calling GetOriginalUrl() :-(. The validators > we are setting below (ETag and Last-Modified) only apply to the final URL (i.e. > GetURL()). > > This should be addressed in a separate CL, so don't worry about it for this one. > I'll push one out that's dependent on the test refactor so that the test is > easier to write. Acknowledged. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_item_impl_unittest.cc:64: return NULL; On 2015/11/25 15:53:57, asanka wrote: > Nit: nullptr for new code. Done. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_manager_impl.cc:131: params->render_view_host_routing_id(), On 2015/11/25 15:53:57, asanka wrote: > We'd need to check what the status is for downloads using these IDs directly. Shouldn't the status already be checked when we initially call into DownloadManager, by DownloadItemImpl? https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_resource_handler.cc:255: download_manager_.get(), On 2015/11/25 15:53:57, asanka wrote: > Can't de-reference a WeakPtr on a thread other that its home thread. We'd need > to pass in the WeakPtr as-is to StartOnUIThread and have StartOnUIThread > dereference it on the UI thread. Yeah, found that out when testing. SHould be fixed. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/url_downloader.cc:164: ResourceRequestInfoImpl* extra_info = new ResourceRequestInfoImpl( On 2015/11/25 15:53:57, asanka wrote: > I'm still not too happy about creating this ResourceRequestInfoImpl here. I know > we are putting a stop-gap in place, but it still feels like we should pull out > the bits in DownloadResourceHandler that deal with request driving and response > handling and invoke it directly rather than try to wrap the DRH itself. > > Have you checked with davidben about what he thinks about this? > > That said, I'm willing to yield to //content owner opinion on this. Wouldn't we still need to create the RRII here anyway, since most of the stack later on assumes that we've got an RRII associated with the URLRequest. Otherwise we'd need to pipe this through to the DownloadManager separately. Also, after removing the request driving/response handling from DownloadResourceHandler, wouldn't DRH be reduced to just creating the DownloadCreateInfo from RRII, and passing that along to the new request driver?
https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/download_manager_impl.cc:131: params->render_view_host_routing_id(), On 2015/11/25 at 17:53:28, svaldez wrote: > On 2015/11/25 15:53:57, asanka wrote: > > We'd need to check what the status is for downloads using these IDs directly. > > Shouldn't the status already be checked when we initially call into DownloadManager, by DownloadItemImpl? Sorry, I meant check with the PlzNavigate folks on what the recommended way of doing this now that we'd like to avoid passing IDs around. https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... content/browser/download/url_downloader.cc:164: ResourceRequestInfoImpl* extra_info = new ResourceRequestInfoImpl( On 2015/11/25 at 17:53:28, svaldez wrote: > On 2015/11/25 15:53:57, asanka wrote: > > I'm still not too happy about creating this ResourceRequestInfoImpl here. I know > > we are putting a stop-gap in place, but it still feels like we should pull out > > the bits in DownloadResourceHandler that deal with request driving and response > > handling and invoke it directly rather than try to wrap the DRH itself. > > > > Have you checked with davidben about what he thinks about this? > > > > That said, I'm willing to yield to //content owner opinion on this. > > Wouldn't we still need to create the RRII here anyway, since most of the stack later on assumes that we've got an RRII associated with the URLRequest. Otherwise we'd need to pipe this through to the DownloadManager separately. Also, after removing the request driving/response handling from DownloadResourceHandler, wouldn't DRH be reduced to just creating the DownloadCreateInfo from RRII, and passing that along to the new request driver? If we are using a captive DRH, we'd need an RRII. But the suggestion is to avoid doing that. All the information we need to establish the new download can be extracted from the URLRequest. See https://codereview.chromium.org/23496076/diff/1/content/browser/download/down... which extracts the core of DRH so that we can use it from both URLDownloader and DRH.
On 2015/11/25 19:43:11, asanka wrote: > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > File content/browser/download/download_manager_impl.cc (right): > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > content/browser/download/download_manager_impl.cc:131: > params->render_view_host_routing_id(), > On 2015/11/25 at 17:53:28, svaldez wrote: > > On 2015/11/25 15:53:57, asanka wrote: > > > We'd need to check what the status is for downloads using these IDs > directly. > > > > Shouldn't the status already be checked when we initially call into > DownloadManager, by DownloadItemImpl? > > Sorry, I meant check with the PlzNavigate folks on what the recommended way of > doing this now that we'd like to avoid passing IDs around. > Sounds like currently we still need to pass it in to RDH, since the work that's been done is only at the ResourceRequestInfo layer. > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > File content/browser/download/url_downloader.cc (right): > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > content/browser/download/url_downloader.cc:164: ResourceRequestInfoImpl* > extra_info = new ResourceRequestInfoImpl( > On 2015/11/25 at 17:53:28, svaldez wrote: > > On 2015/11/25 15:53:57, asanka wrote: > > > I'm still not too happy about creating this ResourceRequestInfoImpl here. I > know > > > we are putting a stop-gap in place, but it still feels like we should pull > out > > > the bits in DownloadResourceHandler that deal with request driving and > response > > > handling and invoke it directly rather than try to wrap the DRH itself. > > > > > > Have you checked with davidben about what he thinks about this? > > > > > > That said, I'm willing to yield to //content owner opinion on this. > > > > Wouldn't we still need to create the RRII here anyway, since most of the stack > later on assumes that we've got an RRII associated with the URLRequest. > Otherwise we'd need to pipe this through to the DownloadManager separately. > Also, after removing the request driving/response handling from > DownloadResourceHandler, wouldn't DRH be reduced to just creating the > DownloadCreateInfo from RRII, and passing that along to the new request driver? > > If we are using a captive DRH, we'd need an RRII. But the suggestion is to avoid > doing that. All the information we need to establish the new download can be > extracted from the URLRequest. See > https://codereview.chromium.org/23496076/diff/1/content/browser/download/down... > which extracts the core of DRH so that we can use it from both URLDownloader and > DRH. Modifying DRH to not require RRI might be cleaner, but I suspect we might also want to pipe some amount of information through RRII anyway. For things like SSL, we need some process_id in order to store/retrieve certs.
On 2015/12/01 20:43:50, svaldez wrote: > On 2015/11/25 19:43:11, asanka wrote: > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > File content/browser/download/download_manager_impl.cc (right): > > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > content/browser/download/download_manager_impl.cc:131: > > params->render_view_host_routing_id(), > > On 2015/11/25 at 17:53:28, svaldez wrote: > > > On 2015/11/25 15:53:57, asanka wrote: > > > > We'd need to check what the status is for downloads using these IDs > > directly. > > > > > > Shouldn't the status already be checked when we initially call into > > DownloadManager, by DownloadItemImpl? > > > > Sorry, I meant check with the PlzNavigate folks on what the recommended way of > > doing this now that we'd like to avoid passing IDs around. > > > Sounds like currently we still need to pass it in to RDH, since the work that's > been done is only at the ResourceRequestInfo layer. > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > File content/browser/download/url_downloader.cc (right): > > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > content/browser/download/url_downloader.cc:164: ResourceRequestInfoImpl* > > extra_info = new ResourceRequestInfoImpl( > > On 2015/11/25 at 17:53:28, svaldez wrote: > > > On 2015/11/25 15:53:57, asanka wrote: > > > > I'm still not too happy about creating this ResourceRequestInfoImpl here. > I > > know > > > > we are putting a stop-gap in place, but it still feels like we should pull > > out > > > > the bits in DownloadResourceHandler that deal with request driving and > > response > > > > handling and invoke it directly rather than try to wrap the DRH itself. > > > > > > > > Have you checked with davidben about what he thinks about this? > > > > > > > > That said, I'm willing to yield to //content owner opinion on this. > > > > > > Wouldn't we still need to create the RRII here anyway, since most of the > stack > > later on assumes that we've got an RRII associated with the URLRequest. > > Otherwise we'd need to pipe this through to the DownloadManager separately. > > Also, after removing the request driving/response handling from > > DownloadResourceHandler, wouldn't DRH be reduced to just creating the > > DownloadCreateInfo from RRII, and passing that along to the new request > driver? > > > > If we are using a captive DRH, we'd need an RRII. But the suggestion is to > avoid > > doing that. All the information we need to establish the new download can be > > extracted from the URLRequest. See > > > https://codereview.chromium.org/23496076/diff/1/content/browser/download/down... > > which extracts the core of DRH so that we can use it from both URLDownloader > and > > DRH. > > Modifying DRH to not require RRI might be cleaner, but I suspect we might also > want to pipe some amount of information through RRII anyway. For things like > SSL, we need some process_id in order to store/retrieve certs. Talking with davidben, I'm going to copy out DRH for now for use by URLDownloader. Unless we have a strong reason to switch DRH to use the cut out section, it might make more sense to leave it as is, and wait until the PlzNavigate work simplfies the ResourceLoader story.
On 2015/12/01 21:30:42, svaldez wrote: > On 2015/12/01 20:43:50, svaldez wrote: > > On 2015/11/25 19:43:11, asanka wrote: > > > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > > File content/browser/download/download_manager_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > > content/browser/download/download_manager_impl.cc:131: > > > params->render_view_host_routing_id(), > > > On 2015/11/25 at 17:53:28, svaldez wrote: > > > > On 2015/11/25 15:53:57, asanka wrote: > > > > > We'd need to check what the status is for downloads using these IDs > > > directly. > > > > > > > > Shouldn't the status already be checked when we initially call into > > > DownloadManager, by DownloadItemImpl? > > > > > > Sorry, I meant check with the PlzNavigate folks on what the recommended way > of > > > doing this now that we'd like to avoid passing IDs around. > > > > > Sounds like currently we still need to pass it in to RDH, since the work > that's > > been done is only at the ResourceRequestInfo layer. > > > > > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > > File content/browser/download/url_downloader.cc (right): > > > > > > > > > https://codereview.chromium.org/1418663010/diff/160001/content/browser/downlo... > > > content/browser/download/url_downloader.cc:164: ResourceRequestInfoImpl* > > > extra_info = new ResourceRequestInfoImpl( > > > On 2015/11/25 at 17:53:28, svaldez wrote: > > > > On 2015/11/25 15:53:57, asanka wrote: > > > > > I'm still not too happy about creating this ResourceRequestInfoImpl > here. > > I > > > know > > > > > we are putting a stop-gap in place, but it still feels like we should > pull > > > out > > > > > the bits in DownloadResourceHandler that deal with request driving and > > > response > > > > > handling and invoke it directly rather than try to wrap the DRH itself. > > > > > > > > > > Have you checked with davidben about what he thinks about this? > > > > > > > > > > That said, I'm willing to yield to //content owner opinion on this. > > > > > > > > Wouldn't we still need to create the RRII here anyway, since most of the > > stack > > > later on assumes that we've got an RRII associated with the URLRequest. > > > Otherwise we'd need to pipe this through to the DownloadManager separately. > > > Also, after removing the request driving/response handling from > > > DownloadResourceHandler, wouldn't DRH be reduced to just creating the > > > DownloadCreateInfo from RRII, and passing that along to the new request > > driver? > > > > > > If we are using a captive DRH, we'd need an RRII. But the suggestion is to > > avoid > > > doing that. All the information we need to establish the new download can be > > > extracted from the URLRequest. See > > > > > > https://codereview.chromium.org/23496076/diff/1/content/browser/download/down... > > > which extracts the core of DRH so that we can use it from both URLDownloader > > and > > > DRH. > > > > Modifying DRH to not require RRI might be cleaner, but I suspect we might also > > want to pipe some amount of information through RRII anyway. For things like > > SSL, we need some process_id in order to store/retrieve certs. > > Talking with davidben, I'm going to copy out DRH for now for use by > URLDownloader. Unless we have a strong reason to switch DRH to use the cut out > section, it might make more sense to leave it as is, and wait until the > PlzNavigate work simplfies the ResourceLoader story. PTAL, let me know if I should switch DRH to use DownloadRequestCore. Doing so will probably require adding back some of the removed resource objects into DRC.
https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_item_impl.cc:1709: } else if (GetBrowserContext()) { This condition must always be true. DownloadItem (and by implication, DownloadItemImpl) is indirectly owned by the BrowserContext. I.e. - DownloadItemImpl is owned by DownloadManagerImpl - DownloadManagerImpl is owned by BrowserContext via UserData. We would like to make it exist without a BrowserContext, but that's a separate yak farm than this one :). https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_item_impl_unittest.cc:64: return nullptr; Why was this necessary? When testing ResumeInterruptedDownload() we should ideally be verifying that it's starting the correct URLRequest instead of aborting it by making GetBrowserContext() return nullptr. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_manager_impl.cc:240: core_ = new Core; Move this to member initialization. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_manager_impl.h (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_manager_impl.h:44: class Core { Core classes are usually used when some core functionality requires a different lifetime than the main object (E.g. we need a refcounted state to attach to some async operation whose lifetime we don't control). In the case of DownloadManagerImpl, it's less of a lifetime issue than it is thread specificity and doesn't cover the core functionality of DownloadManager. Shall we call it something else? Maybe something like IOThreadState or DownloadRequestContainer. Also, if you just have a vector of owned pointers to off thread objects for the only purpose of deleting them on the correct thread, you can use something like: std::vector<scoped_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread>> url_downloaders_; You can then have this object on the UI thread. Of course you'd need to toss a scoped_ptr<UrlDownloader> back up to the UI thread after it's created and starts running. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_request_core.cc:107: CallStartedCB(NULL, DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED); nit: nullptr for new code. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_request_core.cc:113: UMA_HISTOGRAM_TIMES("SB2.DownloadDuration", Do we have lifetime guarantee that the DownloadRequestCore would be disposed of as soon as the download completes? https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:27: scoped_ptr<UrlDownloader> UrlDownloader::BeginDownload( // static above this line. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:62: // appropriately? This was done for downloads because we wanted to treat download requests as a top-level navigation requests. I.e. the request URL at all stages is considered the first party. This class doesn't support redirects, so we can just leave the first_party_url_policy at its default (NEVER_CHANGE_FIRST_PARTY_URL) or whatever value that the caller has set. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:96: if (!request_->status().is_success()) { Nit: no { Also, do you know when this would be true? https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:121: if (request_->status().is_success()) I'm guessing you are copying logic in ResourceLoader that assumes that the request status can change during handler_->OnResponseStarted()? https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:127: void UrlDownloader::OnReadCompleted(net::URLRequest* request, int bytes_read) { Nit: let's move OnReadCompleted below StartReading(). Logically, OnReadCompleted() can't be called prior to StartReading() being called. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:145: DCHECK(!defer); This is not correct. The ByteStreamWriter can require that the reading be paused of the downstream ByteStreamReader isn't reading fast enough. If we don't propagate backpressure, we may end up bloating our read buffers and cause an unbounded amount of memory to be consumed. If the ByteStreamWriter signals that the stream be paused, OnReadCompleted() will set *defer to true, and the loader should pause avoid doing another OnWillRead/Read/OnReadCompleted cycle until the ByteStream requests that the stream be resumed. See the corresponding logic in DownloadResourceHandler. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:169: return; How does OnResponseCompleted() get called in this code path? https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:196: handler_->OnResponseCompleted(request_->status()); We should discard handler_ after this point since it's no longer useful. Also, we seem to be making assumptions that the duration of the network request is tied to the lifetime of the handler.
Also, this review cycle was unacceptably slow. Apologies since that was my fault.
https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_item_impl.cc:1709: } else if (GetBrowserContext()) { On 2015/12/08 16:58:32, asanka wrote: > This condition must always be true. DownloadItem (and by implication, > DownloadItemImpl) is indirectly owned by the BrowserContext. I.e. > > - DownloadItemImpl is owned by DownloadManagerImpl > - DownloadManagerImpl is owned by BrowserContext via UserData. > > We would like to make it exist without a BrowserContext, but that's a separate > yak farm than this one :). GetBrowserContext is null when doing unittesting. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_item_impl_unittest.cc:64: return nullptr; On 2015/12/08 16:58:32, asanka wrote: > Why was this necessary? > > When testing ResumeInterruptedDownload() we should ideally be verifying that > it's starting the correct URLRequest instead of aborting it by making > GetBrowserContext() return nullptr. Without either a WebContents or Browser/ResourceContext we can't create a URLRequest so for tests that code through the ResumeInterruptedDownload codepath, but aren't actually testing it by replacing it with MockResumeInterruptedDownload we need to be able to make a GetBrowserContext call. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_manager_impl.cc:240: core_ = new Core; On 2015/12/08 16:58:32, asanka wrote: > Move this to member initialization. Removed Core. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_manager_impl.h (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_manager_impl.h:44: class Core { On 2015/12/08 16:58:32, asanka wrote: > Core classes are usually used when some core functionality requires a different > lifetime than the main object (E.g. we need a refcounted state to attach to some > async operation whose lifetime we don't control). > > In the case of DownloadManagerImpl, it's less of a lifetime issue than it is > thread specificity and doesn't cover the core functionality of DownloadManager. > Shall we call it something else? Maybe something like IOThreadState or > DownloadRequestContainer. > > Also, if you just have a vector of owned pointers to off thread objects for the > only purpose of deleting them on the correct thread, you can use something like: > > std::vector<scoped_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread>> > url_downloaders_; > > You can then have this object on the UI thread. Of course you'd need to toss a > scoped_ptr<UrlDownloader> back up to the UI thread after it's created and starts > running. Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_request_core.cc:107: CallStartedCB(NULL, DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED); On 2015/12/08 16:58:33, asanka wrote: > nit: nullptr for new code. Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_request_core.cc:113: UMA_HISTOGRAM_TIMES("SB2.DownloadDuration", On 2015/12/08 16:58:32, asanka wrote: > Do we have lifetime guarantee that the DownloadRequestCore would be disposed of > as soon as the download completes? We don't have good lifetime guarantees about DRC's disposal. Should we? We'd need to pipe through access to the DownloadManagerImpl into DownloadItemImpl in order to do so correctly. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:27: scoped_ptr<UrlDownloader> UrlDownloader::BeginDownload( On 2015/12/08 16:58:33, asanka wrote: > // static > > above this line. Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:62: // appropriately? On 2015/12/08 16:58:33, asanka wrote: > This was done for downloads because we wanted to treat download requests as a > top-level navigation requests. I.e. the request URL at all stages is considered > the first party. > > This class doesn't support redirects, so we can just leave the > first_party_url_policy at its default (NEVER_CHANGE_FIRST_PARTY_URL) or whatever > value that the caller has set. Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:96: if (!request_->status().is_success()) { On 2015/12/08 16:58:33, asanka wrote: > Nit: no { > > Also, do you know when this would be true? Requests which are prematurely killed? Copied from ResourceLoader. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:121: if (request_->status().is_success()) On 2015/12/08 16:58:33, asanka wrote: > I'm guessing you are copying logic in ResourceLoader that assumes that the > request status can change during handler_->OnResponseStarted()? Acknowledged. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:127: void UrlDownloader::OnReadCompleted(net::URLRequest* request, int bytes_read) { On 2015/12/08 16:58:33, asanka wrote: > Nit: let's move OnReadCompleted below StartReading(). Logically, > OnReadCompleted() can't be called prior to StartReading() being called. Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:145: DCHECK(!defer); On 2015/12/08 16:58:33, asanka wrote: > This is not correct. The ByteStreamWriter can require that the reading be paused > of the downstream ByteStreamReader isn't reading fast enough. If we don't > propagate backpressure, we may end up bloating our read buffers and cause an > unbounded amount of memory to be consumed. > > If the ByteStreamWriter signals that the stream be paused, OnReadCompleted() > will set *defer to true, and the loader should pause avoid doing another > OnWillRead/Read/OnReadCompleted cycle until the ByteStream requests that the > stream be resumed. See the corresponding logic in DownloadResourceHandler. Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:169: return; On 2015/12/08 16:58:33, asanka wrote: > How does OnResponseCompleted() get called in this code path? Done. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/url_downloader.cc:196: handler_->OnResponseCompleted(request_->status()); On 2015/12/08 16:58:33, asanka wrote: > We should discard handler_ after this point since it's no longer useful. Also, > we seem to be making assumptions that the duration of the network request is > tied to the lifetime of the handler. Done.
Only nits other than BeginDownload() returning a scoper for UrlDownloader. https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_request_core.cc:113: UMA_HISTOGRAM_TIMES("SB2.DownloadDuration", On 2015/12/08 20:39:26, svaldez wrote: > On 2015/12/08 16:58:32, asanka wrote: > > Do we have lifetime guarantee that the DownloadRequestCore would be disposed > of > > as soon as the download completes? > > We don't have good lifetime guarantees about DRC's disposal. Should we? We'd > need to pipe through access to the DownloadManagerImpl into DownloadItemImpl in > order to do so correctly. This is a guarantee currently afforded by ResourceDispatcherHostImpl, but not ResourceLoader. I only mentioned it because this UMA would be inaccruate otherwise. If there's no lifetime guarantee, then I suggest removing this UMA. I'll see about removing it from DRH as well, since I think nobody's looking at it. It's also pretty misleading for resumption requests. https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... content/browser/download/download_manager_impl.cc:52: UrlDownloader* BeginDownload( You can return a scoped_ptr<UrlDownloader, DeleteOnIOThread>. That way we won't leak the object if the task gets dropped on the floor. It's valid to invoke the default constructor for a scoped_ptr<>. I.e. you can do scoped_ptr<UrlDownloader>() when you want to return a nullptr. (in case that's the reason you are returning a raw pointer). Unfortunately, scoped_ptr<>()s don't atomatically convert to another scoped_ptr<> type if the deleters aren't implicitly convertible. So you'll need to repackage the output of UrlDownloader::BeginDownload() into the right scoped_ptr<> type or have UrlDownloader::BeginDownload() return a scoped_ptr<UrlDownloader, DeleteOnIOThread>. Either is OK, but the former is preferable since it assumes that the caller takes responsibility for deleting the UrlDownloader on the correct thread. https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... content/browser/download/url_downloader.cc:202: void UrlDownloader::Resume() { Why the async invocation for ResumeReading()? Unless this class itself relies on being Resumed asynchronously, let's move the responsibility of making an async call to the caller, if necessary. That way we can just have one ResumeReading() method.
https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/1418663010/diff/220001/content/browser/downlo... content/browser/download/download_request_core.cc:113: UMA_HISTOGRAM_TIMES("SB2.DownloadDuration", On 2015/12/08 21:53:56, asanka wrote: > On 2015/12/08 20:39:26, svaldez wrote: > > On 2015/12/08 16:58:32, asanka wrote: > > > Do we have lifetime guarantee that the DownloadRequestCore would be disposed > > of > > > as soon as the download completes? > > > > We don't have good lifetime guarantees about DRC's disposal. Should we? We'd > > need to pipe through access to the DownloadManagerImpl into DownloadItemImpl > in > > order to do so correctly. > > This is a guarantee currently afforded by ResourceDispatcherHostImpl, but not > ResourceLoader. I only mentioned it because this UMA would be inaccruate > otherwise. > > If there's no lifetime guarantee, then I suggest removing this UMA. I'll see > about removing it from DRH as well, since I think nobody's looking at it. It's > also pretty misleading for resumption requests. Actually I ended up adding destruction of UrlDownloader on complete, which should destroy DRC. https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... content/browser/download/download_manager_impl.cc:52: UrlDownloader* BeginDownload( On 2015/12/08 21:53:56, asanka wrote: > You can return a scoped_ptr<UrlDownloader, DeleteOnIOThread>. That way we won't > leak the object if the task gets dropped on the floor. > > It's valid to invoke the default constructor for a scoped_ptr<>. I.e. you can do > scoped_ptr<UrlDownloader>() when you want to return a nullptr. (in case that's > the reason you are returning a raw pointer). > > Unfortunately, scoped_ptr<>()s don't atomatically convert to another > scoped_ptr<> type if the deleters aren't implicitly convertible. So you'll need > to repackage the output of UrlDownloader::BeginDownload() into the right > scoped_ptr<> type or have UrlDownloader::BeginDownload() return a > scoped_ptr<UrlDownloader, DeleteOnIOThread>. > > Either is OK, but the former is preferable since it assumes that the caller > takes responsibility for deleting the UrlDownloader on the correct thread. We can't since using PostTask... with a scoped_ptr return type results in a const scoped_ptr& which prevents us from changing its scope. https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... content/browser/download/url_downloader.cc:202: void UrlDownloader::Resume() { On 2015/12/08 21:53:56, asanka wrote: > Why the async invocation for ResumeReading()? Unless this class itself relies on > being Resumed asynchronously, let's move the responsibility of making an async > call to the caller, if necessary. > > That way we can just have one ResumeReading() method. Done.
Description was changed from ========== Adding WebContent-free Download BUG=225901 ========== to ========== Adding WebContent-free Download BUG=225901,7648 ==========
LGTM. Let's ship it. I'd prefer it if we didn't pass raw pointers across threads specially when the recipient is a Callback bound to a WeakPtr. It should be pretty easy to address. Let's have a quick chat if you are running into trouble. https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1418663010/diff/240001/content/browser/downlo... content/browser/download/download_manager_impl.cc:52: UrlDownloader* BeginDownload( On 2015/12/09 17:20:41, svaldez wrote: > On 2015/12/08 21:53:56, asanka wrote: > > You can return a scoped_ptr<UrlDownloader, DeleteOnIOThread>. That way we > won't > > leak the object if the task gets dropped on the floor. > > > > It's valid to invoke the default constructor for a scoped_ptr<>. I.e. you can > do > > scoped_ptr<UrlDownloader>() when you want to return a nullptr. (in case that's > > the reason you are returning a raw pointer). > > > > Unfortunately, scoped_ptr<>()s don't atomatically convert to another > > scoped_ptr<> type if the deleters aren't implicitly convertible. So you'll > need > > to repackage the output of UrlDownloader::BeginDownload() into the right > > scoped_ptr<> type or have UrlDownloader::BeginDownload() return a > > scoped_ptr<UrlDownloader, DeleteOnIOThread>. > > > > Either is OK, but the former is preferable since it assumes that the caller > > takes responsibility for deleting the UrlDownloader on the correct thread. > > We can't since using PostTask... with a scoped_ptr return type results in a > const scoped_ptr& which prevents us from changing its scope. PostTaskAndReplyWithResult() correctly forwards move only types. The receiving function can accept a scoped_ptr<>.
The CQ bit was checked by svaldez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418663010/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418663010/280001
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 svaldez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1418663010/#ps280001 (title: "Fixing nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418663010/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418663010/280001
Message was sent while issue was closed.
Description was changed from ========== Adding WebContent-free Download BUG=225901,7648 ========== to ========== Adding WebContent-free Download BUG=225901,7648 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Adding WebContent-free Download BUG=225901,7648 ========== to ========== Adding WebContent-free Download BUG=225901,7648 Committed: https://crrev.com/6901c9042900fb14aa061369a1064cd293a628f6 Cr-Commit-Position: refs/heads/master@{#364414} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/6901c9042900fb14aa061369a1064cd293a628f6 Cr-Commit-Position: refs/heads/master@{#364414}
Message was sent while issue was closed.
jochen@chromium.org changed reviewers: + jochen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1418663010/diff/280001/content/browser/downlo... File content/browser/download/url_downloader.cc (right): https://codereview.chromium.org/1418663010/diff/280001/content/browser/downlo... content/browser/download/url_downloader.cc:42: request->SetReferrer(referrer.url.spec()); this is broken. compare with https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch... |
