Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1525753002:
Test both download resumption code paths. (Closed)
Description[Downloads] Test both download resumption code paths.
Download resumption can take two different code paths depending on
whether the WebContents that initiated the download is still around when
the download is resumed or not. Make our tests go through both code
paths based on a parameterized test.
R=svaldez@chromium.org
BUG=7648
Committed: https://crrev.com/d738ccdbf70dff7b1aef51a7b01b084e27088d58
Cr-Commit-Position: refs/heads/master@{#365153}
Patch Set 1 #
Total comments: 12
Patch Set 2 : A couple of renames. #
Dependent Patchsets: Messages
Total messages: 16 (6 generated)
PTAL?
https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:751: ::testing::Values(DownloadResumptionTestType::RESUME_WITH_RENDERER, Is the :: at the beginning necessary? https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1437: CreateWaiter(shell(), 1)); download_shell()? https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1439: StartDownloadAndReturnItem(shell(), request_handler.url())); download_shell()? https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1500: NavigateToURLAndWaitForDownload(shell(), request_handler.url(), download_shell()?
Let me know if there are other readability issues you spot that might make it easier to grasp what's going on. The names are currently a bit confusing. I'll update the CL and ping you again. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:751: ::testing::Values(DownloadResumptionTestType::RESUME_WITH_RENDERER, On 2015/12/14 21:18:15, svaldez wrote: > Is the :: at the beginning necessary? Not strictly necessary, but it's common for the testing namespace to use them since it's a namespace defined by a third party library and shouldn't conflict with any secondary namespaces that are also named 'testing'. See line 55 etc. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1437: CreateWaiter(shell(), 1)); On 2015/12/14 21:18:15, svaldez wrote: > download_shell()? This isn't a resumption test, and hence doesn't need to distinguish between an initiator shell and the shell provided by the outer test fixture. It is unfortunately sandwiched between two resumption tests, but I didn't move it in this CL. I can move this out if you think it's hurting readability. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1439: StartDownloadAndReturnItem(shell(), request_handler.url())); On 2015/12/14 21:18:15, svaldez wrote: > download_shell()? Ditto. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1500: NavigateToURLAndWaitForDownload(shell(), request_handler.url(), On 2015/12/14 21:18:15, svaldez wrote: > download_shell()? This call is intentionally using shell() instead of download_shell() since its usage doesn't need a transient initiator shell. Also, once PrepareToResume() is called download_shell() may no longer exist.
Updated. Could you have another look?
lgtm https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... File content/browser/download/download_browsertest.cc (right): https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:751: ::testing::Values(DownloadResumptionTestType::RESUME_WITH_RENDERER, On 2015/12/14 21:31:02, asanka wrote: > On 2015/12/14 21:18:15, svaldez wrote: > > Is the :: at the beginning necessary? > > Not strictly necessary, but it's common for the testing namespace to use them > since it's a namespace defined by a third party library and shouldn't conflict > with any secondary namespaces that are also named 'testing'. See line 55 etc. Acknowledged. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1437: CreateWaiter(shell(), 1)); On 2015/12/14 21:31:02, asanka wrote: > On 2015/12/14 21:18:15, svaldez wrote: > > download_shell()? > > This isn't a resumption test, and hence doesn't need to distinguish between an > initiator shell and the shell provided by the outer test fixture. > > It is unfortunately sandwiched between two resumption tests, but I didn't move > it in this CL. I can move this out if you think it's hurting readability. Ah, okay. Might be better to move it out from between the resumption tests, but I don't think it hurts readability too much if we just leave it here. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1439: StartDownloadAndReturnItem(shell(), request_handler.url())); On 2015/12/14 21:31:02, asanka wrote: > On 2015/12/14 21:18:15, svaldez wrote: > > download_shell()? > > Ditto. Acknowledged. https://codereview.chromium.org/1525753002/diff/1/content/browser/download/do... content/browser/download/download_browsertest.cc:1500: NavigateToURLAndWaitForDownload(shell(), request_handler.url(), On 2015/12/14 21:31:02, asanka wrote: > On 2015/12/14 21:18:15, svaldez wrote: > > download_shell()? > > This call is intentionally using shell() instead of download_shell() since its > usage doesn't need a transient initiator shell. > > Also, once PrepareToResume() is called download_shell() may no longer exist. Acknowledged.
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/1525753002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525753002/20001
Description was changed from ========== Test both download resumption code paths. Download resumption can take two different code paths depending on whether the WebContents that initiated the download is still around when the download is resumed or not. Make our tests go through both code paths based on a parameterized test. R=svaldez@chromium.org BUG=7648 ========== to ========== [Downloads] Test both download resumption code paths. Download resumption can take two different code paths depending on whether the WebContents that initiated the download is still around when the download is resumed or not. Make our tests go through both code paths based on a parameterized test. R=svaldez@chromium.org BUG=7648 ==========
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525753002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525753002/20001
Message was sent while issue was closed.
Description was changed from ========== [Downloads] Test both download resumption code paths. Download resumption can take two different code paths depending on whether the WebContents that initiated the download is still around when the download is resumed or not. Make our tests go through both code paths based on a parameterized test. R=svaldez@chromium.org BUG=7648 ========== to ========== [Downloads] Test both download resumption code paths. Download resumption can take two different code paths depending on whether the WebContents that initiated the download is still around when the download is resumed or not. Make our tests go through both code paths based on a parameterized test. R=svaldez@chromium.org BUG=7648 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Downloads] Test both download resumption code paths. Download resumption can take two different code paths depending on whether the WebContents that initiated the download is still around when the download is resumed or not. Make our tests go through both code paths based on a parameterized test. R=svaldez@chromium.org BUG=7648 ========== to ========== [Downloads] Test both download resumption code paths. Download resumption can take two different code paths depending on whether the WebContents that initiated the download is still around when the download is resumed or not. Make our tests go through both code paths based on a parameterized test. R=svaldez@chromium.org BUG=7648 Committed: https://crrev.com/d738ccdbf70dff7b1aef51a7b01b084e27088d58 Cr-Commit-Position: refs/heads/master@{#365153} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d738ccdbf70dff7b1aef51a7b01b084e27088d58 Cr-Commit-Position: refs/heads/master@{#365153} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
