Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
722953002:
downloads: add the ability to undo download removal. (Closed)
|
Created:
6 years, 1 month ago by Dan Beam Modified:
6 years ago CC:
chromium-reviews, extensions-reviews_chromium.org, benjhayden+dwatch_chromium.org, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-closure_chromium.org, Randy Smith (Not in Mondays), cbentzel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptiondownloads: add the ability to undo download removal.
Currently this works by pressing Ctrl+z or Cmd+z after removing a download.
We should probably show a banner or communicate to the user that this
functionality exists other than them guessing.
R=arv@chromium.org,benjhayden@chromium.org
BUG=428162
Committed: https://crrev.com/59b0be74d47f3bccceac31f205e515097079a0fa
Cr-Commit-Position: refs/heads/master@{#306549}
Patch Set 1 : whoops #
Total comments: 5
Patch Set 2 : comment #
Total comments: 23
Patch Set 3 : dcheck #
Total comments: 6
Patch Set 4 : fixes #
Total comments: 4
Patch Set 5 : asanka@ review #Patch Set 6 : test fixes #
Messages
Total messages: 45 (17 generated)
dbeam@chromium.org changed reviewers: + arv@chromium.org
R=arv@: chrome/browser/resources ui/webui R=benjhayden@: chrome/browser/downloads chrome/browser/extensions/api/downloads chrome/browser/ui/webui/downloads* content/browser/downloads CC=rdsmith@, asanka@ will add owners for content/public and chrome/browser/history if we like this way of doing things. https://codereview.chromium.org/722953002/diff/20001/chrome/browser/download/... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/722953002/diff/20001/chrome/browser/download/... chrome/browser/download/download_item_model.cc:536: case DownloadItem::REMOVED: not really sure what to do here... https://codereview.chromium.org/722953002/diff/20001/chrome/browser/download/... File chrome/browser/download/download_path_reservation_tracker.cc (right): https://codereview.chromium.org/722953002/diff/20001/chrome/browser/download/... chrome/browser/download/download_path_reservation_tracker.cc:330: // This is handled later by Remove(). should we change the state in the downloads database? maybe in the rare event that the browser crashes or something? (and ~DownloadsDOMHandler isn't run) https://codereview.chromium.org/722953002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/722953002/diff/20001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:192: COMPILE_ASSERT(arraysize(kStateStrings) == DownloadItem::MAX_DOWNLOAD_STATE - 1, i couldn't find this file and wasn't sure what to do here. please advise. https://codereview.chromium.org/722953002/diff/20001/chrome/browser/history/d... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/722953002/diff/20001/chrome/browser/history/d... chrome/browser/history/download_database.cc:98: case DownloadItem::REMOVED: i'll change this if we decide to sync this status to the database https://codereview.chromium.org/722953002/diff/20001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (left): https://codereview.chromium.org/722953002/diff/20001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:786: chrome.send('drag', [this.id_.toString()]); i don't think that |this.id_| being a double (a number in javascript) was actually safe. i've converted it to a string and am handling it slightly differently in C++ to accommodate. it seems unlikely that anybody would hit this in practice, though...
Patchset #1 (id:1) has been deleted
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
When does the file on disk actually go away?
Randy, the normal Remove() path is taken for REMOVED downloads when chrome://downloads is closed and DownloadsDOMHandler is destroyed. Randy, I think I recall you having a preference for adding a state for situations like this rather than adding a flag, but feel free to correct me. I'm also not sure what might break if a download is transitioned from complete/cancelled/interrupted to removed then back, so we might want to scan some of the GetState() callers. Dan, I probably could have figured out how you ensure some of your NOTREACHED(), but I'm feeling lazy. :-) https://codereview.chromium.org/722953002/diff/40001/chrome/browser/download/... File chrome/browser/download/download_item_model.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/download/... chrome/browser/download/download_item_model.cc:536: case DownloadItem::REMOVED: How do you ensure that this isn't reached? https://codereview.chromium.org/722953002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/downloads/downloads_api.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/downloads/downloads_api.cc:192: COMPILE_ASSERT(arraysize(kStateStrings) == DownloadItem::MAX_DOWNLOAD_STATE - 1, Please add a state string instead so that if another state is added in the future, we'll be able to expose it to extensions if we want. I'd like to expose the state to extensions as "removed" so that extensions get the a11y benefit as well. Perhaps the erase function can take a new bool option like "undoable"? And then maybe an undoErase function? You can TODO(benjhayden) those if you want. https://codereview.chromium.org/722953002/diff/40001/chrome/browser/history/d... File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/history/d... chrome/browser/history/download_database.cc:98: case DownloadItem::REMOVED: How do you ensure that this isn't reached? https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.html (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.html:35: <command id="clear-all-command" shortcut="Alt-U+0043"><!-- Alt+C --> Why not implement these entirely in js, why have html elements at all? https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:111: cr.ui.decorate('command', cr.ui.Command); How are these Commands better than onKeyDown? I don't mean to be as confrontational as text looks, I'm actually just trying to learn something new every day. :-) https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:233: case content::DownloadItem::REMOVED: How do you ensure that this isn't reached? https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:268: while (!removed_ids_.empty()) { Could you use DownloadQuery to find downloads whose state is removed instead of using removed_ids_? It might be slightly slower, but it would make it so that the information about which items are removed is in one place rather than two, which, besides being DRY, allows for the possibility that undoable remove could be exposed to extensions. https://codereview.chromium.org/722953002/diff/40001/content/browser/download... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/722953002/diff/40001/content/browser/download... content/browser/download/download_item_impl.cc:382: case REMOVED_INTERNAL: Out of order? Also, how do you ensure that this isn't reached? https://codereview.chromium.org/722953002/diff/40001/content/browser/download... content/browser/download/download_item_impl.cc:547: case REMOVED_INTERNAL: How do you ensure that this isn't reached? https://codereview.chromium.org/722953002/diff/40001/content/browser/download... content/browser/download/download_item_impl.cc:1581: prev_state_ = state_; I might want to be a bit more conservative here. I'd like to at least discourage future use of prev_state_ for anything other than UndoRemove(). Maybe rename it "removed_state_" and only set it when transitioning to REMOVED? https://codereview.chromium.org/722953002/diff/40001/content/public/browser/d... File content/public/browser/download_item.h (right): https://codereview.chromium.org/722953002/diff/40001/content/public/browser/d... content/public/browser/download_item.h:66: // The user removed this download from their history. Clarify that the state is not terminal? "The user removed this download from history but may undo the removal."? https://codereview.chromium.org/722953002/diff/40001/content/public/browser/d... content/public/browser/download_item.h:148: // Undoes the effects of MarkRemoved() and restores the download's |status_| This is an interface, so let's say something like "restores the value of GetState() to its prior value" to avoid implementation details?
WebUI LGTM https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (left): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:786: chrome.send('drag', [this.id_.toString()]); ha. those were the days when chrome.send only supported strings ;-) https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:111: cr.ui.decorate('command', cr.ui.Command); On 2014/11/13 18:01:59, benjhayden_chromium wrote: > How are these Commands better than onKeyDown? > I don't mean to be as confrontational as text looks, I'm actually just trying to > learn something new every day. :-) Do a search for Command Pattern UI. There's lot of material out there explaining its uses and benefits. I added the command pattern for the BMM where it was used for different menu items as well as keyboard shortcuts. https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:135: file_value->SetString("id", base::Uint64ToString(download_item->GetId())); JS numbers cannot represent Uint64. This might lead to trouble down in the future. https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:501: void DownloadsDOMHandler::HandleUndoRemove(const base::ListValue* args) { Don't we want to send an ID of some kind. My concern is with having multiple download managers open at the same time. https://codereview.chromium.org/722953002/diff/40001/content/browser/download... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/722953002/diff/40001/content/browser/download... content/browser/download/download_item_impl.cc:442: if (state_ != REMOVED_INTERNAL) { Why not DCHECK if this stat cannot happen? https://codereview.chromium.org/722953002/diff/40001/content/browser/download... File content/browser/download/download_item_impl.h (right): https://codereview.chromium.org/722953002/diff/40001/content/browser/download... content/browser/download/download_item_impl.h:294: // INTERRUPTED_INTERNAL This comment is confusing.
asanka@chromium.org changed reviewers: + asanka@chromium.org
Have you considered just delaying the Remove() call and showing a mini "Undo" banner in the interim like the popular Gmail feature? I'm opposed to adding a new REMOVED state. The download's state affects a number of processes that are observing the download and responds to the state transition. It seems the REMOVED state should be interpreted by observers as "act as if the download state didn't change." Otherwise undoing a remove would not be semantically equivalent to not having removed it in the first place. This is not a good API and is prone to errors.
On 2014/11/13 19:59:54, asanka wrote: > Have you considered just delaying the Remove() call and showing a mini "Undo" > banner in the interim like the popular Gmail feature? I had thought that we wanted to avoid implementing logic in downloads.js or DownloadsDOMHandler, and keep them simple views. > > I'm opposed to adding a new REMOVED state. The download's state affects a number > of processes that are observing the download and responds to the state > transition. It seems the REMOVED state should be interpreted by observers as > "act as if the download state didn't change." Otherwise undoing a remove would > not be semantically equivalent to not having removed it in the first place. This > is not a good API and is prone to errors. Now that you mention it, yeah, there don't seem to be any observers who should care about the state. And I can imagine extensions implementing this completely in javascript. Sorry, Dan, I steered you wrong. Asanka, would you have this implemented entirely in DownloadsDOMHandler, or is there a better approach?
On 2014/11/13 20:12:41, benjhayden_chromium wrote: > On 2014/11/13 19:59:54, asanka wrote: > > Have you considered just delaying the Remove() call and showing a mini "Undo" > > banner in the interim like the popular Gmail feature? > > I had thought that we wanted to avoid implementing logic in downloads.js or > DownloadsDOMHandler, and keep them simple views. As implemented, the download item is merely holding on to state. The logic still necessarily remains in DownloadsDOMHandler because it alone knows when to actually delete the download. Noone else knows what to do with a REMOVED state other than ignore it. My suggestion was to just invoke Remove when DDH decides it's time to commit to removing the download. Keeping shared state makes sense if all attached UIs / consumers are able and prepared to deal with the state (e.g. a dangerous download or an interruption). > > I'm opposed to adding a new REMOVED state. The download's state affects a > number > > of processes that are observing the download and responds to the state > > transition. It seems the REMOVED state should be interpreted by observers as > > "act as if the download state didn't change." Otherwise undoing a remove would > > not be semantically equivalent to not having removed it in the first place. > This > > is not a good API and is prone to errors. > > Now that you mention it, yeah, there don't seem to be any observers who should > care about the state. > And I can imagine extensions implementing this completely in javascript. > Sorry, Dan, I steered you wrong. > > Asanka, would you have this implemented entirely in DownloadsDOMHandler, or is > there a better approach? Yeah, my preference is for this to be implemented in DDH for reasons pointed out above. dbeam: Has UI folks signed off on this? A couple of thoughts: * While an undo hotkey is great, it's not discoverable. * When it's invoked it undoes all deletions performed during the lifetime of the downloads page. My expectation, if I were to learn that the undo key works on the downloads page, would be for it undo the most recent removal rather than everything so far. * There are other undoable operations that can be performed on the downloads page (accepting a dangerous download, clear all). It might be that the most recent action that the user expects to undo isn't remove. * Currently Ctrl+Z undoes edits to the search box. It looks like that would no longer work (?). Granted, It's possible nobody would notice.
On 2014/11/13 19:59:54, asanka wrote:
> Have you considered just delaying the Remove() call and showing a mini "Undo"
> banner in the interim like the popular Gmail feature?
I don't like mission impossible dialogs ("this message will self-destruct
in..."). I often feel stressed while seeing them as it forces me to make a
choice quickly (psychologically it's [mildly] taxing). However, I think there's
some key differences here.
When you delete in Gmail, nothing's actually final for a few days (things stay
in the trash in the case of accidentally deletion).
When you remove a most visited tab from the New Tab page, it shows an "Undo" or
"Restore all" prompt for ~10s BUT you're not actually doing anything destructive
(you can always "Restore all", it's just a blacklist).
When you remove a download, it's currently permanent and confirmation-less.
This isn't the end of the world, but we created chrome://downloads because this
information is useful (and you're nuking it press haphazardly). For example,
Alt+c might nuke ALL your downloads without confirmation or any understanding of
what you did without a way to undo (we may want to handle that separately or
just show a confirm dialog).
>
> I'm opposed to adding a new REMOVED state. The download's state affects a
number
> of processes that are observing the download and responds to the state
> transition. It seems the REMOVED state should be interpreted by observers as
> "act as if the download state didn't change." Otherwise undoing a remove would
> not be semantically equivalent to not having removed it in the first place.
This
> is not a good API and is prone to errors.
there seems to be 2 options (as I discussed with benjhayden@):
1) real removal (Remove()) + revival (keep an ScopedVector<DownloadItem> and
re-insert on undo)
2) soft removal (MarkRemoved()) + deferred deletion (Remove() on destruction or
something)
benjhayden@ told me it'd be hard to revive objects so I went do this path first
(and the code to get it working seems simple enough). I'd be happy to update
all the native UIs to handle this correctly and/or change various
On*Removed()/On*Updated() observations to deal with the new paradigm.
Alternatively, we can change to option #1 (real removal + revival).
On 2014/11/13 21:24:00, asanka wrote: > On 2014/11/13 20:12:41, benjhayden_chromium wrote: > > On 2014/11/13 19:59:54, asanka wrote: > > > Have you considered just delaying the Remove() call and showing a mini > "Undo" > > > banner in the interim like the popular Gmail feature? > > > > I had thought that we wanted to avoid implementing logic in downloads.js or > > DownloadsDOMHandler, and keep them simple views. > > As implemented, the download item is merely holding on to state. The logic still > necessarily remains in DownloadsDOMHandler because it alone knows when to > actually delete the download. Noone else knows what to do with a REMOVED state > other than ignore it. My suggestion was to just invoke Remove when DDH decides > it's time to commit to removing the download. Keeping shared state makes sense > if all attached UIs / consumers are able and prepared to deal with the state > (e.g. a dangerous download or an interruption). as proposed, the DDH *does* decide when it's time to commit to removing download (when the user closes the page, i.e. ~DDH). > > > > I'm opposed to adding a new REMOVED state. The download's state affects a > > number > > > of processes that are observing the download and responds to the state > > > transition. It seems the REMOVED state should be interpreted by observers as > > > "act as if the download state didn't change." Otherwise undoing a remove > would > > > not be semantically equivalent to not having removed it in the first place. > > This > > > is not a good API and is prone to errors. > > > > Now that you mention it, yeah, there don't seem to be any observers who should > > care about the state. > > And I can imagine extensions implementing this completely in javascript. > > Sorry, Dan, I steered you wrong. > > > > Asanka, would you have this implemented entirely in DownloadsDOMHandler, or is > > there a better approach? > > Yeah, my preference is for this to be implemented in DDH for reasons pointed out > above. I'll look into the hard remove + revive. > > dbeam: Has UI folks signed off on this? A couple of thoughts: hwi@ is the UI person for a11y (the reason this is being implemented). she hasn't been incredibly involved because... there's no UI yet ;). i was just going to implement the feature as a sneaky shortcut but add the UI soon after. i'm happy to do it in the same CL, it just seems like a good logical division. > * While an undo hotkey is great, it's not discoverable. right, just like alt+c is (which we should improve). > * When it's invoked it undoes all deletions performed during the lifetime of the > downloads page. My expectation, if I were to learn that the undo key works on > the downloads page, would be for it undo the most recent removal rather than > everything so far. Currently it works like stack (last download is revived), afaict. I think more pressing UI questions are: a) should removal undoing be persistent (i.e. across DDH lifetime) b) should removal work on any chrome://downloads page (instead of just the one you remove from) c) what happens when a user undones Alt+c? d) should we put a confirmation in front of Alt+c e) how can we make this more discoverable > * There are other undoable operations that can be performed on the downloads > page (accepting a dangerous download, clear all). It might be that the most > recent action that the user expects to undo isn't remove. right, but this was the most destructive. the main intent is allowing assistive technology users to explore or at least undo a destructive action. i agree that switching a download from dangerous -> safe should probably be undo-able. > * Currently Ctrl+Z undoes edits to the search box. It looks like that would no > longer work (?). Granted, It's possible nobody would notice. it only undoes that when the focus is in the search box. there's already code in this CL to handle that: Downloads.prototype.onCanExecute_
On 2014/11/18 23:56:44, Dan Beam wrote: > a) should removal undoing be persistent (i.e. across DDH lifetime) > b) should removal work on any chrome://downloads page (instead of just the one > you remove from) I think these 2 questions will impact the design (would push more towards soft removal) so I'll ask hwi@ to clarify these first before we add a lot of churn/more reviewer effort. thank you everybody for your feedback.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
hwi@ got back to me: https://code.google.com/p/chromium/issues/detail?id=428162#c7 In general she preferred keeping a download around very temporarily and wanted to wait on showing a UI to undo. I've updated the code to do "hard removal" (e.g. actually Remove(), keep around the item with the ability to "revive" it until the page is closed). https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.html (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.html:35: <command id="clear-all-command" shortcut="Alt-U+0043"><!-- Alt+C --> On 2014/11/13 18:01:59, benjhayden_chromium wrote: > Why not implement these entirely in js, why have html elements at all? https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... File chrome/browser/resources/downloads/downloads.js (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/resources... chrome/browser/resources/downloads/downloads.js:111: cr.ui.decorate('command', cr.ui.Command); On 2014/11/13 19:02:30, arv wrote: > On 2014/11/13 18:01:59, benjhayden_chromium wrote: > > How are these Commands better than onKeyDown? > > I don't mean to be as confrontational as text looks, I'm actually just trying > to > > learn something new every day. :-) > > Do a search for Command Pattern UI. There's lot of material out there explaining > its uses and benefits. > > I added the command pattern for the BMM where it was used for different menu > items as well as keyboard shortcuts. what arv@ said + I wanted to share code https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:135: file_value->SetString("id", base::Uint64ToString(download_item->GetId())); On 2014/11/13 19:02:30, arv wrote: > JS numbers cannot represent Uint64. This might lead to trouble down in the > future. the JS only gets a string (now) https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:268: while (!removed_ids_.empty()) { On 2014/11/13 18:01:59, benjhayden_chromium wrote: > Could you use DownloadQuery to find downloads whose state is removed instead of > using removed_ids_? It might be slightly slower, but it would make it so that > the information about which items are removed is in one place rather than two, > which, besides being DRY, allows for the possibility that undoable remove could > be exposed to extensions. no because that'd give us all removed downloads, not those specific to this handler/page
https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/722953002/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/downloads_dom_handler.cc:268: while (!removed_ids_.empty()) { On 2014/11/19 23:09:32, Dan Beam wrote: > On 2014/11/13 18:01:59, benjhayden_chromium wrote: > > Could you use DownloadQuery to find downloads whose state is removed instead > of > > using removed_ids_? It might be slightly slower, but it would make it so that > > the information about which items are removed is in one place rather than two, > > which, besides being DRY, allows for the possibility that undoable remove > could > > be exposed to extensions. > > no because that'd give us all removed downloads, not those specific to this > handler/page whoops, meant to delete this -- the code (in general) has changed a lot so ptal at the updated patchset.
Patchset #3 (id:160001) has been deleted
https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (left): https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... content/browser/download/download_manager_impl.cc:348: downloads_.clear(); STLDeleteValues() does this already https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... content/browser/download/download_manager_impl.cc:248: DCHECK(removed_downloads_.empty()); I could change this to: STLDeleteValues(&removed_downloads_); if you want... but it shouldn't ever be necessary assuming all DownloadsDomHandlers go down before this class
dbeam@chromium.org changed reviewers: - asanka@chromium.org, rdsmith@chromium.org
benjhayden@chromium.org changed reviewers: + asanka@chromium.org
There are benefits and drawbacks to all possible approaches to this problem. Asanka is the one who is most likely to maintain this code and its interactions with the rest of the downloads system, so he should be primary reviewer. https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... content/browser/download/download_item_impl.cc:344: // We have now been deleted. I found these comments helpful. Why remove them? https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... content/browser/download/download_manager_impl.cc:613: FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, item)); Is every listener really ok with having a single item created more than once?
benjhayden@chromium.org changed reviewers: - benjhayden@chromium.org
https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... File content/browser/download/download_item_impl.cc (left): https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... content/browser/download/download_item_impl.cc:344: // We have now been deleted. On 2014/11/21 00:50:00, benjhayden_chromium wrote: > I found these comments helpful. Why remove them? because (with this change) they're no longer correct https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/722953002/diff/180001/content/browser/downloa... content/browser/download/download_manager_impl.cc:613: FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, item)); On 2014/11/21 00:50:01, benjhayden_chromium wrote: > Is every listener really ok with having a single item created more than once? According to my interpretation of the documentation, yes. "this method may be called an arbitrary number of times" https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
The objective of the OnDownloadRemoved() notification is to signal that the download is permanently going away. This different from OnDownloadDestroyed() which means that the specific DownloadItem instance is going away, but the underlying download is still persistent. In response to OnDownloadRemoved() a consumer of the API (say SafeBrowsing or the extensions system) could remove any associated data that they were keeping. This CL is changing the meaning of OnDownloadRemoved() and the concept of 'remove' in general. As with the previous patchset, the safe way for a consumer to respond to OnDownloadRemoved() is to assume that it wasn't removed and then wait for an OnDownloadDestroyed() or OnDownloadRevived(). Even then, the cycling of Remove() -> Revive() only works for a download that has completed. Any inprogress download would, currently, be permanently damaged during the Remove() process. I.e. the underlying request would've been terminated and the file deleted from disk. Revival wouldn't do what the user expects. I remain skeptical of this approach of deleting and reviving at a low level. I still feel that the safe and clean method to implement this is to delay calling Remove() until the caller is ready to commit to the removal. Undo shouldn't be a feature exposed at the DownloadItem or DownloadManager layer. At that level, you can restore the state of the DownloadItemImpl object, but you can't undo the effects of any actions taken by consumers of the downloads system in response to the removal.
On 2014/11/21 20:01:06, asanka wrote: > The objective of the OnDownloadRemoved() notification is to signal that the > download is permanently going away. This different from OnDownloadDestroyed() > which means that the specific DownloadItem instance is going away, but the > underlying download is still persistent. > > In response to OnDownloadRemoved() a consumer of the API (say SafeBrowsing or > the extensions system) could remove any associated data that they were keeping. > This CL is changing the meaning of OnDownloadRemoved() and the concept of > 'remove' in general. As with the previous patchset, the safe way for a consumer > to respond to OnDownloadRemoved() is to assume that it wasn't removed and then > wait for an OnDownloadDestroyed() or OnDownloadRevived(). > > Even then, the cycling of Remove() -> Revive() only works for a download that > has completed. Any inprogress download would, currently, be permanently damaged > during the Remove() process. I.e. the underlying request would've been > terminated and the file deleted from disk. Revival wouldn't do what the user > expects. > > I remain skeptical of this approach of deleting and reviving at a low level. I > still feel that the safe and clean method to implement this is to delay calling > Remove() until the caller is ready to commit to the removal. Undo shouldn't be a > feature exposed at the DownloadItem or DownloadManager layer. At that level, you > can restore the state of the DownloadItemImpl object, but you can't undo the > effects of any actions taken by consumers of the downloads system in response to > the removal. how do other simultaneous download UIs hide this item?
Patchset #4 (id:200001) has been deleted
On 2014/11/21 20:12:59, Dan Beam wrote: > On 2014/11/21 20:01:06, asanka wrote: > > The objective of the OnDownloadRemoved() notification is to signal that the > > download is permanently going away. This different from OnDownloadDestroyed() > > which means that the specific DownloadItem instance is going away, but the > > underlying download is still persistent. > > > > In response to OnDownloadRemoved() a consumer of the API (say SafeBrowsing or > > the extensions system) could remove any associated data that they were > keeping. > > This CL is changing the meaning of OnDownloadRemoved() and the concept of > > 'remove' in general. As with the previous patchset, the safe way for a > consumer > > to respond to OnDownloadRemoved() is to assume that it wasn't removed and then > > wait for an OnDownloadDestroyed() or OnDownloadRevived(). > > > > Even then, the cycling of Remove() -> Revive() only works for a download that > > has completed. Any inprogress download would, currently, be permanently > damaged > > during the Remove() process. I.e. the underlying request would've been > > terminated and the file deleted from disk. Revival wouldn't do what the user > > expects. > > > > I remain skeptical of this approach of deleting and reviving at a low level. I > > still feel that the safe and clean method to implement this is to delay > calling > > Remove() until the caller is ready to commit to the removal. Undo shouldn't be > a > > feature exposed at the DownloadItem or DownloadManager layer. At that level, > you > > can restore the state of the DownloadItemImpl object, but you can't undo the > > effects of any actions taken by consumers of the downloads system in response > to > > the removal. > > how do other simultaneous download UIs hide this item? Not all UIs are going to be ready to undo a remove. That said, you can introduce a 'hide' flag that is either only known to DownloadsDOMHandler or is visible from DownloadItemModel.
On 2014/11/21 20:31:48, asanka wrote: > On 2014/11/21 20:12:59, Dan Beam wrote: > > On 2014/11/21 20:01:06, asanka wrote: > > > The objective of the OnDownloadRemoved() notification is to signal that the > > > download is permanently going away. This different from > OnDownloadDestroyed() > > > which means that the specific DownloadItem instance is going away, but the > > > underlying download is still persistent. > > > > > > In response to OnDownloadRemoved() a consumer of the API (say SafeBrowsing > or > > > the extensions system) could remove any associated data that they were > > keeping. > > > This CL is changing the meaning of OnDownloadRemoved() and the concept of > > > 'remove' in general. As with the previous patchset, the safe way for a > > consumer > > > to respond to OnDownloadRemoved() is to assume that it wasn't removed and > then > > > wait for an OnDownloadDestroyed() or OnDownloadRevived(). > > > > > > Even then, the cycling of Remove() -> Revive() only works for a download > that > > > has completed. Any inprogress download would, currently, be permanently > > damaged > > > during the Remove() process. I.e. the underlying request would've been > > > terminated and the file deleted from disk. Revival wouldn't do what the user > > > expects. > > > > > > I remain skeptical of this approach of deleting and reviving at a low level. > I > > > still feel that the safe and clean method to implement this is to delay > > calling > > > Remove() until the caller is ready to commit to the removal. Undo shouldn't > be > > a > > > feature exposed at the DownloadItem or DownloadManager layer. At that level, > > you > > > can restore the state of the DownloadItemImpl object, but you can't undo the > > > effects of any actions taken by consumers of the downloads system in > response > > to > > > the removal. > > > > how do other simultaneous download UIs hide this item? > > Not all UIs are going to be ready to undo a remove. Sure, but the chrome://downloads page that removes the item should be able to and all other chrome://downloads pages should update their content. I also assume the downloads shelf should be hidden or remove this download when "Remove this item" is clicked on chrome://downloads for a shelf-visible item (just like it currently does). > That said, you can introduce > a 'hide' flag that is either only known to DownloadsDOMHandler or is visible > from DownloadItemModel. It'd have to be known to *all* DownloadsDOMHandlers, right? So in the model seems like a better place.
On 2014/11/21 20:41:46, Dan Beam wrote: > On 2014/11/21 20:31:48, asanka wrote: > > On 2014/11/21 20:12:59, Dan Beam wrote: > > > On 2014/11/21 20:01:06, asanka wrote: > > > > The objective of the OnDownloadRemoved() notification is to signal that > the > > > > download is permanently going away. This different from > > OnDownloadDestroyed() > > > > which means that the specific DownloadItem instance is going away, but the > > > > underlying download is still persistent. > > > > > > > > In response to OnDownloadRemoved() a consumer of the API (say SafeBrowsing > > or > > > > the extensions system) could remove any associated data that they were > > > keeping. > > > > This CL is changing the meaning of OnDownloadRemoved() and the concept of > > > > 'remove' in general. As with the previous patchset, the safe way for a > > > consumer > > > > to respond to OnDownloadRemoved() is to assume that it wasn't removed and > > then > > > > wait for an OnDownloadDestroyed() or OnDownloadRevived(). > > > > > > > > Even then, the cycling of Remove() -> Revive() only works for a download > > that > > > > has completed. Any inprogress download would, currently, be permanently > > > damaged > > > > during the Remove() process. I.e. the underlying request would've been > > > > terminated and the file deleted from disk. Revival wouldn't do what the > user > > > > expects. > > > > > > > > I remain skeptical of this approach of deleting and reviving at a low > level. > > I > > > > still feel that the safe and clean method to implement this is to delay > > > calling > > > > Remove() until the caller is ready to commit to the removal. Undo > shouldn't > > be > > > a > > > > feature exposed at the DownloadItem or DownloadManager layer. At that > level, > > > you > > > > can restore the state of the DownloadItemImpl object, but you can't undo > the > > > > effects of any actions taken by consumers of the downloads system in > > response > > > to > > > > the removal. > > > > > > how do other simultaneous download UIs hide this item? > > > > Not all UIs are going to be ready to undo a remove. > > Sure, but the chrome://downloads page that removes the item should be able to > and all other chrome://downloads pages should update their content. I also > assume the downloads shelf should be hidden or remove this download when "Remove > this item" is clicked on chrome://downloads for a shelf-visible item (just like > it currently does). > > That said, you can introduce > > a 'hide' flag that is either only known to DownloadsDOMHandler or is visible > > from DownloadItemModel. > > It'd have to be known to *all* DownloadsDOMHandlers, right? So in the model > seems like a better place. Tacking on a UserData that DownloadsDOMHandler knows about and firing a UpdateObservers() will work for other chrome://downloads pages. A flag in DownloadItemModel would work across the download shelf implementations and chrome://downloads if you are only interested in hiding. I have a feeling that undoing a removal would be tricky for the download shelf case.
asanka@: ptal -- did you mean something like this?
ping asanka@
On 2014/12/01 08:09:44, Dan Beam wrote: > ping asanka@ ping asanka@ again
LGTM. Sorry about the delay. https://codereview.chromium.org/722953002/diff/240001/chrome/browser/ui/webui... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/722953002/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/downloads_dom_handler.cc:88: static DownloadsDOMHandlerData* Get(const content::DownloadItem* item) { nit: return 'const DownloadsDOMHandlerData*' https://codereview.chromium.org/722953002/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/downloads_dom_handler.cc:643: return NULL; nit: nullptr here and above.
Patchset #6 (id:260001) has been deleted
https://codereview.chromium.org/722953002/diff/240001/chrome/browser/ui/webui... File chrome/browser/ui/webui/downloads_dom_handler.cc (right): https://codereview.chromium.org/722953002/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/downloads_dom_handler.cc:88: static DownloadsDOMHandlerData* Get(const content::DownloadItem* item) { On 2014/12/02 23:59:34, asanka wrote: > nit: return 'const DownloadsDOMHandlerData*' Done. https://codereview.chromium.org/722953002/diff/240001/chrome/browser/ui/webui... chrome/browser/ui/webui/downloads_dom_handler.cc:643: return NULL; On 2014/12/02 23:59:34, asanka wrote: > nit: nullptr here and above. Done.
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/722953002/300001
Message was sent while issue was closed.
Committed patchset #6 (id:300001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/59b0be74d47f3bccceac31f205e515097079a0fa Cr-Commit-Position: refs/heads/master@{#306549} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
