Issue 7056001: views: Split DownloadShelfContextMenuWin out to its own header file.

Issue 7056001: views: Split DownloadShelfContextMenuWin out to its own header file. (Closed)

Created:
9 years, 7 months ago by tfarina

Modified:
9 years, 7 months ago

Reviewers:
asanka, dmazzoni

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

views: Split DownloadShelfContextMenuWin out to its own header file. This moves the implementation of DownloadShelfContextMenuWin from download_item_view.cc to download_shelf_context_menu_view.* Renames DownloadShelfContextMenuWin to DownloadShelfContextMenuView. BUG=28978 TEST=None R=dmazzoni@chromium.org,rdsmith@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86237

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : download_item.h #

Patch Set 5 : include on toolkit_views #

Total comments: 4

Patch Set 6 : asanka review #

Total comments: 1

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+87 lines, -41 lines)			Patch
M	chrome/browser/ui/views/download/download_item_view.h	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/views/download/download_item_view.cc	View	1 2 3 4 5	4 chunks	+2 lines, -39 lines	0 comments	Download
A	chrome/browser/ui/views/download/download_shelf_context_menu_view.h	View	1 2 3 4 5	1 chunk	+40 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/views/download/download_shelf_context_menu_view.cc	View	1 2 3	1 chunk	+39 lines, -0 lines	1 comment	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5	2 chunks	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Randy Smith (Not in Mondays)

Delegating UI download reviews to Asanka (for core download portion; Dominic's still the right person ...

9 years, 7 months ago (2011-05-20 13:45:42 UTC) #2

asanka

Minor nits. LGTM. http://codereview.chromium.org/7056001/diff/4003/chrome/browser/ui/views/download/download_item_view.cc File chrome/browser/ui/views/download/download_item_view.cc (left): http://codereview.chromium.org/7056001/diff/4003/chrome/browser/ui/views/download/download_item_view.cc#oldcode116 chrome/browser/ui/views/download/download_item_view.cc:116: // DownloadItemView ------------------------------------------------------------ Nit: Don't need ...

9 years, 7 months ago (2011-05-20 21:45:16 UTC) #3

tfarina

http://codereview.chromium.org/7056001/diff/4003/chrome/browser/ui/views/download/download_item_view.cc File chrome/browser/ui/views/download/download_item_view.cc (left): http://codereview.chromium.org/7056001/diff/4003/chrome/browser/ui/views/download/download_item_view.cc#oldcode116 chrome/browser/ui/views/download/download_item_view.cc:116: // DownloadItemView ------------------------------------------------------------ On 2011/05/20 21:45:16, asanka wrote: > ...

9 years, 7 months ago (2011-05-20 22:19:23 UTC) #4

dmazzoni

9 years, 7 months ago (2011-05-22 04:39:49 UTC) #5

LGTM.

http://codereview.chromium.org/7056001/diff/6003/chrome/browser/ui/views/down...
File chrome/browser/ui/views/download/download_shelf_context_menu_view.cc
(right):

http://codereview.chromium.org/7056001/diff/6003/chrome/browser/ui/views/down...
chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:23: if
(download_item()->IsComplete())
Just a thought, but I wonder why DownloadShelfContextMenu shouldn't just have a
method GetMenuModel() that grabs the correct model depending on download_item()
- this could be used both here and in download_item_gtk.cc. A quick code search
shows that GetFinishedMenuModel and GetInProgressMenuModel aren't used anywhere
else.

It's pretty trivial either way, but that would put one more line of logic in the
abstract class and one less in the platform-specific implementations.

Expand Messages | Collapse Messages