Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1542063003:
[download] Make DownloadItemObserver be owned by DownloadItem. (Closed)
Description[download] Make DownloadItemObserver be owned by DownloadItem.
Currently DownloadPathRerservationTracker creates a self owned
DownloadItem::Observer implementation called DownloadItemObserver. Make
this class be owned by DownloadItem instead via the SupportsUserData
interface.
By itself this change doesn't buy us much. However, when DownloadItems
start automatically resuming from interruptions without changing the
externally visible state, this ownership would automatically get us
lifetime managment as well as a guarantee that only one
DownloadItemObserver can exist per DownloadItem.
BUG=7648
Committed: https://crrev.com/fbf8cb0e546cd5d20d660a63c8cf36c38e38e680
Cr-Commit-Position: refs/heads/master@{#366654}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove unnecessary RemoveUserData() call. #Messages
Total messages: 16 (8 generated)
The CQ bit was checked by asanka@chromium.org to run a CQ dry run
Description was changed from ========== [download] Make DownloadItemObserver be owned by DownloadItem. Currently DownloadPathRerservationTracker creates a self owned DownloadItem::Observer implementation called DownloadItemObserver. Make this class be owned by DownloadItem instead via the SupportsUserData interface. By itself this change doesn't buy us much. However, when DownloadItems start automatically resuming from interruptions without changing the externally visible state, this ownership would automatically get us lifetime managment as well as a guarantee that only one DownloadItemObserver can exist per DownloadItem. BUG=7648 ========== to ========== [download] Make DownloadItemObserver be owned by DownloadItem. Currently DownloadPathRerservationTracker creates a self owned DownloadItem::Observer implementation called DownloadItemObserver. Make this class be owned by DownloadItem instead via the SupportsUserData interface. By itself this change doesn't buy us much. However, when DownloadItems start automatically resuming from interruptions without changing the externally visible state, this ownership would automatically get us lifetime managment as well as a guarantee that only one DownloadItemObserver can exist per DownloadItem. BUG=7648 ==========
asanka@chromium.org changed reviewers: + svaldez@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1542063003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1542063003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1542063003/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/1542063003/diff/1/chrome/browser/download/dow... chrome/browser/download/download_path_reservation_tracker.cc:348: void DownloadItemObserver::OnDownloadDestroyed(DownloadItem* download) { This is probably fine, though it seems odd to repeat code here and on the OnDownloadUpdated line when this is NOTREACHED.
https://codereview.chromium.org/1542063003/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/1542063003/diff/1/chrome/browser/download/dow... chrome/browser/download/download_path_reservation_tracker.cc:348: void DownloadItemObserver::OnDownloadDestroyed(DownloadItem* download) { On 2015/12/22 19:59:55, svaldez wrote: > This is probably fine, though it seems odd to repeat code here and on the > OnDownloadUpdated line when this is NOTREACHED. Yeah. I removed this call. Calling RemoveUserData() from here is unnecessary since the DownloadItem is going to be destroyed taking all the UserData along with it.
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/1542063003/#ps20001 (title: "Remove unnecessary RemoveUserData() call.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1542063003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1542063003/20001
Message was sent while issue was closed.
Description was changed from ========== [download] Make DownloadItemObserver be owned by DownloadItem. Currently DownloadPathRerservationTracker creates a self owned DownloadItem::Observer implementation called DownloadItemObserver. Make this class be owned by DownloadItem instead via the SupportsUserData interface. By itself this change doesn't buy us much. However, when DownloadItems start automatically resuming from interruptions without changing the externally visible state, this ownership would automatically get us lifetime managment as well as a guarantee that only one DownloadItemObserver can exist per DownloadItem. BUG=7648 ========== to ========== [download] Make DownloadItemObserver be owned by DownloadItem. Currently DownloadPathRerservationTracker creates a self owned DownloadItem::Observer implementation called DownloadItemObserver. Make this class be owned by DownloadItem instead via the SupportsUserData interface. By itself this change doesn't buy us much. However, when DownloadItems start automatically resuming from interruptions without changing the externally visible state, this ownership would automatically get us lifetime managment as well as a guarantee that only one DownloadItemObserver can exist per DownloadItem. 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 ========== [download] Make DownloadItemObserver be owned by DownloadItem. Currently DownloadPathRerservationTracker creates a self owned DownloadItem::Observer implementation called DownloadItemObserver. Make this class be owned by DownloadItem instead via the SupportsUserData interface. By itself this change doesn't buy us much. However, when DownloadItems start automatically resuming from interruptions without changing the externally visible state, this ownership would automatically get us lifetime managment as well as a guarantee that only one DownloadItemObserver can exist per DownloadItem. BUG=7648 ========== to ========== [download] Make DownloadItemObserver be owned by DownloadItem. Currently DownloadPathRerservationTracker creates a self owned DownloadItem::Observer implementation called DownloadItemObserver. Make this class be owned by DownloadItem instead via the SupportsUserData interface. By itself this change doesn't buy us much. However, when DownloadItems start automatically resuming from interruptions without changing the externally visible state, this ownership would automatically get us lifetime managment as well as a guarantee that only one DownloadItemObserver can exist per DownloadItem. BUG=7648 Committed: https://crrev.com/fbf8cb0e546cd5d20d660a63c8cf36c38e38e680 Cr-Commit-Position: refs/heads/master@{#366654} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fbf8cb0e546cd5d20d660a63c8cf36c38e38e680 Cr-Commit-Position: refs/heads/master@{#366654} |
