downloads: clicking "remove" on chrome://downloads should also hide shelf item.

downloads: clicking "remove" on chrome://downloads should hide the shelf item.

R=asanka@chromium.org
BUG=442666
Committed: https://crrev.com/2faad5a1a60b12829037c64ce5786b8017b602db
Cr-Commit-Position: refs/heads/master@{#318788}

[asanka, 2015-02-28 00:58:52]

If the removal state is common across UIs, shall we move it into
DownloadItemModel?

Then removal would look like:

   - Mark the download as removed using the DownloadItemModel
   - Fire DownloadItem::UpdateObservers().

DownloadItem{View,Mac} and DownloadDOMHandler could then check for the removed
flag in OnDownloadUpdated().

https://codereview.chromium.org/966983002/diff/1/chrome/browser/ui/webui/down...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/966983002/diff/1/chrome/browser/ui/webui/down...
chrome/browser/ui/webui/downloads_dom_handler.cc:555: shelf =
browser->window()->GetDownloadShelf();
This might not work since the shelf could be on a different browser window.

[Dan Beam, 2015-02-28 01:05:51]

On 2015/02/28 00:58:52, asanka wrote:
> If the removal state is common across UIs, shall we move it into
> DownloadItemModel?
> 
> Then removal would look like:
> 
>    - Mark the download as removed using the DownloadItemModel
>    - Fire DownloadItem::UpdateObservers().
> 
> DownloadItem{View,Mac} and DownloadDOMHandler could then check for the removed
> flag in OnDownloadUpdated().

you mean like I proposed here? https://codereview.chromium.org/722953002/#msg34

[Dan Beam, 2015-02-28 04:46:04]

check out updated code

https://codereview.chromium.org/966983002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (left):

https://codereview.chromium.org/966983002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:37: scoped_ptr<Delegate>
delegate);
I can do this cleanup separately, but I couldn't find anywhere that |delegate|
was ever used...

[Dan Beam, 2015-02-28 07:10:27]

https://codereview.chromium.org/966983002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (left):

https://codereview.chromium.org/966983002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:37: scoped_ptr<Delegate>
delegate);
On 2015/02/28 04:46:04, Dan Beam wrote:
> I can do this cleanup separately, but I couldn't find anywhere that |delegate|
> was ever used...

nvm, used in tests. stupid codesearch.

[Dan Beam, 2015-03-02 18:46:11]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2015-03-02 18:46:16]

Patchset #3 (id:60001) has been deleted

[Dan Beam, 2015-03-02 18:50:10]

i'll add thakis@ for OWNERS when you're satisfied

https://codereview.chromium.org/966983002/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.h (left):

https://codereview.chromium.org/966983002/diff/40001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.h:37: scoped_ptr<Delegate>
delegate);
On 2015/02/28 07:10:27, Dan Beam wrote:
> On 2015/02/28 04:46:04, Dan Beam wrote:
> > I can do this cleanup separately, but I couldn't find anywhere that
|delegate|
> > was ever used...
> 
> nvm, used in tests. stupid codesearch.

restored, ptal

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_ui_controller.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/download/...
chrome/browser/download/download_ui_controller.cc:20: #include
"chrome/browser/ui/host_desktop.h"
iwyu on the platform that uses them

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1342:
window()->GetDownloadShelf()->AddDownload(download);
shaves a line, didn't really see the point to creating a temporary pointer

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:287: return;
can move this earlier if this tickles the "return inside if/else" rule

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_shelf_view.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_shelf_view.cc:130: AddDownloadView(new
DownloadItemView(download, this));
same story: -1 line

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/downloads_dom_handler.cc:248:
const_cast<content::DownloadItem*>(&item)).ShouldShowInShelf();
open to suggestions if you don't like this

[asanka, 2015-03-02 21:49:34]

lgtm. Thanks for the cleanup!

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/download/download_item_mac.mm (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_mac.mm:45: if
(!download_model_.ShouldShowInShelf()) {
Nit: shall we move this to the top? If this condition is true, then the work
done above this would also not be necessary.

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:287: return;
On 2015/03/02 at 18:50:09, Dan Beam wrote:
> can move this earlier if this tickles the "return inside if/else" rule

Yeah. For the reasons I mentioned in .mm above. Let's exit as early as possible
if the download was removed.

[Dan Beam, 2015-03-02 22:04:08]

added a little more testing

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/download/download_item_mac.mm (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/download/download_item_mac.mm:45: if
(!download_model_.ShouldShowInShelf()) {
On 2015/03/02 21:49:33, asanka wrote:
> Nit: shall we move this to the top? If this condition is true, then the work
> done above this would also not be necessary.

Done.

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/966983002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/download/download_item_view.cc:287: return;
On 2015/03/02 21:49:33, asanka wrote:
> On 2015/03/02 at 18:50:09, Dan Beam wrote:
> > can move this earlier if this tickles the "return inside if/else" rule
> 
> Yeah. For the reasons I mentioned in .mm above. Let's exit as early as
possible
> if the download was removed.

Done.

[asanka, 2015-03-02 22:07:21]

still lgtm!

[Dan Beam, 2015-03-02 22:10:30]

dbeam@chromium.org changed reviewers:
+ thakis@chromium.org

[Dan Beam, 2015-03-02 22:10:30]

+thakis@ for OWNERS (saw you in c/b/ui/cocoa/download so you're the lucky
winner!)

[Nico, 2015-03-02 22:12:14]

rs-lgtm

asanka, you should make yourself an owner of the mac download code if you aren't
already.

[Dan Beam, 2015-03-02 22:14:49]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-03-02 22:16:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/966983002/100001

[commit-bot: I haz the power, 2015-03-02 22:49:39]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-03-02 22:51:03]

Patchset 4 (id:??) landed as
https://crrev.com/2faad5a1a60b12829037c64ce5786b8017b602db
Cr-Commit-Position: refs/heads/master@{#318788}