Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
148133007:
[Downloads] Always call DM::StartDownload() for explicit downloads. (Closed)
|
Created:
6 years, 11 months ago by asanka Modified:
4 years, 10 months ago CC:
benjhayden+dwatch_chromium.org, chromium-reviews, Charlie Harrison, darin-cc_chromium.org, jam, joi+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Downloads] Always call DM::StartDownload() for explicit downloads.
Every explicitly initiated download should be represented in the UI with
the exception of downloads which are hidden or downloads which were
cancelled by the user or were blocked by a throttle. This is currently
not true for downloads whose requests fail before a response is
available (e.g. because of the hostname not being resolved) because the
DownloadManager is only notified during
DownloadResourceHandler::OnResponseStarted(). Failure to notify the
DownloadManager results in no DownloadItem being created, and
subsequently no UI being presented to the user.
The current behavior is also non-ideal for downloads resumption. When a
partial download request receives a HTTP 412 response, the current logic
invokes DownloadManager::StartDownload() with no indication that the
Content-* headers received do not apply to the desired entity. Therefore
the DownloadItem's ETag and Last-Modified headers can be incorrectly
reset after a pre-condition failure.
This CL makes the following changes:
- DownloadItem::Start() now consumes a DownloadCreateInfo since multiple
requests can be associated with a single DownloadItem due to
resumption. Each new request results in a Start() call.
- DownloadResourceHandler invokes DownloadManager::StartDownload() even
when no OnResponseStarted is received. The only exception is if the
associated renderer goes away between the time the download request is
sent and the time the response is received.
- UrlDownloader always invokes DownloadManager::StartDownload() for all
initiated download requests without exception.
- DownloadItemImpl invokes the delegate and requests a filename whenever
one is needed.
- DownloadTargetDeterminer generates a valid filename even if the
DownloadItem is not in-progress.
- DownloadUrlParameters::OnStartedCallback could now receive a valid
DownloadItem even when the request fails.
- ResourceDispatcherHost::BeginDownload() is no longer a public API. All
wannabe downloaders must now go through DownloadManager to initiate a
download.
- ResourceDispatcherHostImpl is no longer responsible for invoking
OnStartedCallback or pass through download parameters.
DownloadRequestCore attaches the data direcly to explicit downloads.
Ideally, explicit download initiation would first start by creating a
DownloadItem. We aren't quite there yet due to the fact that consumers
of the download system make assumptions about the existence of a
download item. I.e. the fallacy that the existence of a download item
must mean that that download is a candidate to be presented to the UI or
other consumers. Once these consumers are fixed, then we can move all
explicit download request initiated into DownloadItem, thus simplifying
much of this logic.
BUG=7648
Committed: https://crrev.com/00b621f5126d538df488090c19175ee892a7161b
Cr-Commit-Position: refs/heads/master@{#376487}
Patch Set 1 : #Patch Set 2 : #
Total comments: 42
Patch Set 3 : Address comments. #Patch Set 4 : Address comments. #Patch Set 5 : Resurrect! #Patch Set 6 : A few more tests. #Patch Set 7 : Also handle failure paths from ResourceDispatcherHostImpl #Patch Set 8 : Moar tests #Patch Set 9 : #
Total comments: 51
Patch Set 10 : Resolve race with download target determination and address comments. #Patch Set 11 : Deal with downloads that are blocked by throttles. #
Total comments: 14
Patch Set 12 : Ketchup with upstream. #
Total comments: 37
Patch Set 13 : #Patch Set 14 : Ok fine. INTERRUPTED_PENDING_TARGET should map to IN_PROGRESS :-P #
Total comments: 3
Patch Set 15 : Address comments #
Total comments: 16
Patch Set 16 : Comment updates #
Total comments: 21
Patch Set 17 : Comments #
Total comments: 5
Patch Set 18 : restore OnStartedCallback behavior. #
Total comments: 4
Patch Set 19 : Fix typos #Messages
Total messages: 53 (11 generated)
A combination of nits and high-level state transition questions/concerns. If it's easier, you're free to ignore the nits :-}. A few questions that will aid me in my next round of review: * What's the rationale for DTD always generating a filename? So that the DI exposes a reasonable guess at target filename if it starts in an interrupted state? * In this case, would we regenerate when we resume, or would we use the already generated filename? * How about after browser restart/restore from history? If you don't have the answers on the tip of your mind, I can figure them out from the code--I'm just trying to save myself a little bit of time, since I know I'll want to know the answers to those questions for my next round of review. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1213: DCHECK(download_file_.get()); Ok, I'm confused. It seems to me that if we're not interrupted at this point, we've got a download file. Also, we're about to do a PostTask() to the DF in a couple of lines. Why remove the DCHECK? Just because it's obvious and we'll trip in an obvious way below if the download_file_ is null? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1446: if (state_ == IN_PROGRESS_INTERNAL) { So this change is removing a bit of redundancy from the code, in a way I'm not completely comfortable with. I'd rather be able to tell all "important" things about the DI state from the state_ variable, as opposed to having extra booleans implied based on null/non-null of various variables. The only condition I think of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have a download_file_ or request_handle_ is between creation and start, which was historically handled by not doing any updates in that (so that no-one could call back into the DI). What problems do you see in going back to that? Is it only the transition around construction->Start()? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1706: weak_ptr_factory_.GetWeakPtr())); Isn't this assuming that neither of the fall-through cases in which DownloadResourceHandler is created but not invoked is going to happen? Is that safe, even for resumption? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:345: // handle. This sounds like it's trying to handle Pause() requests that come in between the OnDownloadCreated() and the call to Start(). But in the normal case (non-resume, no problems starting the download) it seems like we really want this to work for consumers; the only notification they're going to get is OnDownloadCreated(), because it's created in the IN_PROGRESS state. What is a consumer that wants to pause on start supposed to do? (This is an equivalent comment to my comment in dmi.cc about moving the OnDownloadCreated() notification.) https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1134: OnDownloadFileInitialized(new_create_info.interrupt_reason); nit: Might want to change the name if we're expanding the contexts in which it's called? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:425: info->save_info->file_stream.Pass(), Just curious: Why make this an explicit argument? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:443: // request, and an interrupted download being recreated from history. How are you dealing with the issue in the deleted comment that used to be below, i.e. making sure Cancel&etc. work after the consumers get ahold of the DownloadItem? The DownloadFile isn't bound at this point. I'll note that I'm feeling a bit pulled towards a state transition diagram where we always start in INTERRUPTED and transition to IN_PROGRESS in Start() if that's appropriate. That would fit with the above concern, and I think keep the state diagram cleaner. It would either mean we need to teach consumers to expect that in observation of download items, or we mask it somehow (maybe with a STARTING state that acts like INTERRUPTED from our perspective). WDYT? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:443: // request, and an interrupted download being recreated from history. As an alternative to the above comment, we could just create the download item in the proper state by adding an argument to CreateActiveItem() above. Given that that means we won't be doing an IN_PROGRESS->INTERRUPTED state transition, it might solve the question of when to post the OnDownloadCreated() notification. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:443: // request, and an interrupted download being recreated from history. The above thoughts bring up a side issue: When we're resuming a download, and the incoming DCI has a non-NONE DIR, does the consumer see an updated transition when going from INTERRUPTED->INTERRUPTED with a new DIR? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:452: on_started.Run(download, info->interrupt_reason); nit: This violates the contract on started_cb described in DownloadUrlParameters; that comment should be fixed. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:38: // DownloadResourceHandler members from the UI thread. nit: This comment seems outdated; it implies that this is a class method. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:174: response->head.content_length > 0 ? response->head.content_length : 0; nit, question: It feels a little funny to be both playing around in the HttpResponseHeaders and reaching into the blink based content structures; is there a reason to use response-> here instead of getting the same information from headers? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:225: &info->content_disposition); nit, suggestion: It makes some sense to me to switch around the order in which this stuff is done, but I'm not sure why creation of the ByteStream should be in the middle of setting various fields on the info; before or after makes more sense to me? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:279: base::Passed(&stream_reader), nit: I think it'd be clearer to cons up a null one on the fly. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:517: if (!download_manager_notified_ && !started_cb_.is_null()) DCHECK opportunity? Shouldn't these two conditionals be in sync always? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:521: base::Bind(started_cb_, Is the eventual intent to get rid of started_cb? (Mostly asking for curiosity and because I don't like multiple ways of exposing errors, but if it is, a TODO() might be useful doc of the intent.)
On 2014/03/18 21:07:24, rdsmith wrote: > A combination of nits and high-level state transition questions/concerns. If > it's easier, you're free to ignore the nits :-}. > > A few questions that will aid me in my next round of review: > * What's the rationale for DTD always generating a filename? So that the DI > exposes a reasonable guess at target filename if it starts in an interrupted > state? Yeah. We need a display name for the UI. > * In this case, would we regenerate when we resume, or would we use the already > generated filename? We go through the target determination on resumption. This also addresses the case where the contents on the disk may have changed requiring new uniquification etc. > * How about after browser restart/restore from history? Ditto. > If you don't have the answers on the tip of your mind, I can figure them out > from the code--I'm just trying to save myself a little bit of time, since I know > I'll want to know the answers to those questions for my next round of review. >
https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1213: DCHECK(download_file_.get()); On 2014/03/18 21:07:28, rdsmith wrote: > Ok, I'm confused. It seems to me that if we're not interrupted at this point, > we've got a download file. Also, we're about to do a PostTask() to the DF in a > couple of lines. Why remove the DCHECK? Just because it's obvious and we'll > trip in an obvious way below if the download_file_ is null? Ah. The DCHECK removal was due to an interim change that got pulled back. But trying to invoke DownloadFile::RenameAndUniquify() on a NULL DownloadFile would trip hard. I restored the DCHECK() to avoid distractions. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1446: if (state_ == IN_PROGRESS_INTERNAL) { On 2014/03/18 21:07:28, rdsmith wrote: > So this change is removing a bit of redundancy from the code, in a way I'm not > completely comfortable with. I'd rather be able to tell all "important" things > about the DI state from the state_ variable, as opposed to having extra booleans > implied based on null/non-null of various variables. The only condition I think > of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have a > download_file_ or request_handle_ is between creation and start, which was > historically handled by not doing any updates in that (so that no-one could call > back into the DI). What problems do you see in going back to that? Is it only > the transition around construction->Start()? The problem was the handling of downloads that were interrupted on start. Since I'm avoiding creating a DownloadFile for those, the initial Interrupt() call can't assume the existence of a download_file_. I suppose the problem is with the (increasingly) overloaded IN_PROGRESS_INTERNAL state not capturing the finer grained state in the DownloadItem. I see your suggestion to always start the download in an INTERRUPTED_INTERNAL state. I'm a bit apprehensive about that because we'd then be overloading the meaning of INTERRUPTED_INTERNAL which can be confusing. How about another internal state STARTING_INTERNAL which maps externally to IN_PROGRESS? Alternatively, if external consumers had a way to distinguish between new downloads and old ones being restored from history without relying on an initial IN_PROGRESS state, then we can create the download directly in an INTERRUPTED_INTERNAL state if the request failed. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1706: weak_ptr_factory_.GetWeakPtr())); On 2014/03/18 21:07:28, rdsmith wrote: > Isn't this assuming that neither of the fall-through cases in which > DownloadResourceHandler is created but not invoked is going to happen? Is that > safe, even for resumption? I verified that ResourceHandler::OnResponseCompleted() would be invoked if another handler returned false from an OnWillStart() call. The other case where the on_started callback would have been necessary is if the DownloadResourceHandler was created in vain and destroyed. In the case of resumption, we don't go through the BufferedResourceHandler where this problem exists. There's currently no API contract that we can rely on which says that Start() MUST be called if delegate_->ResumeInterruptedDownload() is called. We could enforce such a thing if: a) we don't go through the ResourceLoader stack for resumption (which is one of our long term goals), and/or b) we change the ResourceLoader stack to enforce that any ResourceHandler isn't created in vain in which case we can rely on the OnResponseCompleted() callback to schedule a Start() call. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:345: // handle. On 2014/03/18 21:07:28, rdsmith wrote: > This sounds like it's trying to handle Pause() requests that come in between the > OnDownloadCreated() and the call to Start(). But in the normal case > (non-resume, no problems starting the download) it seems like we really want > this to work for consumers; the only notification they're going to get is > OnDownloadCreated(), because it's created in the IN_PROGRESS state. What is a > consumer that wants to pause on start supposed to do? > > (This is an equivalent comment to my comment in dmi.cc about moving the > OnDownloadCreated() notification.) Hmm. Good point. We could set is_paused_ and respect the flag when we eventually receive the request_handle_. SG? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:425: info->save_info->file_stream.Pass(), On 2014/03/18 21:07:28, rdsmith wrote: > Just curious: Why make this an explicit argument? The ownership of FileStream is being passed into the DownloadFile but it can't modify the the rest of DownloadSaveInfo. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:443: // request, and an interrupted download being recreated from history. On 2014/03/18 21:07:28, rdsmith wrote: > How are you dealing with the issue in the deleted comment that used to be below, > i.e. making sure Cancel&etc. work after the consumers get ahold of the > DownloadItem? The DownloadFile isn't bound at this point. > > I'll note that I'm feeling a bit pulled towards a state transition diagram where > we always start in INTERRUPTED and transition to IN_PROGRESS in Start() if > that's appropriate. That would fit with the above concern, and I think keep the > state diagram cleaner. It would either mean we need to teach consumers to > expect that in observation of download items, or we mask it somehow (maybe with > a STARTING state that acts like INTERRUPTED from our perspective). WDYT? I've been mulling a STARTING state as mentioned elsewhere. The shuffle here is almost purely to account for the problem of external observers not being able to distinguish between new downloads that start interrupted vs. old interrupted downloads being restored from history. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:443: // request, and an interrupted download being recreated from history. On 2014/03/18 21:07:28, rdsmith wrote: > The above thoughts bring up a side issue: When we're resuming a download, and > the incoming DCI has a non-NONE DIR, does the consumer see an updated transition > when going from INTERRUPTED->INTERRUPTED with a new DIR? Yeah. The consumer is supposed to see an OnDownloadUpdated() notification. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_manager_impl.cc:452: on_started.Run(download, info->interrupt_reason); On 2014/03/18 21:07:28, rdsmith wrote: > nit: This violates the contract on started_cb described in > DownloadUrlParameters; that comment should be fixed. Done. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:38: // DownloadResourceHandler members from the UI thread. On 2014/03/18 21:07:28, rdsmith wrote: > nit: This comment seems outdated; it implies that this is a class method. Removed comment. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:174: response->head.content_length > 0 ? response->head.content_length : 0; On 2014/03/18 21:07:28, rdsmith wrote: > nit, question: It feels a little funny to be both playing around in the > HttpResponseHeaders and reaching into the blink based content structures; is > there a reason to use response-> here instead of getting the same information > from headers? The URLRequestJob provides the expected content length that percolates up to response->head.content_length. This may or may not correspond to a (possibly non-existent) Content-Length header. The HttpResponseHeaders should only be used for HTTP specific things. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:225: &info->content_disposition); On 2014/03/18 21:07:28, rdsmith wrote: > nit, suggestion: It makes some sense to me to switch around the order in which > this stuff is done, but I'm not sure why creation of the ByteStream should be in > the middle of setting various fields on the info; before or after makes more > sense to me? Done. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:279: base::Passed(&stream_reader), On 2014/03/18 21:07:28, rdsmith wrote: > nit: I think it'd be clearer to cons up a null one on the fly. Done. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:517: if (!download_manager_notified_ && !started_cb_.is_null()) On 2014/03/18 21:07:28, rdsmith wrote: > DCHECK opportunity? Shouldn't these two conditionals be in sync always? After verifying the OnWillStart() pathway, I decided to DCHECK on started_cb_ being null instead of handling this case. The justification is in the new comment. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_resource_handler.cc:521: base::Bind(started_cb_, On 2014/03/18 21:07:28, rdsmith wrote: > Is the eventual intent to get rid of started_cb? (Mostly asking for curiosity > and because I don't like multiple ways of exposing errors, but if it is, a > TODO() might be useful doc of the intent.) Yeah. I'd like to get rid of started_cb_. I added a TODO for this.
It's still possible for the OnStartedCallback to be invoked without a DownloadItem if RDH decides not to issue the request at all. I think we should still create a DownloadItem in that case so that the error could be surfaced in a consistent manner and avoid failing silently. I'll work on resolving this. I'd still like your thoughts on the download state machine (whether we should introduce a STARTING state).
Quick response before an interview so that we can continue the discussion. Noting (mostly for myself, so I get back to it) that I haven't engaged with your response at https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1446: if (state_ == IN_PROGRESS_INTERNAL) { On 2014/03/19 20:37:06, asanka wrote: > On 2014/03/18 21:07:28, rdsmith wrote: > > So this change is removing a bit of redundancy from the code, in a way I'm not > > completely comfortable with. I'd rather be able to tell all "important" > things > > about the DI state from the state_ variable, as opposed to having extra > booleans > > implied based on null/non-null of various variables. The only condition I > think > > of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have a > > download_file_ or request_handle_ is between creation and start, which was > > historically handled by not doing any updates in that (so that no-one could > call > > back into the DI). What problems do you see in going back to that? Is it > only > > the transition around construction->Start()? > > The problem was the handling of downloads that were interrupted on start. Since > I'm avoiding creating a DownloadFile for those, the initial Interrupt() call > can't assume the existence of a download_file_. > > I suppose the problem is with the (increasingly) overloaded IN_PROGRESS_INTERNAL > state not capturing the finer grained state in the DownloadItem. I see your > suggestion to always start the download in an INTERRUPTED_INTERNAL state. I'm a > bit apprehensive about that because we'd then be overloading the meaning of > INTERRUPTED_INTERNAL which can be confusing. How about another internal state > STARTING_INTERNAL which maps externally to IN_PROGRESS? We could do this, but my instinct is that we should avoid creating states unless we have to; it'll take several iterations to make sure we get the state definition and transitions correct. > Alternatively, if external consumers had a way to distinguish between new > downloads and old ones being restored from history without relying on an initial > IN_PROGRESS state, then we can create the download directly in an > INTERRUPTED_INTERNAL state if the request failed. Yeah, this is the way I'd rather go if we can. In what situations do external consumers need to know the difference between downloads that fail as they're started and downloads that are being restored in an interrupted state? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:345: // handle. On 2014/03/19 20:37:06, asanka wrote: > On 2014/03/18 21:07:28, rdsmith wrote: > > This sounds like it's trying to handle Pause() requests that come in between > the > > OnDownloadCreated() and the call to Start(). But in the normal case > > (non-resume, no problems starting the download) it seems like we really want > > this to work for consumers; the only notification they're going to get is > > OnDownloadCreated(), because it's created in the IN_PROGRESS state. What is a > > consumer that wants to pause on start supposed to do? > > > > (This is an equivalent comment to my comment in dmi.cc about moving the > > OnDownloadCreated() notification.) > > Hmm. Good point. We could set is_paused_ and respect the flag when we eventually > receive the request_handle_. SG? That's effectively accepting that we have a race and handling it. I'd rather see if we could keep the original pattern and never allow the race to happen (by not exposing the DI until it's in a state the consumer actually can play with). Let's see how our other discussions about state transitions fall out and then revisit this issue. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1134: OnDownloadFileInitialized(new_create_info.interrupt_reason); On 2014/03/18 21:07:28, rdsmith wrote: > nit: Might want to change the name if we're expanding the contexts in which it's > called? Just noting you didn't respond to this nit (I don't care a lot, just making sure it isn't dropped on the floor).
Responding to that last point I missed. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1706: weak_ptr_factory_.GetWeakPtr())); On 2014/03/19 20:37:06, asanka wrote: > On 2014/03/18 21:07:28, rdsmith wrote: > > Isn't this assuming that neither of the fall-through cases in which > > DownloadResourceHandler is created but not invoked is going to happen? Is > that > > safe, even for resumption? > > I verified that ResourceHandler::OnResponseCompleted() would be invoked if > another handler returned false from an OnWillStart() call. The other case where > the on_started callback would have been necessary is if the > DownloadResourceHandler was created in vain and destroyed. In the case of > resumption, we don't go through the BufferedResourceHandler where this problem > exists. What about the places outside DownloadResourceHandler where the started callback gets called? (E.g. ResourcDispatcherHostImp::BeginDownload()) > > There's currently no API contract that we can rely on which says that Start() > MUST be called if delegate_->ResumeInterruptedDownload() is called. We could > enforce such a thing if: a) we don't go through the ResourceLoader stack for > resumption (which is one of our long term goals), and/or b) we change the > ResourceLoader stack to enforce that any ResourceHandler isn't created in vain > in which case we can rely on the OnResponseCompleted() callback to schedule a > Start() call.
On 2014/03/20 18:04:45, rdsmith wrote: > Responding to that last point I missed. > > https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... > File content/browser/download/download_item_impl.cc (left): > > https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... > content/browser/download/download_item_impl.cc:1706: > weak_ptr_factory_.GetWeakPtr())); > On 2014/03/19 20:37:06, asanka wrote: > > On 2014/03/18 21:07:28, rdsmith wrote: > > > Isn't this assuming that neither of the fall-through cases in which > > > DownloadResourceHandler is created but not invoked is going to happen? Is > > that > > > safe, even for resumption? > > > > I verified that ResourceHandler::OnResponseCompleted() would be invoked if > > another handler returned false from an OnWillStart() call. The other case > where > > the on_started callback would have been necessary is if the > > DownloadResourceHandler was created in vain and destroyed. In the case of > > resumption, we don't go through the BufferedResourceHandler where this problem > > exists. > > What about the places outside DownloadResourceHandler where the started callback > gets called? (E.g. ResourcDispatcherHostImp::BeginDownload()) https://codereview.chromium.org/148133007/#msg5 :) > > There's currently no API contract that we can rely on which says that Start() > > MUST be called if delegate_->ResumeInterruptedDownload() is called. We could > > enforce such a thing if: a) we don't go through the ResourceLoader stack for > > resumption (which is one of our long term goals), and/or b) we change the > > ResourceLoader stack to enforce that any ResourceHandler isn't created in vain > > in which case we can rely on the OnResponseCompleted() callback to schedule a > > Start() call.
On 2014/03/21 19:10:30, asanka wrote: > On 2014/03/20 18:04:45, rdsmith wrote: > > Responding to that last point I missed. > > > > > https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... > > File content/browser/download/download_item_impl.cc (left): > > > > > https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... > > content/browser/download/download_item_impl.cc:1706: > > weak_ptr_factory_.GetWeakPtr())); > > On 2014/03/19 20:37:06, asanka wrote: > > > On 2014/03/18 21:07:28, rdsmith wrote: > > > > Isn't this assuming that neither of the fall-through cases in which > > > > DownloadResourceHandler is created but not invoked is going to happen? Is > > > that > > > > safe, even for resumption? > > > > > > I verified that ResourceHandler::OnResponseCompleted() would be invoked if > > > another handler returned false from an OnWillStart() call. The other case > > where > > > the on_started callback would have been necessary is if the > > > DownloadResourceHandler was created in vain and destroyed. In the case of > > > resumption, we don't go through the BufferedResourceHandler where this > problem > > > exists. > > > > What about the places outside DownloadResourceHandler where the started > callback > > gets called? (E.g. ResourcDispatcherHostImp::BeginDownload()) > > https://codereview.chromium.org/148133007/#msg5 :) Whoops; sorry :-{. > > > > There's currently no API contract that we can rely on which says that > Start() > > > MUST be called if delegate_->ResumeInterruptedDownload() is called. We could > > > enforce such a thing if: a) we don't go through the ResourceLoader stack for > > > resumption (which is one of our long term goals), and/or b) we change the > > > ResourceLoader stack to enforce that any ResourceHandler isn't created in > vain > > > in which case we can rely on the OnResponseCompleted() callback to schedule > a > > > Start() call.
Patchset #5 (id:480001) has been deleted
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). If the DownloadManager is not notified, no DownloadItem is created, and consequently the failure is never reported to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. Note that if the download URL was invalid and rejected, or if the ResourceHandler chain aborts the request before it starts, then a DownloadItem would still not be created. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). If the DownloadManager is not notified, no DownloadItem is created, and consequently the failure is never reported to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback. BUG=7648 ==========
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). If the DownloadManager is not notified, no DownloadItem is created, and consequently the failure is never reported to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). If the DownloadManager is not notified, no DownloadItem is created, and consequently the failure is never reported to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. BUG=7648 ==========
I resurrected this CL because it was quite necessary for fixing download resumption in the presence of captive portals or other middleboxes. I've updated the CL description to mention the changes that are introduced here. The reworking of the initiation pathway made it possible to also rework how we use ResourceDispatcherHost. It's a lot of changes, but most of it is cleanup after the main code path change and updates to callers to fix assumptions they were making about how the OnStartedCallback works.
On 2016/02/03 03:00:46, asanka wrote: > I resurrected this CL because it was quite necessary for fixing download > resumption in the presence of captive portals or other middleboxes. > > I've updated the CL description to mention the changes that are introduced here. > The reworking of the initiation pathway made it possible to also rework how we > use ResourceDispatcherHost. > > It's a lot of changes, but most of it is cleanup after the main code path change > and updates to callers to fix assumptions they were making about how the > OnStartedCallback works. I'll pull in other reviewers after we come to some agreement that the approach is sane. There's a couple of CLs that are currently dependent on this change going in, which I'll send out once this is reasonably stable.
I apologize; I've gotten my mind about halfway around this CL in concept, so you're not getting as good a review, even at the high level, as you might be. Having said that, I'd like to get you unblocked (though this CL should get a thorough review before being landed, as there are a lot of squirrelly aspects to the control flow, and I haven't managed to do that yet). Specific thoughts: * I (obviously) like the movement towards always creating a DownloadItem when a download is initiated programatically. * I like and value yanking download specific logic out of ResourceDispatcherHostImpl, but I'm a bit uncomfortable with the use of UserData on the URLRequest to avoid that plumbing. On the one hand, it's storing information the download system needs but no-one else does, which is what UserData is for. On the other hand, it's storing data that doesn't have a lot to do with the URLRequest (e.g. the SaveInfo). Overall, I think the gains outweigh the pains, but I did find myself wondering if we could move towards making both all resumed downloads and all programatic downloads be done through UrlDownloader, which would (I think) remove the need for the UserData. * I wasn't certain why the shift over to using delegate methods for communciations from the DRC to its consumer; can you share the motivation there? * I would suggest spelling out (in the CL description or the code) what gets in the way of getting rid of the started callback or at being able to guarantee that it always returns a DownloadItem. But overall, I think this approach is basically good; I don't think I'd be likely to find large things I'd want changed on a more detailed review. Random stuff below that comes up when you're trying to read a CL for the first time; you can ignore or incorporate as you wish. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:1195: DetermineDownloadTarget(); Random thought: Reading through this code, I'm struck by its similarity to the type of problem DoLoop was designed to solve (basically single threaded control flow but with places where writing the code as a single function would require sleeping waiting for async results). I wonder if we should consider rewriting this in the DoLoop() idiom. Low priority, obviously; I'm not really sure it would aid comprehension. But there's a certain value to solving the same problem the same way. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:1231: // or in-use file. Possible alternative implementation: Create a new _INTERNAL state which maps to IN_PROGRESS but the internal code knows means "fail when name is determined". https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:1721: void DownloadItemImpl::InvokeDelegateForResumption() { Why replacing the sync invocation with an async invocation? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:306: DCHECK(info->result != DOWNLOAD_INTERRUPT_REASON_NONE || stream.get()); Should this be an XOR? I..e shouldn't result != NONE && stream.get() also be a failure? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:362: if (info->result == DOWNLOAD_INTERRUPT_REASON_NONE) { nit, suggestion: I think this test is equivalent to if (stream) (DCHECK supports that but doesn't rule out stream && result != NONE, but that's the above comment). Presuming that they are equivalent, I'd rather that this entire function used only one of those tests (and checked the other condition with DCHECKs) throughout rather than "if (stream)" above and "if (result == NONE)" here for consistency. I think when people see different tests in the code, they instinctively assume tests for different things. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:364: // Create the download file and start the download. nit: The placement of this comment makes me feel like it's inaccurate, as DII::Start() is called whether or not we're in this conditional. Now DII::Start() may be called on an error resulting in the download not actually starting, but the placement of the comment and the name of the method still makes the comment confusing. https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... content/public/browser/download_url_parameters.h:120: // the failure. DownloadItem is always non-NULL. This comment contradicts our offline discussion around contexts in which failure can occur without creation of a DownloadItem.
I got most of the way through a second, medium level pass, but I have to leave now, so I'm stopping now and giving you what I have. High level notes: * Just to document our IM conversation, it looks to me as if resumption doesn't use the OnDownloadStarted() callback, which means that if a throttle kills a resumed download, we'll never know. If that's accurate, I think it's a bug. * Not really in this CL, but I'll put the idea in your head: I'm tempted to request a README.md file that describes the main objects and high level control flow for downloads. * Also not really part of this CL, but why is StartDownload() a method on the DownloadManager? It looks like it's only called from content/ https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_create_info.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_create_info.cc:24: total_bytes(0) {} Codesearch can't find a place where this is read or set in the current codebase, and I can't find anywhere where it's read in your CL (though I did see it being set below). Can we just delete this data member? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:170: const DownloadCreateInfo& info, Now that DII::Start() takes a DCI, is it possible to get rid of the one here? Conceptually it seems like it should be, but I haven't traced the code paths. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:940: DCHECK_EQ(url_chain_.back(), new_create_info.url_chain.back()); So I could be misunderstand the code, but it seems like this assumption is dependent on proper behavior of web servers, which I sorta think can't be a good idea. Specifically, IIUC, this invariant is believed true because we stored the final URL in the histogram/DownloadItemImpl on the first attempt, and thus we shouldn't get any redirects on the second attempt. But I don't think there's anything to stop the web server from sticking an extra redirect in between attempt 1 & 2? I could also imagine that we had, e.g., a network failure while we were following the redirect chain, and thus only stored an intermediate URL. This might not be true now (i.e. it might be true that on an initial download attempt any breakage during redirects won't result in a DownlodItemImpl) but if we're aiming to eventually always create a DII for any download attempt, that won't be something we can assume in the future. Thoughts? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:356: ShouldUpdateObservers should_update_observers); I'll just note that I thought of the internal/external state split as solving the same problem that this flag does (state transitions that shouldn't be exposed to consumers are between internal states that map to the same external states) and I wince a bit at having two mechanisms for the same purpose. If you still think this is needed after handling the race we talked about offline, could you share with me your thoughts on the "two mechanisms to handle the same problem" issue? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:299: void DownloadManagerImpl::StartDownload( Suggestion: I find myself wondering if we should consider a name change of some sort for this routine, given that it may or may not start the download depending on the error code that comes in. (Same thought for DII::Start()). https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:46: virtual void OnStart( Suggestion/musing: I'm chewing on naming here. There are a couple of different meanings of Start at different points in this code; starting the request, the response starting, and OnStart() being called (which can happen from OnResponseCompleted() if OnResponseStarted() was never called), so part of me wants a different name that's more specific; OnDownloadStart? OnDownloadCommit (to indicate that the download may be stillborn)? What are your thoughts? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:126: // Passthrough What does "Passthrough" mean in this context? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_resource_handler.h:100: scoped_refptr<ResourceResponse> response_; IIUC, this is being used to pass information from OnResponseStarted() to OnStart() (which is called by way of core_.OnResponseStarted()). I wince at the pattern of using a class data member to convey information to another level of the same call stack; it feels like using a global variable, though not quite as bad. I want to at least explore alternatives; would a "std::string override_mime_type" (which might be empty) argument to DownloadRequestCore::OnResponseStarted() be a reasonable alternative? https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... content/public/browser/download_url_parameters.h:59: // |render_process_host_id| and |render_view_hsot_routing_id|. Is there a way to give a level-appropriate description of when one would use the constructor and when one would use FromWebContents? Also, remind me why we don't just have two constructors instead of a constructor and a static method? I'm sure it was a decision I made way back when but today-me can't figure out what yesterday-me was thinking.
https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_create_info.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_create_info.cc:24: total_bytes(0) {} On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > Codesearch can't find a place where this is read or set in the current codebase, > and I can't find anywhere where it's read in your CL (though I did see it being > set below). Can we just delete this data member? It's set based on the Content-Length of the response and is read by the DownloadItemImpl constructor. CodeSearch may have had a temporary hiccup. It's showing both references now. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:170: const DownloadCreateInfo& info, On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > Now that DII::Start() takes a DCI, is it possible to get rid of the one here? > Conceptually it seems like it should be, but I haven't traced the code paths. It is in fact possible since the state of the download item between construction and having Start() called is only visible inside DownloadManagerImpl. And the latter doesn't care about any of the state initialized here. It's also conceptually cleaner since Start() would be the only place where we need to deal with DownloadCreateInfo. That said, the resulting refactor increases the size of this CL substantially due to how tests work. I'd like to take that on as a follow up. I added a TODO() which is just as good as cleanup. :-P https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:940: DCHECK_EQ(url_chain_.back(), new_create_info.url_chain.back()); On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > So I could be misunderstand the code, but it seems like this assumption is > dependent on proper behavior of web servers, which I sorta think can't be a good > idea. Specifically, IIUC, this invariant is believed true because we stored the > final URL in the histogram/DownloadItemImpl on the first attempt, and thus we > shouldn't get any redirects on the second attempt. But I don't think there's > anything to stop the web server from sticking an extra redirect in between > attempt 1 & 2? I could also imagine that we had, e.g., a network failure while > we were following the redirect chain, and thus only stored an intermediate URL. > This might not be true now (i.e. it might be true that on an initial download > attempt any breakage during redirects won't result in a DownlodItemImpl) but if > we're aiming to eventually always create a DII for any download attempt, that > won't be something we can assume in the future. Thoughts? This is asserted by DownloadRequestCore which interrupts with .. oh wait, that's https://codereview.chromium.org/1544603003/. In reality, any validators we've presented only work for one hop since different servers (or different URLs) don't necessarily honor the same validators. Also, an unexpected redirect is very likely the result of a meddling middlebox rather than the target server deciding not to honor the request this time around. Rather than speculate, the CL mentioned above treats it as an error. But for this round, I'm restoring the redirect handling since it should be dealt with on the other CL. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:1195: DetermineDownloadTarget(); On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > Random thought: Reading through this code, I'm struck by its similarity to the > type of problem DoLoop was designed to solve (basically single threaded control > flow but with places where writing the code as a single function would require > sleeping waiting for async results). I wonder if we should consider rewriting > this in the DoLoop() idiom. Low priority, obviously; I'm not really sure it > would aid comprehension. But there's a certain value to solving the same > problem the same way. This is true. I'm also tempted to pull out the download job handling part out of DownloadItem so that only active downloads have all the guts needed to drive a download request. But yeah, that's a CL for another day. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:1231: // or in-use file. On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > Possible alternative implementation: Create a new _INTERNAL state which maps to > IN_PROGRESS but the internal code knows means "fail when name is determined". Yup. That's what I ended up doing, partly to deal with the race condition I mentioned. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.cc:1721: void DownloadItemImpl::InvokeDelegateForResumption() { On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > Why replacing the sync invocation with an async invocation? This CL relies on Start() never being called synchronously. I added the extra async hop so that we can enforce this from within DII rather than relying on callers doing the right thing. However, we are no longer relying on that due to the extra internal states that I added for the new patchset. So I've removed the extra hop. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:356: ShouldUpdateObservers should_update_observers); On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > I'll just note that I thought of the internal/external state split as solving > the same problem that this flag does (state transitions that shouldn't be > exposed to consumers are between internal states that map to the same external > states) and I wince a bit at having two mechanisms for the same purpose. If you > still think this is needed after handling the race we talked about offline, > could you share with me your thoughts on the "two mechanisms to handle the same > problem" issue? Good point. I removed the ShouldUpdateObservers logic and made explicit UpdateObservers() calls where we are making externally visible changes. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:299: void DownloadManagerImpl::StartDownload( On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > Suggestion: I find myself wondering if we should consider a name change of some > sort for this routine, given that it may or may not start the download depending > on the error code that comes in. (Same thought for DII::Start()). DII::Start() is, I think, OK since it starts the state machine and chugs along even for an interrupted download (since it needs to determine the target). I agree that StartDownload() isn't a great name since it may not start a download. This particular method mostly just means that we have a download response. So maybe something like, OnResponseReady()? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:306: DCHECK(info->result != DOWNLOAD_INTERRUPT_REASON_NONE || stream.get()); On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > Should this be an XOR? I..e shouldn't result != NONE && stream.get() also be a > failure? Done. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:362: if (info->result == DOWNLOAD_INTERRUPT_REASON_NONE) { On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > nit, suggestion: I think this test is equivalent to if (stream) (DCHECK supports > that but doesn't rule out stream && result != NONE, but that's the above > comment). Presuming that they are equivalent, I'd rather that this entire > function used only one of those tests (and checked the other condition with > DCHECKs) throughout rather than "if (stream)" above and "if (result == NONE)" > here for consistency. I think when people see different tests in the code, they > instinctively assume tests for different things. Got it. Changed the other test to if(result == NONE) which is the important aspect we are checking. All others are side-effects. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:364: // Create the download file and start the download. On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > nit: The placement of this comment makes me feel like it's inaccurate, as > DII::Start() is called whether or not we're in this conditional. Now > DII::Start() may be called on an error resulting in the download not actually > starting, but the placement of the comment and the name of the method still > makes the comment confusing. I removed the extra comments which I thought were spurious. The DownloadFile is the other end of the bytestream. Creating it doesn't start the download. That process happens after DII::Start. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:46: virtual void OnStart( On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > Suggestion/musing: I'm chewing on naming here. There are a couple of different > meanings of Start at different points in this code; starting the request, the > response starting, and OnStart() being called (which can happen from > OnResponseCompleted() if OnResponseStarted() was never called), so part of me > wants a different name that's more specific; OnDownloadStart? OnDownloadCommit > (to indicate that the download may be stillborn)? What are your thoughts? I threw one out a bit earlier which was OnResponseReady(). If that's clear, then I can rename it along the whole stack so that it's consistent. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:126: // Passthrough On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > What does "Passthrough" mean in this context? That these fields aren't used by DownloadRequestCore. They are just here so that they can be passed along in the DownloadCreateInfo. These are some of the things that will go away when we can create the DownloadItem first. I've updated the comment to make it clearer. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_resource_handler.h:100: scoped_refptr<ResourceResponse> response_; On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > IIUC, this is being used to pass information from OnResponseStarted() to > OnStart() (which is called by way of core_.OnResponseStarted()). I wince at the > pattern of using a class data member to convey information to another level of > the same call stack; it feels like using a global variable, though not quite as > bad. I want to at least explore alternatives; would a "std::string > override_mime_type" (which might be empty) argument to > DownloadRequestCore::OnResponseStarted() be a reasonable alternative? Switched to original_mime_type which is the name we use in DownloadItem to refer to the field. https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... content/public/browser/download_url_parameters.h:59: // |render_process_host_id| and |render_view_hsot_routing_id|. On 2016/02/10 21:48:46, Randy Smith - Not in Fridays wrote: > Is there a way to give a level-appropriate description of when one would use the > constructor and when one would use FromWebContents? > > Also, remind me why we don't just have two constructors instead of a constructor > and a static method? I'm sure it was a decision I made way back when but > today-me can't figure out what yesterday-me was thinking. At the moment FromWebContents() is just a convenience, which I think is conveyed by the comment. I added comment clarifying the other constructor. https://codereview.chromium.org/148133007/diff/580001/content/public/browser/... content/public/browser/download_url_parameters.h:120: // the failure. DownloadItem is always non-NULL. On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > This comment contradicts our offline discussion around contexts in which failure > can occur without creation of a DownloadItem. Done.
On 2016/02/05 00:51:41, Randy Smith - Not in Fridays wrote: > > * I wasn't certain why the shift over to using delegate methods for > communciations from the DRC to its consumer; can you share the motivation there? Discussed offline. But the key issue is that we want to have a single code path for communicating that a download response is ready, and that code path is now using the delegate interface (which replaced a callback).
On 2016/02/10 21:48:46, Randy Smith - Not in Fridays wrote: > I got most of the way through a second, medium level pass, but I have to leave > now, so I'm stopping now and giving you what I have. High level notes: > > * Just to document our IM conversation, it looks to me as if resumption doesn't > use the OnDownloadStarted() callback, which means that if a throttle kills a > resumed download, we'll never know. If that's accurate, I think it's a bug. Agreed. I'll address it tomorrow. > * Not really in this CL, but I'll put the idea in your head: I'm tempted to > request a README.md file that describes the main objects and high level control > flow for downloads. Acknowledged. > * Also not really part of this CL, but why is StartDownload() a method on the > DownloadManager? It looks like it's only called from content I'll remove that too.
On 2016/02/11 03:48:56, asanka wrote: > On 2016/02/10 21:48:46, Randy Smith - Not in Fridays wrote: > > I got most of the way through a second, medium level pass, but I have to leave > > now, so I'm stopping now and giving you what I have. High level notes: > > > > * Just to document our IM conversation, it looks to me as if resumption > doesn't > > use the OnDownloadStarted() callback, which means that if a throttle kills a > > resumed download, we'll never know. If that's accurate, I think it's a bug. > > Agreed. I'll address it tomorrow. Done and added test (DownloadResumptionContentTest.ResumingRequestBlockedByThrottle). > > > * Not really in this CL, but I'll put the idea in your head: I'm tempted to > > request a README.md file that describes the main objects and high level > control > > flow for downloads. > > Acknowledged. > > > * Also not really part of this CL, but why is StartDownload() a method on the > > DownloadManager? It looks like it's only called from content > > I'll remove that too. Removing StartDownload ended up adding a bunch of unrelated changes to this CL. I'm going to punt that to a separate CL.
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). If the DownloadManager is not notified, no DownloadItem is created, and consequently the failure is never reported to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ==========
Ok, this counts as a review where I feel like I understand the CL to the level at which I want to review these CLs. Finally :-}. I still need to review the test code and will want to review certain pieces of code (primarily state transitions) after your responses to these comments, but that hopefully won't be too hard. All of your "reasonable idea, but putting off to a future CL" responses ACK'd and SGTM. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:356: ShouldUpdateObservers should_update_observers); On 2016/02/11 03:43:07, asanka wrote: > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > I'll just note that I thought of the internal/external state split as solving > > the same problem that this flag does (state transitions that shouldn't be > > exposed to consumers are between internal states that map to the same external > > states) and I wince a bit at having two mechanisms for the same purpose. If > you > > still think this is needed after handling the race we talked about offline, > > could you share with me your thoughts on the "two mechanisms to handle the > same > > problem" issue? > > Good point. I removed the ShouldUpdateObservers logic and made explicit > UpdateObservers() calls where we are making externally visible changes. *nod* I had forgotten that TransitionTo() had that argument, which I have the same concern about (though as I say elsewhere, I'm probably responsible for it :-}); another way to think about it is that not updating observers hides you from observers, but leaves you exposed to consumers who poll your state unexpectedly, which seems like the wrong choice. In my (current?) ideal world, we'd update observers whenever a TransitionTo() changed between internal states that corresponded to different external states, and not otherwise, and make the state diagram work with that logic. But this is really solidly "not in this CL"; you're getting these comments because this CL is <obiwan>making me engage with the downloads architecture in a way I haven't in a long time</obiwan>. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:373: ShouldUpdateObservers notify_action); This makes me architecturally happy but concerned that it's a big move with potential gotchas for this CL; if you felt like this change was too risk in this context, I'm fine with you not doing it. (See above comment for context; it was written before my eyes got here. The meta-point is that I think you and I should avoid letting me push you into making good architectural changes that aren't directly relevant to this CL, given the time constraints.) ETA: I now perceive you as having gone from this model to one in which any state transition needs to be explicitly evaluated for whether it should have an UpdateObservers() following it. Looking only at TransitionTo() I like the old situation better, as the argument list forced new uses of the function to think about the issue, where now people may put in TransitionTo()'s without thinking about it. Coupled with the Interrupt() change above, I'm not sure what the right thing is, but I'm a little tempted to go back to the state before I started making noise and suggest that you/you&I re-address this question in a future CL with an eye towards what the right overall architecture should be. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:363: DCHECK(stream.get()); Hmmm. Was there a specific reason to removing this DCHECK? I'm asking in case I'm missing something--I think of using DCHECKs as a fine way to document and keep in sync parallel expressions of the same state. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:372: if (download_file.get() && delegate_) { nit: My understanding of the style guide is that curly braces should exist around multi-line conditional clauses. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:46: virtual void OnStart( On 2016/02/11 03:43:07, asanka wrote: > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > Suggestion/musing: I'm chewing on naming here. There are a couple of > different > > meanings of Start at different points in this code; starting the request, the > > response starting, and OnStart() being called (which can happen from > > OnResponseCompleted() if OnResponseStarted() was never called), so part of me > > wants a different name that's more specific; OnDownloadStart? > OnDownloadCommit > > (to indicate that the download may be stillborn)? What are your thoughts? > > I threw one out a bit earlier which was OnResponseReady(). If that's clear, then > I can rename it along the whole stack so that it's consistent. I think I like it better, so I vote in favor, but if you have a preference for OnStart(), that's ok by me. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_resource_handler.h:100: scoped_refptr<ResourceResponse> response_; On 2016/02/11 03:43:07, asanka wrote: > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > IIUC, this is being used to pass information from OnResponseStarted() to > > OnStart() (which is called by way of core_.OnResponseStarted()). I wince at > the > > pattern of using a class data member to convey information to another level of > > the same call stack; it feels like using a global variable, though not quite > as > > bad. I want to at least explore alternatives; would a "std::string > > override_mime_type" (which might be empty) argument to > > DownloadRequestCore::OnResponseStarted() be a reasonable alternative? > > Switched to original_mime_type which is the name we use in DownloadItem to refer > to the field. Two thoughts: * Huh. Ok, but "original_mime_type_" I find a bit misleading, since the reason we're keeping this here is to handle the case where the MimeTypeResourceHandler overrode the *original* original mime type. * Happy to go with your preference, but what's your thoughs around not doing this as an argument to OnResponseStarted()? https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:85: void OnAbortedBeforeStart(DownloadInterruptReason reason); Um ... this is probably my being clueless, but I can't find callers for either of these routines in this CL (searching the raw diff). Where are they called from? https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:127: // populate the DownloadCreateInfo when the time comes. nit, suggestion: Given that we have the URLRequest and these fields are the ones we're putting in the UserData on the URLRequest, could we grab them from there when populating the DCI? This is a little wonky since the DRC makes more conceptual sense than UserData on URLRequest, but I think that storing in one place is better than storing in two. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:391: case INTERRUPTED_TARGET_PENDING_INTERNAL: This also seems incompatible with mapping to an external INTERRUPTED state; the external expectation is almost certainly that a Resume() call on the object will actually result in an resumption. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:404: UpdateObservers(); See comment elsewhere about registered observers vs. polling consumers (though I'm not certain what the right choice for this CL is, just calling out that it's an issue I'm curious as to your thoughts about WRT this change). https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:521: case INTERRUPTED_TARGET_PENDING_INTERNAL: I'm beginning to think that the mapping of this to INTERRUPTED rather than IN_PROGRESS in InternalToExternalState was a typo .... :-}. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1304: TransitionTo(TARGET_RESOLVED_INTERNAL); It looks to me as if this state isn't needed; the download item is never in it when any other code can run on the UI thread. Am I wrong? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1413: UpdateObservers(); nit, suggestion: I'd rather have these right next to the state transition functions (Interrupt in this case) so it's clear what's being broadcast to the observers. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1806: weak_ptr_factory_.InvalidateWeakPtrs(); Why doesn't this solve the race that the earlier CL was trying to fix? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1858: case INTERRUPTED_TARGET_PENDING_INTERNAL: Doesn't mapping this to interrupted leave you open to the consumer calling ResumeInterruptedDownload() and being ignored? That the behavior you want? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.h:263: INTERRUPTED_TARGET_PENDING_INTERNAL, (Discussed offline, noting for online record) Needs documentation. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_request_core.cc:453: if (started_) suggestion: DCHECK_EQ(reason, DOWNLOAD_INTERRUPT_REASON_NONE)? https://codereview.chromium.org/148133007/diff/640001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/640001/content/public/browser/... content/public/browser/download_url_parameters.h:125: // the failure. Suggestion: Is it reasonable to lay out the contexts in which the DownloadItem will and won't be null? https://codereview.chromium.org/148133007/diff/640001/content/public/browser/... content/public/browser/download_url_parameters.h:125: // the failure. suggestion, nit: I'd personally put the documentation as to the callback meaning next to the callback definition.
https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1446: if (state_ == IN_PROGRESS_INTERNAL) { On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > On 2014/03/19 20:37:06, asanka wrote: > > On 2014/03/18 21:07:28, rdsmith wrote: > > > So this change is removing a bit of redundancy from the code, in a way I'm > not > > > completely comfortable with. I'd rather be able to tell all "important" > > things > > > about the DI state from the state_ variable, as opposed to having extra > > booleans > > > implied based on null/non-null of various variables. The only condition I > > think > > > of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have a > > > download_file_ or request_handle_ is between creation and start, which was > > > historically handled by not doing any updates in that (so that no-one could > > call > > > back into the DI). What problems do you see in going back to that? Is it > > only > > > the transition around construction->Start()? > > > > The problem was the handling of downloads that were interrupted on start. > Since > > I'm avoiding creating a DownloadFile for those, the initial Interrupt() call > > can't assume the existence of a download_file_. > > > > I suppose the problem is with the (increasingly) overloaded > IN_PROGRESS_INTERNAL > > state not capturing the finer grained state in the DownloadItem. I see your > > suggestion to always start the download in an INTERRUPTED_INTERNAL state. I'm > a > > bit apprehensive about that because we'd then be overloading the meaning of > > INTERRUPTED_INTERNAL which can be confusing. How about another internal state > > STARTING_INTERNAL which maps externally to IN_PROGRESS? > > We could do this, but my instinct is that we should avoid creating states unless > we have to; it'll take several iterations to make sure we get the state > definition and transitions correct. > > > Alternatively, if external consumers had a way to distinguish between new > > downloads and old ones being restored from history without relying on an > initial > > IN_PROGRESS state, then we can create the download directly in an > > INTERRUPTED_INTERNAL state if the request failed. > > Yeah, this is the way I'd rather go if we can. In what situations do external > consumers need to know the difference between downloads that fail as they're > started and downloads that are being restored in an interrupted state? New downloads that start in a failed state need to be displayed to the user, while restored interrupted downloads don't. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1706: weak_ptr_factory_.GetWeakPtr())); On 2014/03/20 18:04:46, Randy Smith - Not in Fridays wrote: > On 2014/03/19 20:37:06, asanka wrote: > > On 2014/03/18 21:07:28, rdsmith wrote: > > > Isn't this assuming that neither of the fall-through cases in which > > > DownloadResourceHandler is created but not invoked is going to happen? Is > > that > > > safe, even for resumption? > > > > I verified that ResourceHandler::OnResponseCompleted() would be invoked if > > another handler returned false from an OnWillStart() call. The other case > where > > the on_started callback would have been necessary is if the > > DownloadResourceHandler was created in vain and destroyed. In the case of > > resumption, we don't go through the BufferedResourceHandler where this problem > > exists. > > What about the places outside DownloadResourceHandler where the started callback > gets called? (E.g. ResourcDispatcherHostImp::BeginDownload()) > > > > > There's currently no API contract that we can rely on which says that Start() > > MUST be called if delegate_->ResumeInterruptedDownload() is called. We could > > enforce such a thing if: a) we don't go through the ResourceLoader stack for > > resumption (which is one of our long term goals), and/or b) we change the > > ResourceLoader stack to enforce that any ResourceHandler isn't created in vain > > in which case we can rely on the OnResponseCompleted() callback to schedule a > > Start() call. > RDH::BeginDownload doesn't call the started callback. We do have one hole where the WebContents might go away between DII::ResumeInterruptedDownload and the response coming back. I'll work on closing that by making the DownloadManager determination not depend on the RVH. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:345: // handle. On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > On 2014/03/19 20:37:06, asanka wrote: > > On 2014/03/18 21:07:28, rdsmith wrote: > > > This sounds like it's trying to handle Pause() requests that come in between > > the > > > OnDownloadCreated() and the call to Start(). But in the normal case > > > (non-resume, no problems starting the download) it seems like we really want > > > this to work for consumers; the only notification they're going to get is > > > OnDownloadCreated(), because it's created in the IN_PROGRESS state. What is > a > > > consumer that wants to pause on start supposed to do? > > > > > > (This is an equivalent comment to my comment in dmi.cc about moving the > > > OnDownloadCreated() notification.) > > > > Hmm. Good point. We could set is_paused_ and respect the flag when we > eventually > > receive the request_handle_. SG? > > That's effectively accepting that we have a race and handling it. I'd rather > see if we could keep the original pattern and never allow the race to happen (by > not exposing the DI until it's in a state the consumer actually can play with). > Let's see how our other discussions about state transitions fall out and then > revisit this issue. But that's not correct. We are no longer in a place where it has one and only one request session. After we expose the download it's still possible for it to lose the request (interruption / auto resume / etc.) The state we expose and the capabilities associated with each state are all racy (i.e. our visible state is always subject to TOCTOU races). https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1134: OnDownloadFileInitialized(new_create_info.interrupt_reason); On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > On 2014/03/18 21:07:28, rdsmith wrote: > > nit: Might want to change the name if we're expanding the contexts in which > it's > > called? > > Just noting you didn't respond to this nit (I don't care a lot, just making sure > it isn't dropped on the floor). Whoops. Missed. I'm doing a pass through the previous patchsets to make sure I get all the comments. Rietveld UX fail. However, this comment is no longer applicable since I removed this call. OnDownloadFileInitialized is now exclusively used as a completion callback for download file initialization. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:356: ShouldUpdateObservers should_update_observers); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > On 2016/02/11 03:43:07, asanka wrote: > > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > > I'll just note that I thought of the internal/external state split as > solving > > > the same problem that this flag does (state transitions that shouldn't be > > > exposed to consumers are between internal states that map to the same > external > > > states) and I wince a bit at having two mechanisms for the same purpose. If > > you > > > still think this is needed after handling the race we talked about offline, > > > could you share with me your thoughts on the "two mechanisms to handle the > > same > > > problem" issue? > > > > Good point. I removed the ShouldUpdateObservers logic and made explicit > > UpdateObservers() calls where we are making externally visible changes. > > *nod* I had forgotten that TransitionTo() had that argument, which I have the > same concern about (though as I say elsewhere, I'm probably responsible for it > :-}); another way to think about it is that not updating observers hides you > from observers, but leaves you exposed to consumers who poll your state > unexpectedly, which seems like the wrong choice. In my (current?) ideal world, > we'd update observers whenever a TransitionTo() changed between internal states > that corresponded to different external states, and not otherwise, and make the > state diagram work with that logic. But this is really solidly "not in this > CL"; you're getting these comments because this CL is <obiwan>making me engage > with the downloads architecture in a way I haven't in a long time</obiwan>. I think the problem is that having things like TransitionTo() automatically update observers make the DownloadItem state non-deterministic. Once we call UpdateObservers, technically we don't even know whether the DownloadItem is still around let alone in the state that we specified as an argument to TransitionTo. The change I introduce in the next CL is to only invoke UpdateObservers() at points where: a) the embedder would need to know the state transition. b) we are yielding the state machine to external signals anyway, and hence there's no harm in an observer calling a method that changes state. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:373: ShouldUpdateObservers notify_action); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > This makes me architecturally happy but concerned that it's a big move with > potential gotchas for this CL; if you felt like this change was too risk in this > context, I'm fine with you not doing it. (See above comment for context; it was > written before my eyes got here. The meta-point is that I think you and I > should avoid letting me push you into making good architectural changes that > aren't directly relevant to this CL, given the time constraints.) > > ETA: I now perceive you as having gone from this model to one in which any state > transition needs to be explicitly evaluated for whether it should have an > UpdateObservers() following it. Looking only at TransitionTo() I like the old > situation better, as the argument list forced new uses of the function to think > about the issue, where now people may put in TransitionTo()'s without thinking > about it. Coupled with the Interrupt() change above, I'm not sure what the > right thing is, but I'm a little tempted to go back to the state before I > started making noise and suggest that you/you&I re-address this question in a > future CL with an eye towards what the right overall architecture should be. Hmm. I can revert if the change makes the CL harder to review. I'll wait for your input on that. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_item_impl.cc:1566: TransitionTo(CANCELLED_INTERNAL); Needs an UpdateObservers() call. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_item_impl.cc:1805: weak_ptr_factory_.InvalidateWeakPtrs(); Perhaps we should do this when the previous request / download session ended. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:391: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > This also seems incompatible with mapping to an external INTERRUPTED state; the > external expectation is almost certainly that a Resume() call on the object will > actually result in an resumption. It's worse than that, we can't trigger a resumption while a target determination is pending. I've updated CanResume() to reflect this, but we should allow resumption from this state, at least by resuming when target determination is complete. But not for this CL since the state machine changes are complicated enough as it is. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:404: UpdateObservers(); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > See comment elsewhere about registered observers vs. polling consumers (though > I'm not certain what the right choice for this CL is, just calling out that it's > an issue I'm curious as to your thoughts about WRT this change). Responded there. Basically, UpdateObservers is best called just before we exit the state machine and yield to external signals. We need to be careful about the state we leave the DII when we make async calls since then someone can examine the DII. Even then, as long as the exposed state is consistent, I'm not concerned. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:521: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > I'm beginning to think that the mapping of this to INTERRUPTED rather than > IN_PROGRESS in InternalToExternalState was a typo .... :-}. ? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1304: TransitionTo(TARGET_RESOLVED_INTERNAL); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > It looks to me as if this state isn't needed; the download item is never in it > when any other code can run on the UI thread. Am I wrong? I've tried to justify the existence of this state in download_item_impl.h. It's needed since in its absence we'd be transitioning from TARGET_PENDING_INTERNAL directly to INTERRUPTED. If that transition is allowed, then we lose the advantage of having the PENDING state. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1413: UpdateObservers(); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > nit, suggestion: I'd rather have these right next to the state transition > functions (Interrupt in this case) so it's clear what's being broadcast to the > observers. Iv'e tried to move the UpdateObservers() calls as close to an exit as possible because the state of the DII after yielding to observers is undefined. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1806: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > Why doesn't this solve the race that the earlier CL was trying to fix? The race was that a DestinationError() or DestinationCompleted() call could come in after we start target determination. The destination observer and the target determination can both be using weak pointers from the same session. However, when the call comes in, we don't have a way to tell that there's a target determination pending. The former logic was to examine the target path, but this isn't reliable for a resumption. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1858: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > Doesn't mapping this to interrupted leave you open to the consumer calling > ResumeInterruptedDownload() and being ignored? That the behavior you want? Resume() already doesn't work on an interrupted download that return false for CanResume(). E.g. not HTTP, and in the future, not a supported HTTP verb. The subtlety I'm introducing is that a download could sometimes change its CanResume() value after entering an INTERRUPTED state. Currently this is correctly handled by the UI because we update the observers when this happens and the UI only looks at the CanResume() flag. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.h:263: INTERRUPTED_TARGET_PENDING_INTERNAL, On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > (Discussed offline, noting for online record) Needs documentation. Done. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_request_core.cc:453: if (started_) On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > suggestion: DCHECK_EQ(reason, DOWNLOAD_INTERRUPT_REASON_NONE)? started_ only implies that the download started successfully. It's still possible for the download to be interrupted part way through. In that case reason would be something other than NONE, and we'd pass the state along to the delegate via the stream_writer_->Close(reason) call. However, if we didn't start successfully, then the reason for the failure must be apparent here. This is checked via the DCHECK_NE(reason, NONE) below. https://codereview.chromium.org/148133007/diff/640001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/640001/content/public/browser/... content/public/browser/download_url_parameters.h:125: // the failure. On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > suggestion, nit: I'd personally put the documentation as to the callback meaning > next to the callback definition. Done. https://codereview.chromium.org/148133007/diff/640001/content/public/browser/... content/public/browser/download_url_parameters.h:125: // the failure. On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > Suggestion: Is it reasonable to lay out the contexts in which the DownloadItem > will and won't be null? I added a brief comment explaining the distinction.
https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_browsertest.cc:1675: ResumingRequestBlockedByThrottle) { I switched to always using UrlDownloader for resumed requests. This also eliminated the hole that this test was testing. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:391: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > This also seems incompatible with mapping to an external INTERRUPTED state; > the > > external expectation is almost certainly that a Resume() call on the object > will > > actually result in an resumption. > > It's worse than that, we can't trigger a resumption while a target determination > is pending. I've updated CanResume() to reflect this, but we should allow > resumption from this state, at least by resuming when target determination is > complete. But not for this CL since the state machine changes are complicated > enough as it is. Fine! We'll go with mapping this to IN_PROGRESS :P https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:521: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > > I'm beginning to think that the mapping of this to INTERRUPTED rather than > > IN_PROGRESS in InternalToExternalState was a typo .... :-}. > > ? NM https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1858: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > > Doesn't mapping this to interrupted leave you open to the consumer calling > > ResumeInterruptedDownload() and being ignored? That the behavior you want? > > Resume() already doesn't work on an interrupted download that return false for > CanResume(). E.g. not HTTP, and in the future, not a supported HTTP verb. The > subtlety I'm introducing is that a download could sometimes change its > CanResume() value after entering an INTERRUPTED state. Currently this is > correctly handled by the UI because we update the observers when this happens > and the UI only looks at the CanResume() flag. Retracted. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:393: class DownloadItemTestWithResumption No longer need two sets of tests since there's only one resumption code path. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1711: TEST_P(DownloadItemDestinationUpdateRaceTest, DumpParameters) { This was only needed for visually verifying that we are generating all the race condition permutations. I instead moved the debug code above to the currying helpers. https://codereview.chromium.org/148133007/diff/680001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/680001/content/browser/downloa... content/browser/download/download_item_impl.cc:324: // invoking observers. Can't remove this one just yet. There's code that depends on seeing an IN_PROGRESS+SAFE state before the COMPLETE state. https://codereview.chromium.org/148133007/diff/680001/content/browser/downloa... content/browser/download/download_item_impl.cc:427: UpdateObservers(); This was also necessary since there was code that depended on seeing a CANCEL state before an OnDestroyed() call. https://codereview.chromium.org/148133007/diff/680001/content/browser/downloa... content/browser/download/download_item_impl.cc:1334: // in an underminate state after invoking observers. As above. Code exists that depends on seeing an IN_PROGRESS+(non empty GetFullPath()) before a COMPLETE.
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests themselves fail (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadManagerImpl, DownloadItemImpl, and DownloadCreateInfo updated to support creating downloads in an interrupted state. - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler notifies DownloadManager even when no OnResponseStarted is received. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ==========
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ==========
asanka@chromium.org changed reviewers: + benjhayden@chromium.org, davidben@chromium.org, sky@chromium.org
Adding owners: benjhayden: chrome/.../downloads_api.cc chrome/.../downloads_api_browsertest.cc sky: chrome/browser/extensions/webstore_installer.cc content/public/browser/download_url_parameters.h content/public/browser/resource_dispatcher_host.h content/public/test/test_download_request_handler.cc content/public/test/test_file_error_injector.cc davidben: content/browser/loader/mime_type_resource_handler.cc content/browser/loader/mime_type_resource_handler_unittest.cc content/browser/loader/resource_dispatcher_host_impl.cc content/browser/loader/resource_dispatcher_host_impl.h content/browser/loader/resource_dispatcher_host_unittest.cc
I'm not an owner of content/public. The other parts you wanted me to look at LGTM
Quick responses with some pings on comments I don't see responses to. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:356: ShouldUpdateObservers should_update_observers); On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > On 2016/02/11 03:43:07, asanka wrote: > > > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > > > I'll just note that I thought of the internal/external state split as > > solving > > > > the same problem that this flag does (state transitions that shouldn't be > > > > exposed to consumers are between internal states that map to the same > > external > > > > states) and I wince a bit at having two mechanisms for the same purpose. > If > > > you > > > > still think this is needed after handling the race we talked about > offline, > > > > could you share with me your thoughts on the "two mechanisms to handle the > > > same > > > > problem" issue? > > > > > > Good point. I removed the ShouldUpdateObservers logic and made explicit > > > UpdateObservers() calls where we are making externally visible changes. > > > > *nod* I had forgotten that TransitionTo() had that argument, which I have the > > same concern about (though as I say elsewhere, I'm probably responsible for it > > :-}); another way to think about it is that not updating observers hides you > > from observers, but leaves you exposed to consumers who poll your state > > unexpectedly, which seems like the wrong choice. In my (current?) ideal > world, > > we'd update observers whenever a TransitionTo() changed between internal > states > > that corresponded to different external states, and not otherwise, and make > the > > state diagram work with that logic. But this is really solidly "not in this > > CL"; you're getting these comments because this CL is <obiwan>making me engage > > with the downloads architecture in a way I haven't in a long time</obiwan>. > > I think the problem is that having things like TransitionTo() automatically > update observers make the DownloadItem state non-deterministic. Once we call > UpdateObservers, technically we don't even know whether the DownloadItem is > still around let alone in the state that we specified as an argument to > TransitionTo. The change I introduce in the next CL is to only invoke > UpdateObservers() at points where: > > a) the embedder would need to know the state transition. > b) we are yielding the state machine to external signals anyway, and hence > there's no harm in an observer calling a method that changes state. SGTM. I'd like to be careful (at some point; I don't think we have to worry about this now) that we're not relying on not calling UpdateObservers to save us over some other yield point, but the fact that UpdateObservers() is a yield point makes this unavoidably more complicated. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_item_impl.h:373: ShouldUpdateObservers notify_action); On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > This makes me architecturally happy but concerned that it's a big move with > > potential gotchas for this CL; if you felt like this change was too risk in > this > > context, I'm fine with you not doing it. (See above comment for context; it > was > > written before my eyes got here. The meta-point is that I think you and I > > should avoid letting me push you into making good architectural changes that > > aren't directly relevant to this CL, given the time constraints.) > > > > ETA: I now perceive you as having gone from this model to one in which any > state > > transition needs to be explicitly evaluated for whether it should have an > > UpdateObservers() following it. Looking only at TransitionTo() I like the old > > situation better, as the argument list forced new uses of the function to > think > > about the issue, where now people may put in TransitionTo()'s without thinking > > about it. Coupled with the Interrupt() change above, I'm not sure what the > > right thing is, but I'm a little tempted to go back to the state before I > > started making noise and suggest that you/you&I re-address this question in a > > future CL with an eye towards what the right overall architecture should be. > > Hmm. I can revert if the change makes the CL harder to review. I'll wait for > your input on that. I think we decided offline to let this be. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:363: DCHECK(stream.get()); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > Hmmm. Was there a specific reason to removing this DCHECK? I'm asking in case > I'm missing something--I think of using DCHECKs as a fine way to document and > keep in sync parallel expressions of the same state. Ping? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:372: if (download_file.get() && delegate_) { On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > nit: My understanding of the style guide is that curly braces should exist > around multi-line conditional clauses. Ping? https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:46: virtual void OnStart( On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > On 2016/02/11 03:43:07, asanka wrote: > > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > > Suggestion/musing: I'm chewing on naming here. There are a couple of > > different > > > meanings of Start at different points in this code; starting the request, > the > > > response starting, and OnStart() being called (which can happen from > > > OnResponseCompleted() if OnResponseStarted() was never called), so part of > me > > > wants a different name that's more specific; OnDownloadStart? > > OnDownloadCommit > > > (to indicate that the download may be stillborn)? What are your thoughts? > > > > I threw one out a bit earlier which was OnResponseReady(). If that's clear, > then > > I can rename it along the whole stack so that it's consistent. > > I think I like it better, so I vote in favor, but if you have a preference for > OnStart(), that's ok by me. Presuming not changing this was a conscious choice, which is fine, but wanting to make sure since you didn't respond. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_resource_handler.h:100: scoped_refptr<ResourceResponse> response_; On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > On 2016/02/11 03:43:07, asanka wrote: > > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > > IIUC, this is being used to pass information from OnResponseStarted() to > > > OnStart() (which is called by way of core_.OnResponseStarted()). I wince at > > the > > > pattern of using a class data member to convey information to another level > of > > > the same call stack; it feels like using a global variable, though not quite > > as > > > bad. I want to at least explore alternatives; would a "std::string > > > override_mime_type" (which might be empty) argument to > > > DownloadRequestCore::OnResponseStarted() be a reasonable alternative? > > > > Switched to original_mime_type which is the name we use in DownloadItem to > refer > > to the field. > > Two thoughts: > * Huh. Ok, but "original_mime_type_" I find a bit misleading, since the reason > we're keeping this here is to handle the case where the MimeTypeResourceHandler > overrode the *original* original mime type. > * Happy to go with your preference, but what's your thoughs around not doing > this as an argument to OnResponseStarted()? Still hoping you'll respond on this issue? https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:85: void OnAbortedBeforeStart(DownloadInterruptReason reason); On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > Um ... this is probably my being clueless, but I can't find callers for either > of these routines in this CL (searching the raw diff). Where are they called > from? Ping? git grep on a patched checkout seems to agree with the raw diff search. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:127: // populate the DownloadCreateInfo when the time comes. On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > nit, suggestion: Given that we have the URLRequest and these fields are the ones > we're putting in the UserData on the URLRequest, could we grab them from there > when populating the DCI? This is a little wonky since the DRC makes more > conceptual sense than UserData on URLRequest, but I think that storing in one > place is better than storing in two. Ping? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:391: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/12 16:06:49, asanka wrote: > On 2016/02/11 23:57:25, asanka wrote: > > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > > This also seems incompatible with mapping to an external INTERRUPTED state; > > the > > > external expectation is almost certainly that a Resume() call on the object > > will > > > actually result in an resumption. > > > > It's worse than that, we can't trigger a resumption while a target > determination > > is pending. I've updated CanResume() to reflect this, but we should allow > > resumption from this state, at least by resuming when target determination is > > complete. But not for this CL since the state machine changes are complicated > > enough as it is. > > Fine! We'll go with mapping this to IN_PROGRESS :P :-}. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1413: UpdateObservers(); On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > nit, suggestion: I'd rather have these right next to the state transition > > functions (Interrupt in this case) so it's clear what's being broadcast to the > > observers. > > Iv'e tried to move the UpdateObservers() calls as close to an exit as possible > because the state of the DII after yielding to observers is undefined. SGTM. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1806: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > > Why doesn't this solve the race that the earlier CL was trying to fix? > > The race was that a DestinationError() or DestinationCompleted() call could come > in after we start target determination. The destination observer and the target > determination can both be using weak pointers from the same session. However, > when the call comes in, we don't have a way to tell that there's a target > determination pending. The former logic was to examine the target path, but this > isn't reliable for a resumption. Right, sorry, still confused. My image was that the other, smaller CL took care of the race by making states finer grained and examining them. But it looks to me like this would also fix the race. The DE() or DC() that comes in after starting target determination would need to have been from a previous pre-resume portion of the DIIs lifetime, wouldn't it? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_request_core.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_request_core.cc:453: if (started_) On 2016/02/11 23:57:25, asanka wrote: > On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > > suggestion: DCHECK_EQ(reason, DOWNLOAD_INTERRUPT_REASON_NONE)? > > started_ only implies that the download started successfully. It's still > possible for the download to be interrupted part way through. In that case > reason would be something other than NONE, and we'd pass the state along to the > delegate via the stream_writer_->Close(reason) call. > > However, if we didn't start successfully, then the reason for the failure must > be apparent here. This is checked via the DCHECK_NE(reason, NONE) below. Acknowledged.
https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:363: DCHECK(stream.get()); On 2016/02/12 18:01:18, Randy Smith - Not in Fridays wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > Hmmm. Was there a specific reason to removing this DCHECK? I'm asking in > case > > I'm missing something--I think of using DCHECKs as a fine way to document and > > keep in sync parallel expressions of the same state. > > Ping? It was redundant with the DCHECK at the top of the function. I can reinstate it if it makes it easier to keep track of what's valid where. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:372: if (download_file.get() && delegate_) { On 2016/02/12 18:01:18, Randy Smith - Not in Fridays wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > nit: My understanding of the style guide is that curly braces should exist > > around multi-line conditional clauses. > > Ping? Ah. Done. https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_request_core.h:46: virtual void OnStart( On 2016/02/12 18:01:18, Randy Smith - Not in Fridays wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > On 2016/02/11 03:43:07, asanka wrote: > > > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > > > Suggestion/musing: I'm chewing on naming here. There are a couple of > > > different > > > > meanings of Start at different points in this code; starting the request, > > the > > > > response starting, and OnStart() being called (which can happen from > > > > OnResponseCompleted() if OnResponseStarted() was never called), so part of > > me > > > > wants a different name that's more specific; OnDownloadStart? > > > OnDownloadCommit > > > > (to indicate that the download may be stillborn)? What are your thoughts? > > > > > > I threw one out a bit earlier which was OnResponseReady(). If that's clear, > > then > > > I can rename it along the whole stack so that it's consistent. > > > > I think I like it better, so I vote in favor, but if you have a preference for > > OnStart(), that's ok by me. > > Presuming not changing this was a conscious choice, which is fine, but wanting > to make sure since you didn't respond. Oops. I missed this in my previous pass. Shall I do the rename in a follow up? It involves some churn since we are also trying to get rid of the DownloadManager::StartDownload() interface (which is part of the rename stack). https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_resource_handler.h (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_resource_handler.h:100: scoped_refptr<ResourceResponse> response_; On 2016/02/12 18:01:18, Randy Smith - Not in Fridays wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > On 2016/02/11 03:43:07, asanka wrote: > > > On 2016/02/10 21:48:45, Randy Smith - Not in Fridays wrote: > > > > IIUC, this is being used to pass information from OnResponseStarted() to > > > > OnStart() (which is called by way of core_.OnResponseStarted()). I wince > at > > > the > > > > pattern of using a class data member to convey information to another > level > > of > > > > the same call stack; it feels like using a global variable, though not > quite > > > as > > > > bad. I want to at least explore alternatives; would a "std::string > > > > override_mime_type" (which might be empty) argument to > > > > DownloadRequestCore::OnResponseStarted() be a reasonable alternative? > > > > > > Switched to original_mime_type which is the name we use in DownloadItem to > > refer > > > to the field. > > > > Two thoughts: > > * Huh. Ok, but "original_mime_type_" I find a bit misleading, since the > reason > > we're keeping this here is to handle the case where the > MimeTypeResourceHandler > > overrode the *original* original mime type. > > * Happy to go with your preference, but what's your thoughs around not doing > > this as an argument to OnResponseStarted()? > > Still hoping you'll respond on this issue? Went with an extra argument to OnResponseStarted(). https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:85: void OnAbortedBeforeStart(DownloadInterruptReason reason); On 2016/02/12 18:01:19, Randy Smith - Not in Fridays wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > Um ... this is probably my being clueless, but I can't find callers for either > > of these routines in this CL (searching the raw diff). Where are they called > > from? > > Ping? git grep on a patched checkout seems to agree with the raw diff search. Removed. There are for the dependent CL. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:127: // populate the DownloadCreateInfo when the time comes. On 2016/02/12 18:01:19, Randy Smith - Not in Fridays wrote: > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > nit, suggestion: Given that we have the URLRequest and these fields are the > ones > > we're putting in the UserData on the URLRequest, could we grab them from there > > when populating the DCI? This is a little wonky since the DRC makes more > > conceptual sense than UserData on URLRequest, but I think that storing in one > > place is better than storing in two. > > Ping? I started out doing that, but it ended up not being quite as clean as I wanted because of requests that don't go through CreateRequestOnIOThread() https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1806: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/02/12 18:01:19, Randy Smith - Not in Fridays wrote: > On 2016/02/11 23:57:25, asanka wrote: > > On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > > > Why doesn't this solve the race that the earlier CL was trying to fix? > > > > The race was that a DestinationError() or DestinationCompleted() call could > come > > in after we start target determination. The destination observer and the > target > > determination can both be using weak pointers from the same session. However, > > when the call comes in, we don't have a way to tell that there's a target > > determination pending. The former logic was to examine the target path, but > this > > isn't reliable for a resumption. > > Right, sorry, still confused. My image was that the other, smaller CL took care > of the race by making states finer grained and examining them. But it looks to > me like this would also fix the race. The DE() or DC() that comes in after > starting target determination would need to have been from a previous pre-resume > portion of the DIIs lifetime, wouldn't it? It could be the same. The workflow is: - initialize download file. ----- [1] - initialize download file completes. [ download observer stuff happens ] - delegate starts download target determination [ download observer stuff happens ] - delegate determines download target - start intermediate rename [ download observer stuff happens ] - end intermediate rename ----- [2] The destination observation calls can start coming through after [1]. [2] is when we officially conclude target determination, and the first point where successful new downloads have both a target and a current path.
Some further quick responses (I hadn't seen your responses to the old PS comments previously). I'm not blocked on anything here. Top level question (copied from IM): Where are you currently WRT solving the problem of the WebContents going away while we're pinging a resumption off the IO thread/RDH? Is that currently in this CL? Do you intend it to be? https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1446: if (state_ == IN_PROGRESS_INTERNAL) { On 2016/02/11 23:57:24, asanka wrote: > On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > > On 2014/03/19 20:37:06, asanka wrote: > > > On 2014/03/18 21:07:28, rdsmith wrote: > > > > So this change is removing a bit of redundancy from the code, in a way I'm > > not > > > > completely comfortable with. I'd rather be able to tell all "important" > > > things > > > > about the DI state from the state_ variable, as opposed to having extra > > > booleans > > > > implied based on null/non-null of various variables. The only condition I > > > think > > > > of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have a > > > > download_file_ or request_handle_ is between creation and start, which was > > > > historically handled by not doing any updates in that (so that no-one > could > > > call > > > > back into the DI). What problems do you see in going back to that? Is it > > > only > > > > the transition around construction->Start()? > > > > > > The problem was the handling of downloads that were interrupted on start. > > Since > > > I'm avoiding creating a DownloadFile for those, the initial Interrupt() call > > > can't assume the existence of a download_file_. > > > > > > I suppose the problem is with the (increasingly) overloaded > > IN_PROGRESS_INTERNAL > > > state not capturing the finer grained state in the DownloadItem. I see your > > > suggestion to always start the download in an INTERRUPTED_INTERNAL state. > I'm > > a > > > bit apprehensive about that because we'd then be overloading the meaning of > > > INTERRUPTED_INTERNAL which can be confusing. How about another internal > state > > > STARTING_INTERNAL which maps externally to IN_PROGRESS? > > > > We could do this, but my instinct is that we should avoid creating states > unless > > we have to; it'll take several iterations to make sure we get the state > > definition and transitions correct. > > > > > Alternatively, if external consumers had a way to distinguish between new > > > downloads and old ones being restored from history without relying on an > > initial > > > IN_PROGRESS state, then we can create the download directly in an > > > INTERRUPTED_INTERNAL state if the request failed. > > > > Yeah, this is the way I'd rather go if we can. In what situations do external > > consumers need to know the difference between downloads that fail as they're > > started and downloads that are being restored in an interrupted state? > > New downloads that start in a failed state need to be displayed to the user, > while restored interrupted downloads don't. I feel absolutely no need to resolve this old philosophical issue for this CL, but I do still hold the generic opinions expressed two years ago :-}. Let's chat offline at some point about what set of guidelines we want to have for managing DII state going forward. It's far more complicated than I'd ideally like, and that may or may not be inevitable. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:345: // handle. On 2016/02/11 23:57:24, asanka wrote: > On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > > On 2014/03/19 20:37:06, asanka wrote: > > > On 2014/03/18 21:07:28, rdsmith wrote: > > > > This sounds like it's trying to handle Pause() requests that come in > between > > > the > > > > OnDownloadCreated() and the call to Start(). But in the normal case > > > > (non-resume, no problems starting the download) it seems like we really > want > > > > this to work for consumers; the only notification they're going to get is > > > > OnDownloadCreated(), because it's created in the IN_PROGRESS state. What > is > > a > > > > consumer that wants to pause on start supposed to do? > > > > > > > > (This is an equivalent comment to my comment in dmi.cc about moving the > > > > OnDownloadCreated() notification.) > > > > > > Hmm. Good point. We could set is_paused_ and respect the flag when we > > eventually > > > receive the request_handle_. SG? > > > > That's effectively accepting that we have a race and handling it. I'd rather > > see if we could keep the original pattern and never allow the race to happen > (by > > not exposing the DI until it's in a state the consumer actually can play > with). > > Let's see how our other discussions about state transitions fall out and then > > revisit this issue. > > But that's not correct. We are no longer in a place where it has one and only > one request session. After we expose the download it's still possible for it to > lose the request (interruption / auto resume / etc.) The state we expose and the > capabilities associated with each state are all racy (i.e. our visible state is > always subject to TOCTOU races). Ok. I'm good with your race handling suggestion earlier in the thread. Is that what's now reflected in the code? ("TOCTOU"?) https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/148133007/diff/580001/content/browser/downloa... content/browser/download/download_manager_impl.cc:363: DCHECK(stream.get()); On 2016/02/12 18:31:46, asanka wrote: > On 2016/02/12 18:01:18, Randy Smith - Not in Fridays wrote: > > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > > Hmmm. Was there a specific reason to removing this DCHECK? I'm asking in > > case > > > I'm missing something--I think of using DCHECKs as a fine way to document > and > > > keep in sync parallel expressions of the same state. > > > > Ping? > > It was redundant with the DCHECK at the top of the function. I can reinstate it > if it makes it easier to keep track of what's valid where. Nah, I'm good. Thanks for the pointer. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_item_impl.cc:1566: TransitionTo(CANCELLED_INTERNAL); On 2016/02/11 23:57:25, asanka wrote: > Needs an UpdateObservers() call. This looks like a note you made to yourself that you didn't execute on; status? https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_item_impl.cc:1805: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/02/11 23:57:25, asanka wrote: > Perhaps we should do this when the previous request / download session ended. I think we do (given that Interrupt() does a ReleaseDownloadFile() if we have a download_file_ pointer and the only way we can lose a download_file_ pointer is through ReleaseDownloadFile()). Are you seeing some way to have an outstanding valid weak pointer at this point in the code? https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:127: // populate the DownloadCreateInfo when the time comes. On 2016/02/12 18:31:46, asanka wrote: > On 2016/02/12 18:01:19, Randy Smith - Not in Fridays wrote: > > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > > nit, suggestion: Given that we have the URLRequest and these fields are the > > ones > > > we're putting in the UserData on the URLRequest, could we grab them from > there > > > when populating the DCI? This is a little wonky since the DRC makes more > > > conceptual sense than UserData on URLRequest, but I think that storing in > one > > > place is better than storing in two. > > > > Ping? > > I started out doing that, but it ended up not being quite as clean as I wanted > because of requests that don't go through CreateRequestOnIOThread() Ok with this not being in this CL, but: Aren't those exactly the requests where these fields aren't needed (i.e. the ones in which we're getting a new request from the renderers, and thus none of save_info_, id, or the callback is used)? https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl.cc:1806: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/02/12 18:31:47, asanka wrote: > On 2016/02/12 18:01:19, Randy Smith - Not in Fridays wrote: > > On 2016/02/11 23:57:25, asanka wrote: > > > On 2016/02/11 22:08:09, Randy Smith - Not in Fridays wrote: > > > > Why doesn't this solve the race that the earlier CL was trying to fix? > > > > > > The race was that a DestinationError() or DestinationCompleted() call could > > come > > > in after we start target determination. The destination observer and the > > target > > > determination can both be using weak pointers from the same session. > However, > > > when the call comes in, we don't have a way to tell that there's a target > > > determination pending. The former logic was to examine the target path, but > > this > > > isn't reliable for a resumption. > > > > Right, sorry, still confused. My image was that the other, smaller CL took > care > > of the race by making states finer grained and examining them. But it looks > to > > me like this would also fix the race. The DE() or DC() that comes in after > > starting target determination would need to have been from a previous > pre-resume > > portion of the DIIs lifetime, wouldn't it? > > It could be the same. The workflow is: > > - initialize download file. ----- [1] > - initialize download file completes. > > [ download observer stuff happens ] > > - delegate starts download target determination > > [ download observer stuff happens ] > > - delegate determines download target > - start intermediate rename > > [ download observer stuff happens ] > > - end intermediate rename ----- [2] > > The destination observation calls can start coming through after [1]. [2] is > when we officially conclude target determination, and the first point where > successful new downloads have both a target and a current path. Ah, got it. Thanks! https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_request_core.h:58: // Invokes Delegate::OnStart() with download start parameters. nit: comment use of parameter.
Sending out an intermediate set of comments because of the potential blocker in the new hole created by moving UpdateObservers() below; I figure giving you maximal time to deal with that is a virtue. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_browsertest.cc:1675: ResumingRequestBlockedByThrottle) { On 2016/02/12 16:06:49, asanka wrote: > I switched to always using UrlDownloader for resumed requests. This also > eliminated the hole that this test was testing. Acknowledged. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:393: class DownloadItemTestWithResumption On 2016/02/12 16:06:49, asanka wrote: > No longer need two sets of tests since there's only one resumption code path. Acknowledged. https://codereview.chromium.org/148133007/diff/640001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:1711: TEST_P(DownloadItemDestinationUpdateRaceTest, DumpParameters) { On 2016/02/12 16:06:49, asanka wrote: > This was only needed for visually verifying that we are generating all the race > condition permutations. I instead moved the debug code above to the currying > helpers. Acknowledged. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:324: // invoking observers. I presume there are no observers that do anything like this currently? Are the observers that will be seriously confused by moving this after MaybeCompleteDownload()? Or does that mess up dangerous download processing? (This seems like a big enough hole to be worth a bug and a fix sometime soon, though not introduced in this CL, so not blocking.) https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:1334: // in an underminate state after invoking observers. Unlike the other place where you have the same comment, this one concerns me for this review, because this is a semantic change for this CL that increases danger. I'd like to have some assurance that this problem isn't an actual problem before stamp.
https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:1334: // in an underminate state after invoking observers. On 2016/02/12 19:10:28, Randy Smith - Not in Fridays wrote: > Unlike the other place where you have the same comment, this one concerns me for > this review, because this is a semantic change for this CL that increases > danger. I'd like to have some assurance that this problem isn't an actual > problem before stamp. Semantics are the same? I.e. both before and after the change, DII ends up calling UpdateObserver() prior to MaybeCompleteDownload().
https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:1446: if (state_ == IN_PROGRESS_INTERNAL) { On 2016/02/12 18:58:10, Randy Smith - Not in Fridays wrote: > On 2016/02/11 23:57:24, asanka wrote: > > On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > > > On 2014/03/19 20:37:06, asanka wrote: > > > > On 2014/03/18 21:07:28, rdsmith wrote: > > > > > So this change is removing a bit of redundancy from the code, in a way > I'm > > > not > > > > > completely comfortable with. I'd rather be able to tell all "important" > > > > things > > > > > about the DI state from the state_ variable, as opposed to having extra > > > > booleans > > > > > implied based on null/non-null of various variables. The only condition > I > > > > think > > > > > of offhand in which the DI is in IN_PROGRESS_INTERNAL and we don't have > a > > > > > download_file_ or request_handle_ is between creation and start, which > was > > > > > historically handled by not doing any updates in that (so that no-one > > could > > > > call > > > > > back into the DI). What problems do you see in going back to that? Is > it > > > > only > > > > > the transition around construction->Start()? > > > > > > > > The problem was the handling of downloads that were interrupted on start. > > > Since > > > > I'm avoiding creating a DownloadFile for those, the initial Interrupt() > call > > > > can't assume the existence of a download_file_. > > > > > > > > I suppose the problem is with the (increasingly) overloaded > > > IN_PROGRESS_INTERNAL > > > > state not capturing the finer grained state in the DownloadItem. I see > your > > > > suggestion to always start the download in an INTERRUPTED_INTERNAL state. > > I'm > > > a > > > > bit apprehensive about that because we'd then be overloading the meaning > of > > > > INTERRUPTED_INTERNAL which can be confusing. How about another internal > > state > > > > STARTING_INTERNAL which maps externally to IN_PROGRESS? > > > > > > We could do this, but my instinct is that we should avoid creating states > > unless > > > we have to; it'll take several iterations to make sure we get the state > > > definition and transitions correct. > > > > > > > Alternatively, if external consumers had a way to distinguish between new > > > > downloads and old ones being restored from history without relying on an > > > initial > > > > IN_PROGRESS state, then we can create the download directly in an > > > > INTERRUPTED_INTERNAL state if the request failed. > > > > > > Yeah, this is the way I'd rather go if we can. In what situations do > external > > > consumers need to know the difference between downloads that fail as they're > > > started and downloads that are being restored in an interrupted state? > > > > New downloads that start in a failed state need to be displayed to the user, > > while restored interrupted downloads don't. > > I feel absolutely no need to resolve this old philosophical issue for this CL, > but I do still hold the generic opinions expressed two years ago :-}. Let's > chat offline at some point about what set of guidelines we want to have for > managing DII state going forward. It's far more complicated than I'd ideally > like, and that may or may not be inevitable. Acknowledged. https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/420001/content/browser/downloa... content/browser/download/download_item_impl.cc:345: // handle. On 2016/02/12 18:58:10, Randy Smith - Not in Fridays wrote: > On 2016/02/11 23:57:24, asanka wrote: > > On 2014/03/20 15:24:54, Randy Smith - Not in Fridays wrote: > > > On 2014/03/19 20:37:06, asanka wrote: > > > > On 2014/03/18 21:07:28, rdsmith wrote: > > > > > This sounds like it's trying to handle Pause() requests that come in > > between > > > > the > > > > > OnDownloadCreated() and the call to Start(). But in the normal case > > > > > (non-resume, no problems starting the download) it seems like we really > > want > > > > > this to work for consumers; the only notification they're going to get > is > > > > > OnDownloadCreated(), because it's created in the IN_PROGRESS state. > What > > is > > > a > > > > > consumer that wants to pause on start supposed to do? > > > > > > > > > > (This is an equivalent comment to my comment in dmi.cc about moving the > > > > > OnDownloadCreated() notification.) > > > > > > > > Hmm. Good point. We could set is_paused_ and respect the flag when we > > > eventually > > > > receive the request_handle_. SG? > > > > > > That's effectively accepting that we have a race and handling it. I'd > rather > > > see if we could keep the original pattern and never allow the race to happen > > (by > > > not exposing the DI until it's in a state the consumer actually can play > > with). > > > Let's see how our other discussions about state transitions fall out and > then > > > revisit this issue. > > > > But that's not correct. We are no longer in a place where it has one and only > > one request session. After we expose the download it's still possible for it > to > > lose the request (interruption / auto resume / etc.) The state we expose and > the > > capabilities associated with each state are all racy (i.e. our visible state > is > > always subject to TOCTOU races). > > Ok. I'm good with your race handling suggestion earlier in the thread. Is that > what's now reflected in the code? > > ("TOCTOU"?) Time Of Check vs. Time Of Use. :-) Basically, the result of a CanResume() call isn't a guarantee that the caller can call Resume() and expect something to happen in an arbitrary point in the future. I didn't add logic to set is_paused_ here and honor it the next time around. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_item_impl.cc:1566: TransitionTo(CANCELLED_INTERNAL); On 2016/02/12 18:58:10, Randy Smith - Not in Fridays wrote: > On 2016/02/11 23:57:25, asanka wrote: > > Needs an UpdateObservers() call. > > This looks like a note you made to yourself that you didn't execute on; status? Stale. We've relieved Interrupt() of the responsibility of updating observers. Cancel() has an UpdateObservers() call that does it for us. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_item_impl.cc:1805: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/02/12 18:58:10, Randy Smith - Not in Fridays wrote: > On 2016/02/11 23:57:25, asanka wrote: > > Perhaps we should do this when the previous request / download session ended. > > I think we do (given that Interrupt() does a ReleaseDownloadFile() if we have a > download_file_ pointer and the only way we can lose a download_file_ pointer is > through ReleaseDownloadFile()). Are you seeing some way to have an outstanding > valid weak pointer at this point in the code? This was a note to self to check. It is possible to have a pending weak reference from a DeleteDownloadedFile call. That would happen if there was an intermediate file after an interruption and Cancel() was called. However, such a download wouldn't be resumable and hence we wouldn't get here. https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/620001/content/browser/downloa... content/browser/download/download_request_core.h:127: // populate the DownloadCreateInfo when the time comes. On 2016/02/12 18:58:10, Randy Smith - Not in Fridays wrote: > On 2016/02/12 18:31:46, asanka wrote: > > On 2016/02/12 18:01:19, Randy Smith - Not in Fridays wrote: > > > On 2016/02/11 22:08:08, Randy Smith - Not in Fridays wrote: > > > > nit, suggestion: Given that we have the URLRequest and these fields are > the > > > ones > > > > we're putting in the UserData on the URLRequest, could we grab them from > > there > > > > when populating the DCI? This is a little wonky since the DRC makes more > > > > conceptual sense than UserData on URLRequest, but I think that storing in > > one > > > > place is better than storing in two. > > > > > > Ping? > > > > I started out doing that, but it ended up not being quite as clean as I wanted > > because of requests that don't go through CreateRequestOnIOThread() > > Ok with this not being in this CL, but: Aren't those exactly the requests where > these fields aren't needed (i.e. the ones in which we're getting a new request > from the renderers, and thus none of save_info_, id, or the callback is used)? True. But there would also not be a UserData. So we'd end up having to check a few places whether there's a UserData or not. The alternative is to use the default (empty) values for these fields when there are no explicit values to use. That's what we are doing in this CL. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:324: // invoking observers. On 2016/02/12 19:10:28, Randy Smith - Not in Fridays wrote: > I presume there are no observers that do anything like this currently? Are the > observers that will be seriously confused by moving this after > MaybeCompleteDownload()? Or does that mess up dangerous download processing? > > (This seems like a big enough hole to be worth a bug and a fix sometime soon, > though not introduced in this CL, so not blocking.) Yeah. None of the observers do anything crazy, but as I mentioned over IM, requiring that isn't a great API since callers can't readily understand which operations are safe and which ones are not. I filed https://code.google.com/p/chromium/issues/detail?id=586610 to track this and referenced it from the TODOs. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_request_core.h (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_request_core.h:58: // Invokes Delegate::OnStart() with download start parameters. On 2016/02/12 18:58:11, Randy Smith - Not in Fridays wrote: > nit: comment use of parameter. Done.
Ok, fully detailed review of everything non-test. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:367: case INTERRUPTED_TARGET_PENDING_INTERNAL: Hasn't the request already failed, or doesn't exist, in this state? https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:392: case INTERRUPTED_TARGET_PENDING_INTERNAL: Same as above. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:1178: // filename. nit, suggestion: I had some trouble matching this comment to the test below; I think my confusion was "interrupted on initiation" not including "interrupted on resumption". Maybe add "... initiation, or interrupted on resumption and don't have a target ..." to make it clear that interrupted on initiation refers to the global initiation, not this particular request initiation? https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:1523: case TARGET_PENDING_INTERNAL: I thought we couldn't interrupt from TARGET_PENDING_INTERNAL? (If that's right, sorry I didn't catch it on the other CL.) https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.h:275: INTERRUPTED_TARGET_PENDING_INTERNAL, There aren't any states that are documented as transitioning *to* this state. https://codereview.chromium.org/148133007/diff/720001/chrome/browser/download... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/148133007/diff/720001/chrome/browser/download... chrome/browser/download/download_target_determiner.cc:254: download_->GetState() != DownloadItem::IN_PROGRESS) With apologies, but: Is this logic still relevant? Will target determination work reasonably in the absence of being able to detect INTERRUPTED_TARGET_PENDING? https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl.cc:1498: void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { nit, suggestion: I couldn't find any place in the code where Interrupt was called without updating the observers immediately afterwards. Maybe the update should just be next to the TransitionTo() calls in Interrupt? (Completely up to you.) https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl.h:265: // exposed as interrupted, so that target determination can take it into Comment out of date. https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:72: // NOTE: This is not safe and should only be done in a limited set of cases "This is not safe" strikes me as insufficiently precise. Is it true that the it is safe if the a download attempt has been previously made with a web contents and this parameter will be associated with that previous attempt? Is there any way that we can get a download in an interrupted state because of safety issues, and then skip the safety check on resumption? (Second question's about the CL as a whole, not this comment.) https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:82: // Should be set to true if the download is being initiated by web content. nit, vague suggestion: Maybe wordsmith a little so it doesn't sound like you're referring to whether or not there's an associated web contents (in readers mind from the previous comment)? https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:114: // Body of the HTTP POST request. nit, suggestion: "Body of the request if method above was 'POST'" https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:157: // effective if |file_path| is non-empty. Suggestion: "not effective" -> "Ignored"
A couple of test notes and questions. https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_browsertest.cc:2032: // DownloadItem that's created in an interrupted state. How does this work with using UrlDownloader for all resumption requests? Can we resume from this interrupted state and avoid the policy check? https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (left): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:623: TEST_F(DownloadItemTest, NotificationAfterRemove) { Why removed?
https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:367: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > Hasn't the request already failed, or doesn't exist, in this state? True. Moved to the no-op block above. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:392: case INTERRUPTED_TARGET_PENDING_INTERNAL: On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > Same as above. Same. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:1178: // filename. On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > nit, suggestion: I had some trouble matching this comment to the test below; I > think my confusion was "interrupted on initiation" not including "interrupted on > resumption". Maybe add "... initiation, or interrupted on resumption and don't > have a target ..." to make it clear that interrupted on initiation refers to the > global initiation, not this particular request initiation? Ah. I simplified the condition and the comment to just use target_path_.empty(). If the download state is INITIAL_INTERNAL, then target_path_ will be empty. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.cc:1523: case TARGET_PENDING_INTERNAL: On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > I thought we couldn't interrupt from TARGET_PENDING_INTERNAL? (If that's right, > sorry I didn't catch it on the other CL.) It's not allowed. But we can transition to CANCELLED from TARGET_PENDING which comes down the same path. This switch is doing state specific prep before we attempt the state transition below. TransitionTo will ensure that the only transition allowed is TARGET_PENDING -> CANCEL. https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/700001/content/browser/downloa... content/browser/download/download_item_impl.h:275: INTERRUPTED_TARGET_PENDING_INTERNAL, On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > There aren't any states that are documented as transitioning *to* this state. The comments were missing the new transitions. Added them. https://codereview.chromium.org/148133007/diff/720001/chrome/browser/download... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/148133007/diff/720001/chrome/browser/download... chrome/browser/download/download_target_determiner.cc:254: download_->GetState() != DownloadItem::IN_PROGRESS) On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > With apologies, but: Is this logic still relevant? Will target determination > work reasonably in the absence of being able to detect > INTERRUPTED_TARGET_PENDING? It will work, but the resulting target path reservation will be discarded when the download is marked as interrupted at the end of the path determination process. https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_browsertest.cc:2032: // DownloadItem that's created in an interrupted state. On 2016/02/12 22:09:23, Randy Smith - Not in Fridays wrote: > How does this work with using UrlDownloader for all resumption requests? Can we > resume from this interrupted state and avoid the policy check? Phew. Good catch. The NETWORK_INVALID_REQUEST error was not on the RESUME_MODE_INVALID list for some reason. I added a check to make sure this download is not resumable. https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl.cc:1498: void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > nit, suggestion: I couldn't find any place in the code where Interrupt was > called without updating the observers immediately afterwards. Maybe the update > should just be next to the TransitionTo() calls in Interrupt? (Completely up to > you.) Yeah. It seems we can move the UpdateObservers() in there, but I'm still leaning towards treating Interrupt() as an internal method and have the external API make the UpdateObserver calls explicitly. https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl.h:265: // exposed as interrupted, so that target determination can take it into On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > Comment out of date. Done. https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl_unittest.cc (left): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl_unittest.cc:623: TEST_F(DownloadItemTest, NotificationAfterRemove) { On 2016/02/12 22:09:23, Randy Smith - Not in Fridays wrote: > Why removed? Whoops. Restored. https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:72: // NOTE: This is not safe and should only be done in a limited set of cases On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > "This is not safe" strikes me as insufficiently precise. Is it true that the it > is safe if the a download attempt has been previously made with a web contents > and this parameter will be associated with that previous attempt? Is there any > way that we can get a download in an interrupted state because of safety issues, > and then skip the safety check on resumption? (Second question's about the CL > as a whole, not this comment.) For the WebContents associated cases, we don't create a DownloadItem if the download was blocked by a throttle or a resource handler. https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:82: // Should be set to true if the download is being initiated by web content. On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > nit, vague suggestion: Maybe wordsmith a little so it doesn't sound like you're > referring to whether or not there's an associated web contents (in readers mind > from the previous comment)? Done. https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:157: // effective if |file_path| is non-empty. On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > Suggestion: "not effective" -> "Ignored" Done.
https://codereview.chromium.org/148133007/diff/740001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/148133007/diff/740001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api.cc:1058: error_ = content::DownloadInterruptReasonToString(interrupt_reason); Let me see if I'm reading this right. The OnStarted callback used to guarantee that, if a DownloadItem had been created, then the interrupt_reason would be NONE, otherwise it would not be NONE. (Right? Or was this always broken? :-) Now, a DownloadItem may still be created or not be created, and that has no relation to the interrupt_reason. The CL description seems to imply that a DownloadItem would always be non-NULL, but perhaps I skimmed it too quickly? When might this |item| be NULL? Why would there be an item with a non-NONE interrupt reason? Why would there not be an item but a NONE interrupt reason? I'm trying to figure out how to explain to extension authors how to make sense of a callback receiving {error: 'NONE'} without a download id, and whether the if(item) branch needs to set error_ as well, but then how to make sense of {error: 'something'} with a download id.
LGTM; use of comments below up to you (I do think the state transition comments need to be updated, but I trust your judgement over mine once I've pointed you at the areas that don't look right to me). https://codereview.chromium.org/148133007/diff/720001/chrome/browser/download... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/148133007/diff/720001/chrome/browser/download... chrome/browser/download/download_target_determiner.cc:254: download_->GetState() != DownloadItem::IN_PROGRESS) On 2016/02/12 23:34:47, asanka wrote: > On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > > With apologies, but: Is this logic still relevant? Will target determination > > work reasonably in the absence of being able to detect > > INTERRUPTED_TARGET_PENDING? > > It will work, but the resulting target path reservation will be discarded when > the download is marked as interrupted at the end of the path determination > process. Acknowledged. https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/148133007/diff/720001/content/browser/downloa... content/browser/download/download_item_impl.cc:1498: void DownloadItemImpl::Interrupt(DownloadInterruptReason reason) { On 2016/02/12 23:34:47, asanka wrote: > On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > > nit, suggestion: I couldn't find any place in the code where Interrupt was > > called without updating the observers immediately afterwards. Maybe the > update > > should just be next to the TransitionTo() calls in Interrupt? (Completely up > to > > you.) > > Yeah. It seems we can move the UpdateObservers() in there, but I'm still leaning > towards treating Interrupt() as an internal method and have the external API > make the UpdateObserver calls explicitly. SG. https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:72: // NOTE: This is not safe and should only be done in a limited set of cases On 2016/02/12 23:34:47, asanka wrote: > On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > > "This is not safe" strikes me as insufficiently precise. Is it true that the > it > > is safe if the a download attempt has been previously made with a web contents > > and this parameter will be associated with that previous attempt? Is there > any > > way that we can get a download in an interrupted state because of safety > issues, > > and then skip the safety check on resumption? (Second question's about the CL > > as a whole, not this comment.) > > For the WebContents associated cases, we don't create a DownloadItem if the > download was blocked by a throttle or a resource handler. SG, though that only answers my second question. I still tend to think (nit) that this interface should be a little more clearly documented as to in what way id == -1 isn't safe and what the requirements are / what "previously vetted" means. https://codereview.chromium.org/148133007/diff/740001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/740001/content/browser/downloa... content/browser/download/download_item_impl.h:244: // INTERRUPTED_INTERNAL: Afater a failed Start() call. I believe that INTERRUPTED_TARGET_PENDING_INTERNAL should also be on this list? Also, I don't think that INTERRUPTED_INTERNAL should be on it (I don't see a way for that transition to happen in Start(), and IsValidStateTransition() doesn't seem to include it either.)
https://codereview.chromium.org/148133007/diff/740001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/148133007/diff/740001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api.cc:1058: error_ = content::DownloadInterruptReasonToString(interrupt_reason); On 2016/02/13 00:07:32, benjhayden_chromium wrote: > Let me see if I'm reading this right. > > The OnStarted callback used to guarantee that, if a DownloadItem had been > created, then the interrupt_reason would be NONE, otherwise it would not be > NONE. (Right? Or was this always broken? :-) > > Now, a DownloadItem may still be created or not be created, and that has no > relation to the interrupt_reason. > > The CL description seems to imply that a DownloadItem would always be non-NULL, > but perhaps I skimmed it too quickly? > > When might this |item| be NULL? > Why would there be an item with a non-NONE interrupt reason? > Why would there not be an item but a NONE interrupt reason? > > I'm trying to figure out how to explain to extension authors how to make sense > of a callback receiving {error: 'NONE'} without a download id, and whether the > if(item) branch needs to set error_ as well, but then how to make sense of > {error: 'something'} with a download id. FWIW, my understanding of the code is that we may sometimes have a DownloadItem and a non-NONE error code, but we will never have no DownloadItem and a NONE error code, Which I think means the DCHECK directly above this one should actually have been left in. But you should believe Asanka over me (though if I'm wrong about that invariant I feel like I was pretty clueless in the review--I hope I'm right).
https://codereview.chromium.org/148133007/diff/740001/chrome/browser/extensio... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/148133007/diff/740001/chrome/browser/extensio... chrome/browser/extensions/api/downloads/downloads_api.cc:1058: error_ = content::DownloadInterruptReasonToString(interrupt_reason); On 2016/02/13 00:19:29, Randy Smith - Not in Fridays wrote: > On 2016/02/13 00:07:32, benjhayden_chromium wrote: > > Let me see if I'm reading this right. > > > > The OnStarted callback used to guarantee that, if a DownloadItem had been > > created, then the interrupt_reason would be NONE, otherwise it would not be > > NONE. (Right? Or was this always broken? :-) > > > > Now, a DownloadItem may still be created or not be created, and that has no > > relation to the interrupt_reason. > > > > The CL description seems to imply that a DownloadItem would always be > non-NULL, > > but perhaps I skimmed it too quickly? > > > > When might this |item| be NULL? > > Why would there be an item with a non-NONE interrupt reason? > > Why would there not be an item but a NONE interrupt reason? > > > > I'm trying to figure out how to explain to extension authors how to make sense > > of a callback receiving {error: 'NONE'} without a download id, and whether the > > if(item) branch needs to set error_ as well, but then how to make sense of > > {error: 'something'} with a download id. > > FWIW, my understanding of the code is that we may sometimes have a DownloadItem > and a non-NONE error code, but we will never have no DownloadItem and a NONE > error code, Which I think means the DCHECK directly above this one should > actually have been left in. But you should believe Asanka over me (though if > I'm wrong about that invariant I feel like I was pretty clueless in the > review--I hope I'm right). You're right that the above DCHECK should'be been left in. If the download is nullptr, then the interrupt reason should be something other than NONE. I'll re-instate it. However, the interrupt reasons were always supposed to indicate the state of a download. Not explain its absence. That said, we can set it to NONE when returning a valid download item, and then the extension authors can inspect the download item on their own.
I restored the OnStartedCallback semantics. We are now returning a valid download item and an interrupt reason of NONE. The download is also in progress initially. However as soon as the target determination is complete, it will switch to interrupted. This happens for a larger class of downloads than previously. And eventually we are going to get to a place where a download is always created. But we are not quite there yet. https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/720001/content/public/browser/... content/public/browser/download_url_parameters.h:72: // NOTE: This is not safe and should only be done in a limited set of cases On 2016/02/13 00:16:31, Randy Smith - Not in Fridays wrote: > On 2016/02/12 23:34:47, asanka wrote: > > On 2016/02/12 21:57:19, Randy Smith - Not in Fridays wrote: > > > "This is not safe" strikes me as insufficiently precise. Is it true that > the > > it > > > is safe if the a download attempt has been previously made with a web > contents > > > and this parameter will be associated with that previous attempt? Is there > > any > > > way that we can get a download in an interrupted state because of safety > > issues, > > > and then skip the safety check on resumption? (Second question's about the > CL > > > as a whole, not this comment.) > > > > For the WebContents associated cases, we don't create a DownloadItem if the > > download was blocked by a throttle or a resource handler. > > SG, though that only answers my second question. I still tend to think (nit) > that this interface should be a little more clearly documented as to in what way > id == -1 isn't safe and what the requirements are / what "previously vetted" > means. I added a brief explanation. I'm not sure if it's sufficient. https://codereview.chromium.org/148133007/diff/740001/content/browser/downloa... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/148133007/diff/740001/content/browser/downloa... content/browser/download/download_item_impl.h:244: // INTERRUPTED_INTERNAL: Afater a failed Start() call. On 2016/02/13 00:16:31, Randy Smith - Not in Fridays wrote: > I believe that INTERRUPTED_TARGET_PENDING_INTERNAL should also be on this list? > Also, I don't think that INTERRUPTED_INTERNAL should be on it (I don't see a way > for that transition to happen in Start(), and IsValidStateTransition() doesn't > seem to include it either.) Done.
Thanks! downloads_api_browsertest changes lgtm downloads_api non-changes lgtm :-)
content lgtm, but I didn't look at content/browser/download. Assuming rdsmith took care of that and the main review. https://codereview.chromium.org/148133007/diff/760001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/760001/content/public/browser/... content/public/browser/download_url_parameters.h:45: // downlaod request. For new downloads, this callback is invoked after the downlaod -> download https://codereview.chromium.org/148133007/diff/760001/content/public/browser/... content/public/browser/download_url_parameters.h:149: // Suggessted filename for the download. The suggestion can be overridden by Suggested
Thanks everyone! https://codereview.chromium.org/148133007/diff/760001/content/public/browser/... File content/public/browser/download_url_parameters.h (right): https://codereview.chromium.org/148133007/diff/760001/content/public/browser/... content/public/browser/download_url_parameters.h:45: // downlaod request. For new downloads, this callback is invoked after the On 2016/02/17 01:09:03, davidben wrote: > downlaod -> download Done. https://codereview.chromium.org/148133007/diff/760001/content/public/browser/... content/public/browser/download_url_parameters.h:149: // Suggessted filename for the download. The suggestion can be overridden by On 2016/02/17 01:09:04, davidben wrote: > Suggested Done.
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, rdsmith@chromium.org, benjhayden@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/148133007/#ps780001 (title: "Fix typos")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/148133007/780001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/148133007/780001
Message was sent while issue was closed.
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:780001)
Message was sent while issue was closed.
Description was changed from ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 ========== to ========== [Downloads] Always call DM::StartDownload() for explicit downloads. Every explicitly initiated download should be represented in the UI with the exception of downloads which are hidden or downloads which were cancelled by the user or were blocked by a throttle. This is currently not true for downloads whose requests fail before a response is available (e.g. because of the hostname not being resolved) because the DownloadManager is only notified during DownloadResourceHandler::OnResponseStarted(). Failure to notify the DownloadManager results in no DownloadItem being created, and subsequently no UI being presented to the user. The current behavior is also non-ideal for downloads resumption. When a partial download request receives a HTTP 412 response, the current logic invokes DownloadManager::StartDownload() with no indication that the Content-* headers received do not apply to the desired entity. Therefore the DownloadItem's ETag and Last-Modified headers can be incorrectly reset after a pre-condition failure. This CL makes the following changes: - DownloadItem::Start() now consumes a DownloadCreateInfo since multiple requests can be associated with a single DownloadItem due to resumption. Each new request results in a Start() call. - DownloadResourceHandler invokes DownloadManager::StartDownload() even when no OnResponseStarted is received. The only exception is if the associated renderer goes away between the time the download request is sent and the time the response is received. - UrlDownloader always invokes DownloadManager::StartDownload() for all initiated download requests without exception. - DownloadItemImpl invokes the delegate and requests a filename whenever one is needed. - DownloadTargetDeterminer generates a valid filename even if the DownloadItem is not in-progress. - DownloadUrlParameters::OnStartedCallback could now receive a valid DownloadItem even when the request fails. - ResourceDispatcherHost::BeginDownload() is no longer a public API. All wannabe downloaders must now go through DownloadManager to initiate a download. - ResourceDispatcherHostImpl is no longer responsible for invoking OnStartedCallback or pass through download parameters. DownloadRequestCore attaches the data direcly to explicit downloads. Ideally, explicit download initiation would first start by creating a DownloadItem. We aren't quite there yet due to the fact that consumers of the download system make assumptions about the existence of a download item. I.e. the fallacy that the existence of a download item must mean that that download is a candidate to be presented to the UI or other consumers. Once these consumers are fixed, then we can move all explicit download request initiated into DownloadItem, thus simplifying much of this logic. BUG=7648 Committed: https://crrev.com/00b621f5126d538df488090c19175ee892a7161b Cr-Commit-Position: refs/heads/master@{#376487} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/00b621f5126d538df488090c19175ee892a7161b Cr-Commit-Position: refs/heads/master@{#376487} |
