Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1544653002:
[Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. (Closed)
Description[Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem.
The RESUMING_INTERNAL state is used to indicate that the DownloadItem
has commenced a resumption attempt. The DownloadItem will remain in this
state until the network request receives a response.
Formerly, we treated this internal state as INTERRUPTED for all entities
that are observing the download item, including the UI. However, in
retrospect, this doesn't make sense and prevents the DownloadItem from
cleanly handling cases where it needs to transition back to an
interrupted state following a resumption attempt. In the latter case,
the download would otherwise try to transition from INTERRUPTED ->
INTERRUPTED which doesn't make a whole lot of sense to an outside
observer.
In addition, keeping the download in the INTERRUPTED state prevents an
outside observer from being able to cleanly tell when a resumption
attempt has begun. Furthermore, the INTERRUPTED state exhibited while an
automatic resumption is being attempted is not actionable to all outside
observers.
To rectify these, this change makes the RESUMING_INTERNAL state be
visible to external observers as IN_PROGRESS.
BUG=7648
R=svaldez@chromium.org
Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569
Cr-Commit-Position: refs/heads/master@{#366542}
Reverted due to failure in XP bots.
Reverted: https://crrev.com/5a45da8d4a87ffc6100ab8b684f3ad2eaef639b4
Cr-Commit-Position: refs/heads/master@{#366574}
Committed: https://crrev.com/0af68969edff080ce4298756432341b6cd7ddc5d
Cr-Commit-Position: refs/heads/master@{#366681}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments. #Patch Set 3 : Establishing dependency on other fixes. #
Depends on Patchset: Messages
Total messages: 26 (12 generated)
https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_item_impl_unittest.cc (left): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_item_impl_unittest.cc:569: ASSERT_EQ(1, observer.interrupt_count()); Shouldn't we keep this line with ASSERT_EQ(0? https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_item_impl_unittest.cc:582: CleanupItem(item, nullptr, DownloadItem::IN_PROGRESS); Should this be nullptr? We haven't Canceled DownloadItem in this test. https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_manager_impl.cc (left): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_manager_impl.cc:387: DCHECK_EQ(DownloadItem::INTERRUPTED, download->GetState()); Not DCHECK(IN_PROGRESS)?
https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_item_impl_unittest.cc (left): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_item_impl_unittest.cc:569: ASSERT_EQ(1, observer.interrupt_count()); On 2015/12/21 21:18:43, svaldez wrote: > Shouldn't we keep this line with ASSERT_EQ(0? I chose to remove it because the assertion isn't pertinent to what's being tested and only serves to make this a characterization test. The only thing that we really want to test is that Delegate::ResumeInterruptedDownload() was called with the correct URL. Arguably, we should also remove the CheckAndResetDownloadUpdated() assertion as well. So I removed that as well. As explained in the comment below, the verification of test expectations at tear down is sufficient for the test to fail if the correct ResumeInterruptedDownload() call was not seen. The CheckAndResetDownloadUpdated() is necessary for the ContinueAfterInterrupted test which specifically verifies the behavior of automatic resumption. https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_item_impl_unittest.cc:582: CleanupItem(item, nullptr, DownloadItem::IN_PROGRESS); On 2015/12/21 21:18:44, svaldez wrote: > Should this be nullptr? We haven't Canceled DownloadItem in this test. The DownloadResourceHandler/UrlDownloader and the DownloadFile are both associated with a single network request. When a DownloadItem no longer has an active network request (because it was cancelled, completed or interrupted), those objects go away. In this case, the download was interrupted, and hence the DownloadFile has also been destroyed already. A new one would be created once a server response is received for the resumption request. But we end the test before that point. Hence there's no DownloadFile here. https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_manager_impl.cc (left): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_manager_impl.cc:387: DCHECK_EQ(DownloadItem::INTERRUPTED, download->GetState()); On 2015/12/21 21:18:44, svaldez wrote: > Not DCHECK(IN_PROGRESS)? I chose to remove this because the MergeOriginInfoOnResume() call already has a more specific DCHECK for RESUMING_INTERNAL. However, having a DCHECK(IN_PROGRESS) here is defensive. So I added it.
lgtm https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_item_impl_unittest.cc (left): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_item_impl_unittest.cc:569: ASSERT_EQ(1, observer.interrupt_count()); On 2015/12/21 21:46:10, asanka wrote: > On 2015/12/21 21:18:43, svaldez wrote: > > Shouldn't we keep this line with ASSERT_EQ(0? > > I chose to remove it because the assertion isn't pertinent to what's being > tested and only serves to make this a characterization test. The only thing that > we really want to test is that Delegate::ResumeInterruptedDownload() was called > with the correct URL. > > Arguably, we should also remove the CheckAndResetDownloadUpdated() assertion as > well. So I removed that as well. As explained in the comment below, the > verification of test expectations at tear down is sufficient for the test to > fail if the correct ResumeInterruptedDownload() call was not seen. > > The CheckAndResetDownloadUpdated() is necessary for the ContinueAfterInterrupted > test which specifically verifies the behavior of automatic resumption. Acknowledged. https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_item_impl_unittest.cc (right): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_item_impl_unittest.cc:582: CleanupItem(item, nullptr, DownloadItem::IN_PROGRESS); On 2015/12/21 21:46:10, asanka wrote: > On 2015/12/21 21:18:44, svaldez wrote: > > Should this be nullptr? We haven't Canceled DownloadItem in this test. > > The DownloadResourceHandler/UrlDownloader and the DownloadFile are both > associated with a single network request. When a DownloadItem no longer has an > active network request (because it was cancelled, completed or interrupted), > those objects go away. In this case, the download was interrupted, and hence the > DownloadFile has also been destroyed already. A new one would be created once a > server response is received for the resumption request. But we end the test > before that point. Hence there's no DownloadFile here. Acknowledged. https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... File content/browser/download/download_manager_impl.cc (left): https://codereview.chromium.org/1544653002/diff/1/content/browser/download/do... content/browser/download/download_manager_impl.cc:387: DCHECK_EQ(DownloadItem::INTERRUPTED, download->GetState()); On 2015/12/21 21:46:11, asanka wrote: > On 2015/12/21 21:18:44, svaldez wrote: > > Not DCHECK(IN_PROGRESS)? > > I chose to remove this because the MergeOriginInfoOnResume() call already has a > more specific DCHECK for RESUMING_INTERNAL. > > However, having a DCHECK(IN_PROGRESS) here is defensive. So I added it. 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/1544653002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1544653002/20001
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/1544653002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1544653002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org ========== to ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1544743003/ by noel@chromium.org. The reason for reverting is: DownloadTest.Resumption_Automatic began failing in browser_tests on Windows-XP-SP3 after this patch landed. See: https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build... Reverting to see if we can green the chromium tree. .
Message was sent while issue was closed.
Description was changed from ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} ========== to ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} ==========
Description was changed from ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} ========== to ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} Reverted due to failure in XP bots. Reverted: https://crrev.com/5a45da8d4a87ffc6100ab8b684f3ad2eaef639b4 Cr-Commit-Position: refs/heads/master@{#366574} ==========
Let's see if https://codereview.chromium.org/1542063003/ resolved the issue. https://codereview.chromium.org/1549573002/ should make it much more likely that this will also trigger a failure in bots other than XP.
The CQ bit was checked by asanka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org Link to the patchset: https://codereview.chromium.org/1544653002/#ps40001 (title: "Establishing dependency on other fixes.")
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/1544653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1544653002/40001
Message was sent while issue was closed.
Description was changed from ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} Reverted due to failure in XP bots. Reverted: https://crrev.com/5a45da8d4a87ffc6100ab8b684f3ad2eaef639b4 Cr-Commit-Position: refs/heads/master@{#366574} ========== to ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} Reverted due to failure in XP bots. Reverted: https://crrev.com/5a45da8d4a87ffc6100ab8b684f3ad2eaef639b4 Cr-Commit-Position: refs/heads/master@{#366574} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} Reverted due to failure in XP bots. Reverted: https://crrev.com/5a45da8d4a87ffc6100ab8b684f3ad2eaef639b4 Cr-Commit-Position: refs/heads/master@{#366574} ========== to ========== [Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem. The RESUMING_INTERNAL state is used to indicate that the DownloadItem has commenced a resumption attempt. The DownloadItem will remain in this state until the network request receives a response. Formerly, we treated this internal state as INTERRUPTED for all entities that are observing the download item, including the UI. However, in retrospect, this doesn't make sense and prevents the DownloadItem from cleanly handling cases where it needs to transition back to an interrupted state following a resumption attempt. In the latter case, the download would otherwise try to transition from INTERRUPTED -> INTERRUPTED which doesn't make a whole lot of sense to an outside observer. In addition, keeping the download in the INTERRUPTED state prevents an outside observer from being able to cleanly tell when a resumption attempt has begun. Furthermore, the INTERRUPTED state exhibited while an automatic resumption is being attempted is not actionable to all outside observers. To rectify these, this change makes the RESUMING_INTERNAL state be visible to external observers as IN_PROGRESS. BUG=7648 R=svaldez@chromium.org Committed: https://crrev.com/ad29c15e3115a256cec6ee0ab0ba3138ef087569 Cr-Commit-Position: refs/heads/master@{#366542} Reverted due to failure in XP bots. Reverted: https://crrev.com/5a45da8d4a87ffc6100ab8b684f3ad2eaef639b4 Cr-Commit-Position: refs/heads/master@{#366574} Committed: https://crrev.com/0af68969edff080ce4298756432341b6cd7ddc5d Cr-Commit-Position: refs/heads/master@{#366681} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0af68969edff080ce4298756432341b6cd7ddc5d Cr-Commit-Position: refs/heads/master@{#366681} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
