No need to pass DownloadItemModel ownership.

Don't pass DownloadItemModel ownership.

DownloadItemModel is a thin wrapper around DownloadItem*. It's
easier to instantiate it from a DownloadItem* than try to pass
ownership around.

Doing so was required when BaseDownloadItemModel existed, and the
actual model could be either a DownloadItemModel or a
SavePageModel. However this is no longer the case.

In addition, DownloadShelfContextMenu should track the lifetime
of the corresponding DownloadItem. It is possible for the
DownloadItem to be destroyed while the context menu is showing.

BUG=116551
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175558

[asanka, 2012-12-27 20:32:27]

sky: This stems partly from https://codereview.chromium.org/11419169/#msg4 .
Ownership of DownloadItemModel doesn't need to be passed around anymore. So
instead of passing a scoped_ptr, I'm passing a DownloadItem* around and
instantiating a DownloadItemModel when necessary.


Could you look at chrome/browser/ui ?

rdsmith: Could you look at chrome/browser/download ?

[Randy Smith (Not in Mondays), 2012-12-28 19:02:46]

chrome/browser/download LGTM.

[Not for this CL]  I find myself wondering if there's a better base abstraction
for the DownloadItem interface than a basically empty DownloadItemModel class. 
Might it make sense to combine DownloadItemModel and DownloadItemModelData and
just make the whole thing hang off the DownloadItem?  With a static getter
taking the DI & etc?  Just a thought.

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/download/do...
File chrome/browser/download/download_shelf_context_menu.h (right):

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.h:62: virtual void
OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
Why protected rather than private?

[sky, 2013-01-07 18:28:24]

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc
(right):

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:29:
DCHECK(menu_model);
How come this is a DCHECK, but in the corresponding gtk class it isn't?

[asanka, 2013-01-07 20:19:25]

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/download/do...
File chrome/browser/download/download_shelf_context_menu.h (right):

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/download/do...
chrome/browser/download/download_shelf_context_menu.h:62: virtual void
OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
On 2012/12/28 19:02:46, rdsmith wrote:
> Why protected rather than private?

Moved to private.

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc
(right):

https://codereview.chromium.org/11673004/diff/4001/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:29:
DCHECK(menu_model);
On 2013/01/07 18:28:24, sky wrote:
> How come this is a DCHECK, but in the corresponding gtk class it isn't?

Oops. I updated download_shelf_context_menu_gtk.cc.

[sky, 2013-01-07 22:02:51]

LGTM

[asanka, 2013-01-07 22:09:15]

On 2013/01/07 22:02:51, sky wrote:
> LGTM

Thanks!

[commit-bot: I haz the power, 2013-01-07 23:19:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/11673004/10001

[commit-bot: I haz the power, 2013-01-08 15:11:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/11673004/10001

[commit-bot: I haz the power, 2013-01-08 18:27:32]

Change committed as 175558