Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1203983004:
Stop using SpawnedTestServer in DownloadContentTest.* (Closed)
|
Created:
5 years, 6 months ago by asanka Modified:
5 years ago CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop using SpawnedTestServer in DownloadContentTest.*
This CL introduces TestDownloadRequestHandler, a URLRequestJob based
interceptor that can be used to mock URLRequest behavior. The new
request handler supports simulating network and server errors, and is a
replacement for the testserver.py based test setup //content is
currently using to test download resumption.
Moving away from SpawnedTestServer allows us to re-enable the tests on
Android.
R=rdsmith@chromium.org,svaldez@chromium.org,sky@chromium.org
BUG=215894
BUG=493347
BUG=7648
Committed: https://crrev.com/c7bf9bf1624d05928bbd4ee58d2c5e377fd6cd04
Cr-Commit-Position: refs/heads/master@{#363108}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 6
Patch Set 5 : #
Total comments: 60
Patch Set 6 : Address comments #
Total comments: 22
Patch Set 7 : Move If-Range change out to a separate CL. #
Total comments: 2
Patch Set 8 : #Patch Set 9 : Address comments. #
Total comments: 6
Patch Set 10 : Comment updates #
Total comments: 2
Patch Set 11 : (merge with origin/master) #Patch Set 12 : Catch up with changes to ReadRawData. #Patch Set 13 : Catch up with ReadRawData changes and add MSVC friendly move assignment operator. #Patch Set 14 : MSVC doesn't want to make default move constructors either. #
Messages
Total messages: 56 (22 generated)
asanka@chromium.org changed reviewers: + rdsmith@chromium.org
asanka@chromium.org changed reviewers: + svaldez@chromium.org
svaldez: This CL adds a URLRequestJob that can inject errors into a URL request. There's some code that builds on this to add tests for redirects, timeouts and other network errors. Those additional tests aren't here since I want to include those with fixes to our resumption state machine which currently doesn't handle those cases well. Let me know what you think if you think we should try to do this with EmbeddedTestServer.
On 2015/11/06 22:28:37, asanka wrote: > svaldez: This CL adds a URLRequestJob that can inject errors into a URL request. > > There's some code that builds on this to add tests for redirects, timeouts and > other network errors. Those additional tests aren't here since I want to include > those with fixes to our resumption state machine which currently doesn't handle > those cases well. Let me know what you think if you think we should try to do > this with EmbeddedTestServer. Looking at this, its mostly doable with EmbeddedTestServer, and probably eventually you'll want to migrate this testing to use EmbeddedTestServer, though if this code is already implemented and working, you might just want to use this for now and then think about migrating over to ETS at a later time.
On 2015/11/09 at 14:52:21, svaldez wrote: > On 2015/11/06 22:28:37, asanka wrote: > > svaldez: This CL adds a URLRequestJob that can inject errors into a URL request. > > > > There's some code that builds on this to add tests for redirects, timeouts and > > other network errors. Those additional tests aren't here since I want to include > > those with fixes to our resumption state machine which currently doesn't handle > > those cases well. Let me know what you think if you think we should try to do > > this with EmbeddedTestServer. > > Looking at this, its mostly doable with EmbeddedTestServer, and probably eventually you'll want to migrate this testing to use EmbeddedTestServer, though if this code is already implemented and working, you might just want to use this for now and then think about migrating over to ETS at a later time. Yup. I was looking at this more from an architectural perspective. The failures that we anticipate needing to replicate[*] include failures that happen in the big cloud of boxes that exist between the client and the server. Replicating those errors using the server seemed to me to be incorrect layering. The URLRequestJob, on the other hand, represents all boxes from the client to the server. As for the issues that can happen outside the network pipeline, we have the test_file_error_injector and various DownloadFile mocks. [*] as mentioned earlier, I wrote some tests for a couple of arcane cases, which all failed because we don't handle those cases yet. I'd like to fix these issues in the coming weeks.
asanka@chromium.org changed reviewers: + phajdan.jr@chromium.org
Description was changed from ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,davidben@chromium.org BUG=215894 BUG=493347 BUG=7648 ========== to ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,phajdan.jr@chromium.org BUG=215894 BUG=493347 BUG=7648 ==========
Ok. Mailing the CL out in earnest. rdsmith: Review everything. svaldez is your apprentice and should also review everything. :P phajdan.jr: I placed the test_download_request_hanlder in content/public/test because the interrupted download stuff will need to be tested from //chrome as well (and any other embedder) since it's something that has to be largely driven by the embedder. Could you give it an OWNERs look through?
Haven't really looked at download_browsertest or test_download_request_handler yet, I'll try taking a look on my flight. Couple nits on the rest. Could you also remove the HandleRangeReset TODO in net/test/embedded_test_server/default_handlers.cc since it no longer needs to be implemented with this CL. https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... content/browser/download/download_browsertest.cc:1445: ASSERT_TRUE(origin_one.InitializeAndWaitUntilReady()); Use .Start() https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... File content/browser/download/download_item_impl_delegate.h (right): https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... content/browser/download/download_item_impl_delegate.h:29: const base::FilePath& // Intermediate file path Might want to exclude this change, or unify spacing for comments. https://codereview.chromium.org/1203983004/diff/60001/net/tools/testserver/te... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/1203983004/diff/60001/net/tools/testserver/te... net/tools/testserver/testserver.py:1373: #then final CRLF. Extra change?
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... content/browser/download/download_browsertest.cc:1445: ASSERT_TRUE(origin_one.InitializeAndWaitUntilReady()); On 2015/11/11 at 20:42:41, svaldez wrote: > Use .Start() Done. https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... File content/browser/download/download_item_impl_delegate.h (right): https://codereview.chromium.org/1203983004/diff/60001/content/browser/downloa... content/browser/download/download_item_impl_delegate.h:29: const base::FilePath& // Intermediate file path On 2015/11/11 at 20:42:41, svaldez wrote: > Might want to exclude this change, or unify spacing for comments. This was 'git cl format's doing. But removed since its inclusion was accidental. https://codereview.chromium.org/1203983004/diff/60001/net/tools/testserver/te... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/1203983004/diff/60001/net/tools/testserver/te... net/tools/testserver/testserver.py:1373: #then final CRLF. On 2015/11/11 at 20:42:41, svaldez wrote: > Extra change? Indeed. Undone.
Cool! Does that mean I can send him up against some clearly superior opponent, secure in the knowledge that whether he wins or loses, my agenda will be advanced?? :-} Nice cleanup. First round of comments. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:942: IN_PROC_BROWSER_TEST_F(DownloadContentTest, Resume_BasicWithStrongValidators) { Does this really involve basic auth? It's not obvious at the test level (or at the TestDownloadRequestHandler .h file level) if so. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:949: auto interruption = parameters.injected_errors.front(); nit, suggestion: I'm a little uncomfortable with auto here, specifically because it doesn't surface that you're copying a value rather than a pointer. Willing to replace with actual type? https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:970: // downloads system and may change as implementation details change. Suggestion: I'm not sure if it's worth it, but I think what you really want to check here is that after the interruption, the next request attempted to download the range past the interruption (whatever the server did or the download system did pre-interruption). I think you could write it that way; search for the request that failed, and look at what the one after it asked for. (And yay for thinking about characterization vs. behavior.) https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1010: const TestDownloadRequestHandler::InjectedError interruption = I would like you to be consistent about use or lack of use of auto in TestDownloadRequestHandler consumers. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1039: // downloads system and may change as implementation details change. This is pretty much the same test as above, right? Is it worth abstracting it into a function, so that if it needs to change later it can more easily? https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1062: // Confirm we don't try to resume if we don't have a verifier. nit, suggestion: I *think* the terminology we settled on was "restart" == start from beginning, "continue" == range request where we left off, "resume" == union of those two. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1135: // intermediate file is missing. I'm trying to come up with the failure modes we're probing for here. I'm inclined to say that, however the system got the whole file, if it did get the whole file after we deleted the initial bytes out from under it, it's properly handled the file deletion recovery case. Does it matter to correctness what the underlying requests are? This argument doesn't apply in the other cases because it matters to *performance* what the underlying requests are; we want to make sure we made range requests if we could. Here we can't, and we're explicitly falling back to the non-performant path. So I don't think we care about what the on-the-wire bits looked like, only that we got all the bits correctly. WDYT? https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1243: Resume_RecoverFromFinalRenameError) { Why two lines? (I suspect the answer is "git cl format", but it does look like it would fit on a single line). https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1380: EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState()); Is this characterization? Isn't it just because Resume() results in a post to the IO thread and the download state doesn't get interrupted until the pingback? (Actually, this seems like an API failure--the download state shouldn't be interrupted after a resume. I can imagine the implementation considerations that drive this, but I'm inclined to think we should fix them?) https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1418: EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1); Possibly we should go over the API contract details around removal; I'm having the same vague-memory-plus-sinking-feeling looking at this that I had looking at the Resume->Interrupted lack of transition above. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:104: request->SetExtraRequestHeaderByName("If-Range", params->etag(), true); a-r nit: Should this be in this CL? It looks like a real bug if this needed to be changed. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:24: class Interceptor : public net::URLRequestInterceptor { nit, suggestion: Use a more fully qualified name so that codesearch/*tags can more easily find this class? https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:36: void GetAndResetCompletedRequests( General comment throughout this file: Comments as to thread homes? Those comments can be for the whole class, or per method if useful, but given that (I think?) everything on TestDownloadRequestHandler expects to be called on the UI thread, and this class is pure IO thread, it seems worthwhile a line calling out the usage. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:39: const TestDownloadRequestHandler::CompletedRequest& request_info); nit: Use parallel variable names for these two functions? (I.e. request{,s} or request_info{,s}?) https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:92: class StaticResponseJob : public net::URLRequestJob { nit: Single line comment as to purpose? (In the context of this .cc file PartialResponseJob was self-explanatory, but I'm not sure of what StaticResponseJob does, and none of the methods in it give any hints.) https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:139: DVLOG(1) << "Starting request for " << request()->url().spec(); Just confirming that you wanted to land the log entries? https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:155: if (parameters_->support_byte_ranges && I feel like a comment or two would go a long way here. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:163: offset_end_ = byte_ranges[0].last_byte_position(); Does this need to be the min of the parameters_->size - 1 and the byte_ranges[0].last_byte_position()? Also, I'm inclined to think the class should error out in some fashion if there are multiple ranges? https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:440: BrowserThread::PostTask( Comment about why a simple call to a setter starts the serving? (Because before that the interceptor wasn't claiming any requests, I presume.) https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:24: // A request handler that can be used to mock the behavior a URLRequestJob for a nit: "of a"? https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:35: // This test class is meant to help test failures in #2 and #3 above. The test Wouldn't errors on the server normally show up with a bad HTTP status code rather than a net:: error? So isn't this more narrowly targeted at #2? https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:72: Parameters(const Parameters& other); suggestion: I'm tempted to ask for a comment calling out copy construction, here or at the end of the class, just because the default for chrome is to disallow that. Also: Copy constructor without assignment operator seems weird; if one can default, why can't both? Conversely, if you're going to enable one, enabling the other seems wise? https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:152: // known, but don't do it). Suggestion: Can you assert/dcheck if they do that? Or have initialization automatically use a new URL, JIC? The undefined but consistent and allowed behavior seems a bit attractive-nuisance like. The check wouldn't need to be general; I'm happy with people explicitly specifying the URL getting what they deserve, I'd just like the default behavior to be easy to use correctly.
https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:432: return download->GetState() == DownloadItem::COMPLETE; Might just want to merge these into DownloadStateFilter(DownloadItem::State state, DownloadItem* download) and then Bind appropriately on use. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:532: DownloadUpdatedObserver(download, base::Bind(&DownloadInterruptFilter)) See above. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:659: ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState()); Can this race and be in COMPLETE state? https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:845: EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState()); See above. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:995: // A partial resumption results in an HTTP 200 response. I.e. ther server *the
To notch reviews! A+++. Will send more CLs to review. No really. I've promised both of you additional CLs in the comments to address some of the unrelated issues that you've identified. I'm mailing this out, but I'll rebase this CL on the others once they are ready. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:432: return download->GetState() == DownloadItem::COMPLETE; On 2015/11/12 at 20:25:51, svaldez wrote: > Might just want to merge these into DownloadStateFilter(DownloadItem::State state, DownloadItem* download) and then Bind appropriately on use. Good call. I changed called it IsDownloadInState() since it's more of a predicate now that it's not satisfying the Filter interface. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:532: DownloadUpdatedObserver(download, base::Bind(&DownloadInterruptFilter)) On 2015/11/12 at 20:25:51, svaldez wrote: > See above. Done. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:659: ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState()); On 2015/11/12 at 20:25:51, svaldez wrote: > Can this race and be in COMPLETE state? Great catch! In practice we get lucky, but only because of a couple of implementation details that we shouldn't depend on. This is especially true since that area of code (navigation) is currently under flux. I.e. we are indirectly calling NavigateToURLBlockUntilNavigationsComplete() and assuming that the DownloadItem hasn't progressed past IN_PROGRESS by the time the navigation completes. The ResourceLoader stack simulates a cancellation to the AsyncResourceHandler before the DownloadResourceHandler takes over. This cancellation propagates into the renderer, and back to the browser and causes the WeContents to signal a stop loading event. This has to consistently happen before the DownloadResourceHandler triggers a new download to be created, the new download determines its target, and that download slurps enough data to progress past the IN_PROGRESS state. I'm reluctant to introduce unrelated changes in the same CL, so I'll spin up a new one and send it your way to change how we handle StartDownloadAndReturnItem(). https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:845: EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState()); On 2015/11/12 at 20:25:51, svaldez wrote: > See above. Ditto. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:942: IN_PROC_BROWSER_TEST_F(DownloadContentTest, Resume_BasicWithStrongValidators) { On 2015/11/12 at 00:57:57, rdsmith wrote: > Does this really involve basic auth? It's not obvious at the test level (or at the TestDownloadRequestHandler .h file level) if so. Basic meaning it's a basic test with no additional complications. :) I guess I'll remove the "basic" part since it's confusing. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:949: auto interruption = parameters.injected_errors.front(); On 2015/11/12 at 00:57:57, rdsmith wrote: > nit, suggestion: I'm a little uncomfortable with auto here, specifically because it doesn't surface that you're copying a value rather than a pointer. Willing to replace with actual type? Done. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:970: // downloads system and may change as implementation details change. On 2015/11/12 at 00:57:56, rdsmith wrote: > Suggestion: I'm not sure if it's worth it, but I think what you really want to check here is that after the interruption, the next request attempted to download the range past the interruption (whatever the server did or the download system did pre-interruption). I think you could write it that way; search for the request that failed, and look at what the one after it asked for. > > (And yay for thinking about characterization vs. behavior.) Yeah. I was thinking of ways to generalize it. The fact that we request the range from the very next byte onwards is also, IMO, an implementation detail. In the past we've thrown around doing slightly overlapping ranges or spot checking to verify that the byte ranges we are getting agrees with the ranges we already have. The most general way to check, might be to verify that in the end we have all the bits for the requested resource and then verify that the sum of the data transferred over the wire is substantially more economical than starting the request from scratch. I.e. sum of data transferred < (size received before resumption + size of entire resource). But that seemed excessively weak. I'm open for suggestions, but overall, I couldn't convince myself that it's a win to generalize this further in order to avoid a characterization risk. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:995: // A partial resumption results in an HTTP 200 response. I.e. ther server On 2015/11/12 at 20:25:51, svaldez wrote: > *the Done. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1010: const TestDownloadRequestHandler::InjectedError interruption = On 2015/11/12 at 00:57:57, rdsmith wrote: > I would like you to be consistent about use or lack of use of auto in TestDownloadRequestHandler consumers. Yup. Did a consistency pass. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1039: // downloads system and may change as implementation details change. On 2015/11/12 at 00:57:57, rdsmith wrote: > This is pretty much the same test as above, right? Is it worth abstracting it into a function, so that if it needs to change later it can more easily? This one asserts that the resumption resulted in a restart whereas the previous test asserts that resumption resulted in a continuation. After reconsideration, I now believe that it is not a characterization risk to assume that there will be two requests on the wire: * The first one that transfers up to the interruption point. * The second request that transfers the whole resource. I've updated the test accordingly. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1062: // Confirm we don't try to resume if we don't have a verifier. On 2015/11/12 at 00:57:57, rdsmith wrote: > nit, suggestion: I *think* the terminology we settled on was "restart" == start from beginning, "continue" == range request where we left off, "resume" == union of those two. You're correct. Updated. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1135: // intermediate file is missing. On 2015/11/12 at 00:57:57, rdsmith wrote: > I'm trying to come up with the failure modes we're probing for here. I'm inclined to say that, however the system got the whole file, if it did get the whole file after we deleted the initial bytes out from under it, it's properly handled the file deletion recovery case. Does it matter to correctness what the underlying requests are? > > This argument doesn't apply in the other cases because it matters to *performance* what the underlying requests are; we want to make sure we made range requests if we could. Here we can't, and we're explicitly falling back to the non-performant path. So I don't think we care about what the on-the-wire bits looked like, only that we got all the bits correctly. > > WDYT? We are verifying that the download logic abandons the validators and issues a fresh request since it has lost the data, and that it abandons the validators without too many unnecessary requests in the middle. The characterization aspect here is that the logic sends a partial request in the middle unnecessarily because our logic doesn't look at the intermediate file until a response is seen. I consider this to be a but that we should fix at some point, but one that is perhaps not relevant to this test. I've removed the checks since as far as correctness goes, the ReadAndVerifyFileContents() call is sufficient. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1243: Resume_RecoverFromFinalRenameError) { On 2015/11/12 at 00:57:57, rdsmith wrote: > Why two lines? (I suspect the answer is "git cl format", but it does look like it would fit on a single line). 'git cl format' :) but it's also 81 characters without the line break. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1380: EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState()); On 2015/11/12 at 00:57:57, rdsmith wrote: > Is this characterization? Isn't it just because Resume() results in a post to the IO thread and the download state doesn't get interrupted until the pingback? > > (Actually, this seems like an API failure--the download state shouldn't be interrupted after a resume. I can imagine the implementation considerations that drive this, but I'm inclined to think we should fix them?) Correct on both counts. I consider this to be an API failure that we don't expose RESUMING_INTERNAL as IN_PROGRESS, but it's something I'd like to address in a separate CL. For this one, I reworked the logic so that we are waiting for the network request to be initiated, and then removing the download and then continuing the network request. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:1418: EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1); On 2015/11/12 at 00:57:57, rdsmith wrote: > Possibly we should go over the API contract details around removal; I'm having the same vague-memory-plus-sinking-feeling looking at this that I had looking at the Resume->Interrupted lack of transition above. Yup. Reworked in the same manner, but the API should be fixed in another CL. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_manager_impl.cc:104: request->SetExtraRequestHeaderByName("If-Range", params->etag(), true); On 2015/11/12 at 00:57:57, rdsmith wrote: > a-r nit: Should this be in this CL? It looks like a real bug if this needed to be changed. I'll make another CL and make this CL depend on that for the change from If-Match to If-Range. This isn't a correctness bug, but rather an efficiency improvement. If-Match results in a compliant server sending an HTTP 412 in the case of an ETag mismatch. Then the downloads logic needs to send an unconditionalized request to fetch the entire resource. If-Range, on the other hand, will respond with a 200 along with the entire entity if there's an ETag mismatch. It's just one request shorter. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:24: class Interceptor : public net::URLRequestInterceptor { On 2015/11/12 at 00:57:57, rdsmith wrote: > nit, suggestion: Use a more fully qualified name so that codesearch/*tags can more easily find this class? Done. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:36: void GetAndResetCompletedRequests( On 2015/11/12 at 00:57:57, rdsmith wrote: > General comment throughout this file: Comments as to thread homes? Those comments can be for the whole class, or per method if useful, but given that (I think?) everything on TestDownloadRequestHandler expects to be called on the UI thread, and this class is pure IO thread, it seems worthwhile a line calling out the usage. Done. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:39: const TestDownloadRequestHandler::CompletedRequest& request_info); On 2015/11/12 at 00:57:57, rdsmith wrote: > nit: Use parallel variable names for these two functions? (I.e. request{,s} or request_info{,s}?) Done. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:92: class StaticResponseJob : public net::URLRequestJob { On 2015/11/12 at 00:57:57, rdsmith wrote: > nit: Single line comment as to purpose? (In the context of this .cc file PartialResponseJob was self-explanatory, but I'm not sure of what StaticResponseJob does, and none of the methods in it give any hints.) This got removed. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:139: DVLOG(1) << "Starting request for " << request()->url().spec(); On 2015/11/12 at 00:57:57, rdsmith wrote: > Just confirming that you wanted to land the log entries? Yeah. They are D* logs that only result in code for debug builds and don't spew any output unless explicitly requested via a --vmodule flag. I left them in since they were pretty useful during debugging for me and I thought it would be useful for others as well. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:155: if (parameters_->support_byte_ranges && On 2015/11/12 at 00:57:57, rdsmith wrote: > I feel like a comment or two would go a long way here. Yup. It looks a bit dense now that I look at it again. So I refactored it into several functions instead. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:163: offset_end_ = byte_ranges[0].last_byte_position(); On 2015/11/12 at 00:57:57, rdsmith wrote: > Does this need to be the min of the parameters_->size - 1 and the byte_ranges[0].last_byte_position()? The HttpByteRange::ComputeBounds() method takes care of this. I.e. it adjusts the range so that it fits inside 0..size-1. > Also, I'm inclined to think the class should error out in some fashion if there are multiple ranges? Handled in the new code. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.cc:440: BrowserThread::PostTask( On 2015/11/12 at 00:57:57, rdsmith wrote: > Comment about why a simple call to a setter starts the serving? (Because before that the interceptor wasn't claiming any requests, I presume.) It's a bit unclear, but StartServing(const Parameters&) starts serving based on the given parameters. It doesn't mean that it wasn't serving before. On the implementation side, it just sets the job factory to use based on the new parameters. The Interceptor is RAII and is already intercepting requests, but doesn't do anything until it has a factory to work with. I'll add a comment to explain. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:24: // A request handler that can be used to mock the behavior a URLRequestJob for a On 2015/11/12 at 00:57:57, rdsmith wrote: > nit: "of a"? Done. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:35: // This test class is meant to help test failures in #2 and #3 above. The test On 2015/11/12 at 00:57:57, rdsmith wrote: > Wouldn't errors on the server normally show up with a bad HTTP status code rather than a net:: error? So isn't this more narrowly targeted at #2? Currently, yes. But it's also controlling the entire server response, and hence can accomplish #3 as well. I added an OnStartHandler to deal with pausing and error generation during the Start() loop which effectively makes this fulfill #3 as well. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:72: Parameters(const Parameters& other); On 2015/11/12 at 00:57:57, rdsmith wrote: > suggestion: I'm tempted to ask for a comment calling out copy construction, here or at the end of the class, just because the default for chrome is to disallow that. > > Also: Copy constructor without assignment operator seems weird; if one can default, why can't both? Conversely, if you're going to enable one, enabling the other seems wise? Thanks for calling this out. During an intermediate stage, the compiler decided it couldn't generate a default copy constructor. But now it can. Removed the copy constructor. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:152: // known, but don't do it). On 2015/11/12 at 00:57:57, rdsmith wrote: > Suggestion: Can you assert/dcheck if they do that? Or have initialization automatically use a new URL, JIC? The undefined but consistent and allowed behavior seems a bit attractive-nuisance like. The check wouldn't need to be general; I'm happy with people explicitly specifying the URL getting what they deserve, I'd just like the default behavior to be easy to use correctly. On debug builds, using the same URL to register two interceptors will hit a DCHECK in net::URLRequestFilter. Assuming tests are built with DCHECKs on, that should suffice to curb abuse. I made the default constructor use a generated URL and added a note that callers shouldn't expect the URL to be the same across different runs of the same test.
https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:659: ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState()); On 2015/11/13 at 21:40:01, asanka wrote: > On 2015/11/12 at 20:25:51, svaldez wrote: > > Can this race and be in COMPLETE state? > > Great catch! > In practice we get lucky, but only because of a couple of implementation details that we shouldn't depend on. This is especially true since that area of code (navigation) is currently under flux. > > I.e. we are indirectly calling NavigateToURLBlockUntilNavigationsComplete() and assuming that the DownloadItem hasn't progressed past IN_PROGRESS by the time the navigation completes. The ResourceLoader stack simulates a cancellation to the AsyncResourceHandler before the DownloadResourceHandler takes over. This cancellation propagates into the renderer, and back to the browser and causes the WeContents to signal a stop loading event. This has to consistently happen before the DownloadResourceHandler triggers a new download to be created, the new download determines its target, and that download slurps enough data to progress past the IN_PROGRESS state. > > I'm reluctant to introduce unrelated changes in the same CL, so I'll spin up a new one and send it your way to change how we handle StartDownloadAndReturnItem(). Actually, no. I misspoke. Here and in the instance below, we are invoking URLRequestSlowDownloadJob. This particular test fixture is set up to deal with this kind of race (doh!). The response doesn't progress past a specific point until another request is made (to kFinishDownloadJob or kErrorDownloadJob). This ensures that the download item, once created, doesn't make it past the IN_PROGRESS state.
Description was changed from ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,phajdan.jr@chromium.org BUG=215894 BUG=493347 BUG=7648 ========== to ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,phajdan.jr@chromium.org BUG=215894 BUG=493347 BUG=7648 ==========
Just sending a quick note to make Helen aware of this CL, as this is another URLRequestJob that she'll need to integrate her ReadRawData refactor change with.
Next round. Once we reach closure on these, I'm happy, and these are pretty much all nits. https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1203983004/diff/100001/content/browser/downlo... content/browser/download/download_browsertest.cc:970: // downloads system and may change as implementation details change. On 2015/11/13 21:40:01, asanka wrote: > On 2015/11/12 at 00:57:56, rdsmith wrote: > > Suggestion: I'm not sure if it's worth it, but I think what you really want to > check here is that after the interruption, the next request attempted to > download the range past the interruption (whatever the server did or the > download system did pre-interruption). I think you could write it that way; > search for the request that failed, and look at what the one after it asked for. > > > > > (And yay for thinking about characterization vs. behavior.) > > Yeah. I was thinking of ways to generalize it. The fact that we request the > range from the very next byte onwards is also, IMO, an implementation detail. In > the past we've thrown around doing slightly overlapping ranges or spot checking > to verify that the byte ranges we are getting agrees with the ranges we already > have. > > The most general way to check, might be to verify that in the end we have all > the bits for the requested resource and then verify that the sum of the data > transferred over the wire is substantially more economical than starting the > request from scratch. I.e. sum of data transferred < (size received before > resumption + size of entire resource). But that seemed excessively weak. > > I'm open for suggestions, but overall, I couldn't convince myself that it's a > win to generalize this further in order to avoid a characterization risk. Hmmm. Maybe have an observer track total bytes transferred and just do a comparison? But I don't have any strong opinions here, I just agree with you that it's worth some thought to see if we can test behavior rather than characterization. But I think we've satisfied honor :-}. https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:72: Parameters(const Parameters& other); On 2015/11/13 21:40:02, asanka wrote: > On 2015/11/12 at 00:57:57, rdsmith wrote: > > suggestion: I'm tempted to ask for a comment calling out copy construction, > here or at the end of the class, just because the default for chrome is to > disallow that. > > > > Also: Copy constructor without assignment operator seems weird; if one can > default, why can't both? Conversely, if you're going to enable one, enabling > the other seems wise? > > Thanks for calling this out. During an intermediate stage, the compiler decided > it couldn't generate a default copy constructor. But now it can. Removed the > copy constructor. I read the style guide as recommending but not requiring explicitly declaring default copy constructors and assignment operators with = default. Up to you, just wanted to make sure you were aware of that recommend-but-not-require (https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types ) https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:134: // asynchronously in order to avoid unexpected re-entrancy. I don't understand this sentence; what does avoiding re-entrancy have to do with avoiding network requests? https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:150: void StoreValueAndInvokeClosure(const base::Closure& quit_closure, nitty nit: The name "quit_closure" presumes how this function will be used. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:157: // Xorshift* PRNG from https://en.wikipedia.org/wiki/Xorshift Did not review except to validate that it looked appropriately arcane :-}. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:60: class TestDownloadRequestHandler { Is there a comment as to the thread home of this class and I missed it? https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:65: // A callback of this type can be used to intercept the Start() event of a new nit, suggestion: "this type" -> "type OnStartHandler" and move the definition of OnStartResponseCallback to below this comment. The comment really covers both types. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:100: static Parameters WithSingleInterruption(); I think a comment for this function would be good; WithSingleInterruption is descriptive, but there's a lot of knobs to tweak, and it would be good for the user to know which way those knobs were tweaked when they use this semi-default parameter value. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:165: // of |offset|. I.e. |injected_errors| must be sorted by |offset|. I didn't actually get this implication; I thought that what you had above might mean just that if two injected errors might apply to a particular read, the first one encountered would win. On second thought, that doesn't make sense, but it might still be worthwhile to clarify the first sentence ("Injected errors are considered..."). Maybe something like "process in the order ... following errors aren't considered until earlier ones have been executed." Or you could just leave it like this; this paragraph is very clear. But without this paragraph, I wouldn't have realized what it's saying, so I'm not sure the implication follows naturally. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:187: int64_t transferred_content_length = -1; nit: I'm having an instinctive question as to whether this is the transferred content length from the server perspective or the client perspective (which may really truly not matter--they should always be identical, given that we're faking out the network) and whether this is before or after content encoding (which may also not matter, if we never use content encoding). Still, I figured I'd raise that I have questions when I read this structure and let you decide what to do about it :-}. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:226: // interception won't be updated until the posted task executes. I presume it's post-and-forget rather than blocking? https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:266: // * |seed| is chaotic. I don't know what this means? I can sorta understand what a chaotic pattern would mean, but I think that's basically covered by the aperiodicity. What does |seed| being chaotic mean? https://codereview.chromium.org/1203983004/diff/140001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/140001/content/public/test/te... content/public/test/test_download_request_handler.cc:518: struct TestDownloadRequestHandler::InterceptorProxy { Yuck. I take it declaring a weak pointer requires the class to be fully declared? (ETA: I think it's worth a comment here as to why the wrapping; otherwise, each person who reads this file in the future is going to go through the same reference chase I just did.)
I just landed https://codereview.chromium.org/1439953006/. Hope it will stick :) You might want to rebase.
https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/100001/content/public/test/te... content/public/test/test_download_request_handler.h:72: Parameters(const Parameters& other); On 2015/11/17 at 22:21:42, rdsmith wrote: > On 2015/11/13 21:40:02, asanka wrote: > > On 2015/11/12 at 00:57:57, rdsmith wrote: > > > suggestion: I'm tempted to ask for a comment calling out copy construction, > > here or at the end of the class, just because the default for chrome is to > > disallow that. > > > > > > Also: Copy constructor without assignment operator seems weird; if one can > > default, why can't both? Conversely, if you're going to enable one, enabling > > the other seems wise? > > > > Thanks for calling this out. During an intermediate stage, the compiler decided > > it couldn't generate a default copy constructor. But now it can. Removed the > > copy constructor. > > I read the style guide as recommending but not requiring explicitly declaring default copy constructors and assignment operators with = default. Up to you, just wanted to make sure you were aware of that recommend-but-not-require (https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types ) Thanks for pointing this out. I've added the default constructors and operators for this class. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:134: // asynchronously in order to avoid unexpected re-entrancy. On 2015/11/17 at 22:21:42, rdsmith wrote: > I don't understand this sentence; what does avoiding re-entrancy have to do with avoiding network requests? Had we hit the network, we are likely to block and end up completing asynchronously. Since we've avoided all that blocking work, Start() can complete synchronously. Instead of completing synchronously, PartialResponseJob is going to post a message and simulate a asynchronous completion. This makes the call behave slightly more like a real network request. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:150: void StoreValueAndInvokeClosure(const base::Closure& quit_closure, On 2015/11/17 at 22:21:42, rdsmith wrote: > nitty nit: The name "quit_closure" presumes how this function will be used. Renamed to 'closure'. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:157: // Xorshift* PRNG from https://en.wikipedia.org/wiki/Xorshift On 2015/11/17 at 22:21:42, rdsmith wrote: > Did not review except to validate that it looked appropriately arcane :-}. :-P https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:60: class TestDownloadRequestHandler { On 2015/11/17 at 22:21:42, rdsmith wrote: > Is there a comment as to the thread home of this class and I missed it? Added one. It can be used on any thread, however the class itself isn't thread safe. Also added thread safety checking for TestDownloadRequestHandler. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:65: // A callback of this type can be used to intercept the Start() event of a new On 2015/11/17 at 22:21:42, rdsmith wrote: > nit, suggestion: "this type" -> "type OnStartHandler" and move the definition of OnStartResponseCallback to below this comment. The comment really covers both types. Done. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:100: static Parameters WithSingleInterruption(); On 2015/11/17 at 22:21:42, rdsmith wrote: > I think a comment for this function would be good; WithSingleInterruption is descriptive, but there's a lot of knobs to tweak, and it would be good for the user to know which way those knobs were tweaked when they use this semi-default parameter value. Done. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:165: // of |offset|. I.e. |injected_errors| must be sorted by |offset|. On 2015/11/17 at 22:21:43, rdsmith wrote: > I didn't actually get this implication; I thought that what you had above might mean just that if two injected errors might apply to a particular read, the first one encountered would win. On second thought, that doesn't make sense, but it might still be worthwhile to clarify the first sentence ("Injected errors are considered..."). Maybe something like "process in the order ... following errors aren't considered until earlier ones have been executed." > > Or you could just leave it like this; this paragraph is very clear. But without this paragraph, I wouldn't have realized what it's saying, so I'm not sure the implication follows naturally. I tried to clarify it a little bit. See if it makes more sense now. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:187: int64_t transferred_content_length = -1; On 2015/11/17 at 22:21:42, rdsmith wrote: > nit: I'm having an instinctive question as to whether this is the transferred content length from the server perspective or the client perspective (which may really truly not matter--they should always be identical, given that we're faking out the network) and whether this is before or after content encoding (which may also not matter, if we never use content encoding). Still, I figured I'd raise that I have questions when I read this structure and let you decide what to do about it :-}. Good point. I renamed this to transferred_byte_count and documented that this is the count of bytes received by the client of URLRequestJob after decoding. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:226: // interception won't be updated until the posted task executes. On 2015/11/17 at 22:21:42, rdsmith wrote: > I presume it's post-and-forget rather than blocking? Post-and-forget. Added a short note. https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.h:266: // * |seed| is chaotic. On 2015/11/17 at 22:21:42, rdsmith wrote: > I don't know what this means? I can sorta understand what a chaotic pattern would mean, but I think that's basically covered by the aperiodicity. What does |seed| being chaotic mean? Updated comment. Hopefully it clarifies the comment? The new comment reads: * |seed| is chaotic. Different seeds produce "very different" data. This means that there's no trivial mapping between the sequences generated by two distinct seeds. This property means that we'd be able to tell if resumption causes different byte streams to be mistakenly mixed together. https://codereview.chromium.org/1203983004/diff/140001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/140001/content/public/test/te... content/public/test/test_download_request_handler.cc:518: struct TestDownloadRequestHandler::InterceptorProxy { On 2015/11/17 at 22:21:43, rdsmith wrote: > Yuck. I take it declaring a weak pointer requires the class to be fully declared? > > (ETA: I think it's worth a comment here as to why the wrapping; otherwise, each person who reads this file in the future is going to go through the same reference chase I just did.) It was a compromise between a lot of verbosity in the .cc file and an extra level of indirection. I've removed the proxy. In order to reduce the verbosity somewhat, I opted to calling the interceptor Interceptor again. But this time it has the TestDownloadRequestHandler:: qualifier.
https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:134: // asynchronously in order to avoid unexpected re-entrancy. On 2015/11/23 22:04:59, asanka wrote: > On 2015/11/17 at 22:21:42, rdsmith wrote: > > I don't understand this sentence; what does avoiding re-entrancy have to do > with avoiding network requests? > > Had we hit the network, we are likely to block and end up completing > asynchronously. Since we've avoided all that blocking work, Start() can complete > synchronously. Instead of completing synchronously, PartialResponseJob is going > to post a message and simulate a asynchronous completion. This makes the call > behave slightly more like a real network request. Gotcha. Suggestion, nit: add ".. is avoiding network requests *and hence may complete synchronously*, it schedules ...". https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... content/public/test/test_download_request_handler.cc:234: interceptor_->GetClientTaskRunner()->PostTask( Comment somewhere (here or at declaration of interceptor_) as to why/when it's safe to indirect through the weak pointer without checking to see if it's null? https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... content/public/test/test_download_request_handler.h:70: // created on. Once the callback has a response ready, it can invoke the nit: "thread on which TestDownloadHandler was created."
https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/120001/content/public/test/te... content/public/test/test_download_request_handler.cc:134: // asynchronously in order to avoid unexpected re-entrancy. On 2015/11/28 at 00:38:19, rdsmith wrote: > On 2015/11/23 22:04:59, asanka wrote: > > On 2015/11/17 at 22:21:42, rdsmith wrote: > > > I don't understand this sentence; what does avoiding re-entrancy have to do > > with avoiding network requests? > > > > Had we hit the network, we are likely to block and end up completing > > asynchronously. Since we've avoided all that blocking work, Start() can complete > > synchronously. Instead of completing synchronously, PartialResponseJob is going > > to post a message and simulate a asynchronous completion. This makes the call > > behave slightly more like a real network request. > > Gotcha. Suggestion, nit: add ".. is avoiding network requests *and hence may complete synchronously*, it schedules ...". Done. https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... content/public/test/test_download_request_handler.cc:234: interceptor_->GetClientTaskRunner()->PostTask( On 2015/11/28 at 00:38:19, rdsmith wrote: > Comment somewhere (here or at declaration of interceptor_) as to why/when it's safe to indirect through the weak pointer without checking to see if it's null? It's not safe since they are not lifetime-linked. We are checking for the !interceptor_.get() call in line 228. https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... content/public/test/test_download_request_handler.h:70: // created on. Once the callback has a response ready, it can invoke the On 2015/11/28 at 00:38:19, rdsmith wrote: > nit: "thread on which TestDownloadHandler was created." I've done the dangling preposition away with.
lgtm https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... File content/public/test/test_download_request_handler.cc (right): https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... content/public/test/test_download_request_handler.cc:234: interceptor_->GetClientTaskRunner()->PostTask( On 2015/11/30 18:00:10, asanka wrote: > On 2015/11/28 at 00:38:19, rdsmith wrote: > > Comment somewhere (here or at declaration of interceptor_) as to why/when it's > safe to indirect through the weak pointer without checking to see if it's null? > > It's not safe since they are not lifetime-linked. We are checking for the > !interceptor_.get() call in line 228. Ah, whoops, missed that. Thanks. https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/180001/content/public/test/te... content/public/test/test_download_request_handler.h:70: // created on. Once the callback has a response ready, it can invoke the On 2015/11/30 18:00:10, asanka wrote: > On 2015/11/28 at 00:38:19, rdsmith wrote: > > nit: "thread on which TestDownloadHandler was created." > > I've done the dangling preposition away with. :-}.
Description was changed from ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,phajdan.jr@chromium.org BUG=215894 BUG=493347 BUG=7648 ========== to ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,sky@chromium.org BUG=215894 BUG=493347 BUG=7648 ==========
asanka@chromium.org changed reviewers: + sky@chromium.org - phajdan.jr@chromium.org
sky: Could you take a look at content/public/test/test_download_request_handler.* ? Justification for its existence is explained at the top of test_download_request_handler.h.
https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... content/public/test/test_download_request_handler.h:5: #ifndef CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_ Is there a reason this class needs to live in content? Can't it live next to the only that that is using it?
https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... File content/public/test/test_download_request_handler.h (right): https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... content/public/test/test_download_request_handler.h:5: #ifndef CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_ On 2015/12/01 at 21:58:50, sky wrote: > Is there a reason this class needs to live in content? Can't it live next to the only that that is using it? We want to also switch over //chrome/browser/download/download_browsertest.cc to use this when we add automatic resumption in //chrome. Automatic resumption relies on a number of signals (user preferences, connection type changes, etc..) that are exposed at //chrome and not //content.
On 2015/12/02 15:41:34, asanka wrote: > https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... > File content/public/test/test_download_request_handler.h (right): > > https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... > content/public/test/test_download_request_handler.h:5: #ifndef > CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_ > On 2015/12/01 at 21:58:50, sky wrote: > > Is there a reason this class needs to live in content? Can't it live next to > the only that that is using it? > > We want to also switch over //chrome/browser/download/download_browsertest.cc to > use this when we add automatic resumption in //chrome. Automatic resumption > relies on a number of signals (user preferences, connection type changes, etc..) > that are exposed at //chrome and not //content. Unless I'm missing something download_browsertest.cc is the only place using TestDownloadRequestHandler. If no one else is going to use TestDownloadRequestHandler, can't it live in chrome/browser/download?
On 2015/12/02 18:30:57, sky wrote: > On 2015/12/02 15:41:34, asanka wrote: > > > https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... > > File content/public/test/test_download_request_handler.h (right): > > > > > https://codereview.chromium.org/1203983004/diff/200001/content/public/test/te... > > content/public/test/test_download_request_handler.h:5: #ifndef > > CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_ > > On 2015/12/01 at 21:58:50, sky wrote: > > > Is there a reason this class needs to live in content? Can't it live next to > > the only that that is using it? > > > > We want to also switch over //chrome/browser/download/download_browsertest.cc > to > > use this when we add automatic resumption in //chrome. Automatic resumption > > relies on a number of signals (user preferences, connection type changes, > etc..) > > that are exposed at //chrome and not //content. > > Unless I'm missing something download_browsertest.cc is the only place using > TestDownloadRequestHandler. If no one else is going to use > TestDownloadRequestHandler, can't it live in chrome/browser/download? Are you conflating content/browser/download_browsertest.cc and chrome/browser/download_browsertest.cc? (MInor mea culpa--I really should have named them different things in the great chrome/content split.)
I likely am: LGTM
Thanks everyone!
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1203983004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1203983004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1203983004/#ps260001 (title: "Catch up with ReadRawData changes and add MSVC friendly move assignment operator.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1203983004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1203983004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1203983004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1203983004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1203983004/#ps280001 (title: "MSVC doesn't want to make default move constructors either.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1203983004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1203983004/280001
The CQ bit was unchecked by asanka@chromium.org
The CQ bit was checked by asanka@chromium.org
The CQ bit was unchecked by asanka@chromium.org
The CQ bit was checked by asanka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1203983004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1203983004/280001
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,sky@chromium.org BUG=215894 BUG=493347 BUG=7648 ========== to ========== Stop using SpawnedTestServer in DownloadContentTest.* This CL introduces TestDownloadRequestHandler, a URLRequestJob based interceptor that can be used to mock URLRequest behavior. The new request handler supports simulating network and server errors, and is a replacement for the testserver.py based test setup //content is currently using to test download resumption. Moving away from SpawnedTestServer allows us to re-enable the tests on Android. R=rdsmith@chromium.org,svaldez@chromium.org,sky@chromium.org BUG=215894 BUG=493347 BUG=7648 Committed: https://crrev.com/c7bf9bf1624d05928bbd4ee58d2c5e377fd6cd04 Cr-Commit-Position: refs/heads/master@{#363108} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c7bf9bf1624d05928bbd4ee58d2c5e377fd6cd04 Cr-Commit-Position: refs/heads/master@{#363108} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
