Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17315009:
[Downloads] Add more browsertests for resumption. (Closed)
|
Created:
7 years, 6 months ago by asanka Modified:
7 years, 5 months ago CC:
chromium-reviews, benjhayden+dwatch_chromium.org, darin-cc_chromium.org, jam, joi+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Downloads] Add more browser tests for resumption.
During downloads resumption, most of the legwork is done by the core
downloads code in content/browser/downloads. However, the
chrome/browser/download is also responsible for download filename
determination as well as prompting the user when necessary. These are
browser tests for testing resumption at the chrome/ level.
DownloadItemImpl modified to preserve the download interrupt reason
when resuming and also making sure that OnDownloadUpdated()
notifications are sent when the value of DownloadItem::CanResume()
changes.
TBR=joi
BUG=7648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210052
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Address comments and break off content/ part of CL #Patch Set 4 : Merge with r207865 #Patch Set 5 : Merge with r209222 #Patch Set 6 : Serialize alls to TestFileErrorInjector accounting methods. #
Messages
Total messages: 14 (0 generated)
PTAL?
https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:3105: // resumption. nit: Description doesn't differentiate from previous test. https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:3197: completion_observer->WaitForFinished(); nit: I'd rather have this wait for a transition out of IN_PROGRESS and then confirm that it's transitioned to COMPLETED. Another interrupt would result in a test timeout rather than a quick failure (if I'm reading the code correctly?) https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... content/browser/download/download_item_impl.cc:1360: if (download_file_.get()) { What are the circumstances that trigger this code (and the parallel request_handle_ code below)? This makes me worried that our state machine isn't well-defined, as I think it's implying we can be interrupted while we're in RESUMING. https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... content/browser/download/download_item_impl.cc:1390: UpdateObservers(); Making a note to myself to come back to this after I understand the state diagram implications better.
https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... File chrome/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:3105: // resumption. On 2013/06/19 21:46:21, rdsmith wrote: > nit: Description doesn't differentiate from previous test. Done. https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:3197: completion_observer->WaitForFinished(); On 2013/06/19 21:46:21, rdsmith wrote: > nit: I'd rather have this wait for a transition out of IN_PROGRESS and then > confirm that it's transitioned to COMPLETED. Another interrupt would result in > a test timeout rather than a quick failure (if I'm reading the code correctly?) Done. https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... content/browser/download/download_item_impl.cc:1360: if (download_file_.get()) { On 2013/06/19 21:46:21, rdsmith wrote: > What are the circumstances that trigger this code (and the parallel > request_handle_ code below)? This makes me worried that our state machine isn't > well-defined, as I think it's implying we can be interrupted while we're in > RESUMING. That's exactly the transition that's being handled here (RESUMING -> INTERRUPTED), which happens when the resumption request fails before a response is received.
On 2013/06/20 20:04:01, asanka wrote: > https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... > File chrome/browser/download/download_browsertest.cc (right): > > https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... > chrome/browser/download/download_browsertest.cc:3105: // resumption. > On 2013/06/19 21:46:21, rdsmith wrote: > > nit: Description doesn't differentiate from previous test. > > Done. > > https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... > chrome/browser/download/download_browsertest.cc:3197: > completion_observer->WaitForFinished(); > On 2013/06/19 21:46:21, rdsmith wrote: > > nit: I'd rather have this wait for a transition out of IN_PROGRESS and then > > confirm that it's transitioned to COMPLETED. Another interrupt would result > in > > a test timeout rather than a quick failure (if I'm reading the code > correctly?) > > Done. > > https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... > File content/browser/download/download_item_impl.cc (right): > > https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... > content/browser/download/download_item_impl.cc:1360: if (download_file_.get()) { > On 2013/06/19 21:46:21, rdsmith wrote: > > What are the circumstances that trigger this code (and the parallel > > request_handle_ code below)? This makes me worried that our state machine > isn't > > well-defined, as I think it's implying we can be interrupted while we're in > > RESUMING. > > That's exactly the transition that's being handled here (RESUMING -> > INTERRUPTED), which happens when the resumption request fails before a response > is received. Sorry, say a bit more here? What's can cause the resumption request to fail before a response is received?
On 2013/06/20 20:09:23, rdsmith wrote: > On 2013/06/20 20:04:01, asanka wrote: > > > https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... > > File chrome/browser/download/download_browsertest.cc (right): > > > > > https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... > > chrome/browser/download/download_browsertest.cc:3105: // resumption. > > On 2013/06/19 21:46:21, rdsmith wrote: > > > nit: Description doesn't differentiate from previous test. > > > > Done. > > > > > https://codereview.chromium.org/17315009/diff/2001/chrome/browser/download/do... > > chrome/browser/download/download_browsertest.cc:3197: > > completion_observer->WaitForFinished(); > > On 2013/06/19 21:46:21, rdsmith wrote: > > > nit: I'd rather have this wait for a transition out of IN_PROGRESS and then > > > confirm that it's transitioned to COMPLETED. Another interrupt would result > > in > > > a test timeout rather than a quick failure (if I'm reading the code > > correctly?) > > > > Done. > > > > > https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... > > File content/browser/download/download_item_impl.cc (right): > > > > > https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... > > content/browser/download/download_item_impl.cc:1360: if (download_file_.get()) > { > > On 2013/06/19 21:46:21, rdsmith wrote: > > > What are the circumstances that trigger this code (and the parallel > > > request_handle_ code below)? This makes me worried that our state machine > > isn't > > > well-defined, as I think it's implying we can be interrupted while we're in > > > RESUMING. > > > > That's exactly the transition that's being handled here (RESUMING -> > > INTERRUPTED), which happens when the resumption request fails before a > response > > is received. > > Sorry, say a bit more here? What's can cause the resumption request to fail > before a response is received? Sorry, to be specific, I meant interrupted before OnResponseStarted is called. E.g. the server is unreachable. These are cases where we don't end up calling DownloadManager::StartDownload().
https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... content/browser/download/download_item_impl.cc:1360: if (download_file_.get()) { On 2013/06/20 20:04:01, asanka wrote: > On 2013/06/19 21:46:21, rdsmith wrote: > > What are the circumstances that trigger this code (and the parallel > > request_handle_ code below)? This makes me worried that our state machine > isn't > > well-defined, as I think it's implying we can be interrupted while we're in > > RESUMING. > > That's exactly the transition that's being handled here (RESUMING -> > INTERRUPTED), which happens when the resumption request fails before a response > is received. Gotcha. To the extent possible, I'd like to make state related conditionals explicitly depend on the internal state, as part of trying to keep the code-path-important changes in state to the state variable as much as we can. Willing to rewrite these to be conditionalized on state_, possibly with some assertions? https://codereview.chromium.org/17315009/diff/13001/content/browser/download/... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/17315009/diff/13001/content/browser/download/... content/browser/download/download_item_impl.cc:1390: UpdateObservers(); It's a nit, and I won't block landing for it, but I'm a bit uncomfortable about this--it has a code smell to it. Either the conditional inside of TransitionTo() is correct and we can rely on it, or we're doing the UpdateObservers() decision at the wrong place. So I want to at least consider doing this differently. Options that come to mind: * Hoist the UpdateObservers() outside of the TransitionTo() and let callers decide whether to update observers. This is a somewhat painful explosion of code, and I think a bit hard to maintain. A variation of this would be to add a boolean to TransitionTo() indicating whether or not to update; I think that's easier to maintain as it being in the function signature means that callers are forced to think about whether they want to update or not. * Make the conditional in TransitionTo() take into account the resumption mode that's being transitioned to; maybe trigger the UpdateObservers() if we're transitioning to a CanResume() that either doesn't allow resumption or requires the user to resume manually? There may well be other good options--I'm not enamored of either of the ones I sketch out. But I'd like to explore if this is the right thing.
https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/17315009/diff/2001/content/browser/download/d... content/browser/download/download_item_impl.cc:1360: if (download_file_.get()) { On 2013/06/20 20:55:22, rdsmith wrote: > On 2013/06/20 20:04:01, asanka wrote: > > On 2013/06/19 21:46:21, rdsmith wrote: > > > What are the circumstances that trigger this code (and the parallel > > > request_handle_ code below)? This makes me worried that our state machine > > isn't > > > well-defined, as I think it's implying we can be interrupted while we're in > > > RESUMING. > > > > That's exactly the transition that's being handled here (RESUMING -> > > INTERRUPTED), which happens when the resumption request fails before a > response > > is received. > > Gotcha. To the extent possible, I'd like to make state related conditionals > explicitly depend on the internal state, as part of trying to keep the > code-path-important changes in state to the state variable as much as we can. > Willing to rewrite these to be conditionalized on state_, possibly with some > assertions? Yup. That's a good suggestion. https://codereview.chromium.org/17315009/diff/13001/content/browser/download/... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/17315009/diff/13001/content/browser/download/... content/browser/download/download_item_impl.cc:1390: UpdateObservers(); On 2013/06/20 20:55:23, rdsmith wrote: > It's a nit, and I won't block landing for it, but I'm a bit uncomfortable about > this--it has a code smell to it. Either the conditional inside of > TransitionTo() is correct and we can rely on it, or we're doing the > UpdateObservers() decision at the wrong place. So I want to at least consider > doing this differently. > > Options that come to mind: > * Hoist the UpdateObservers() outside of the TransitionTo() and let callers > decide whether to update observers. This is a somewhat painful explosion of > code, and I think a bit hard to maintain. A variation of this would be to add > a boolean to TransitionTo() indicating whether or not to update; I think that's > easier to maintain as it being in the function signature means that callers are > forced to think about whether they want to update or not. > > * Make the conditional in TransitionTo() take into account the resumption mode > that's being transitioned to; maybe trigger the UpdateObservers() if we're > transitioning to a CanResume() that either doesn't allow resumption or requires > the user to resume manually? > > There may well be other good options--I'm not enamored of either of the ones I > sketch out. But I'd like to explore if this is the right thing. I moved the UpdateObserver changes to https://codereview.chromium.org/17515003/ along with an implementation of the first option.
LGTM.
On 2013/06/21 01:20:30, rdsmith wrote: > LGTM. Randy: I made a change to test_file_error_injector to account for some flakyness I saw. Could you take a peek?
LGTM (but pay attention to the tests; this is the kind of thing that could produce subtle bugs. Though in this case it sounds like it's fixing a subtle bug :-}.)
On 2013/07/03 15:43:02, rdsmith wrote: > LGTM (but pay attention to the tests; this is the kind of thing that could > produce subtle bugs. Though in this case it sounds like it's fixing a subtle > bug :-}.) Thanks! joi: Adding you as TBR. The change to content/public/test resolves a race and doesn't change the public API.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/17315009/37001
Message was sent while issue was closed.
Change committed as 210052
Message was sent while issue was closed.
LGTM for //content/public. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
