Issue 172074: Show or open downloaded items on the UI thread for Mac

Issue 172074: Show or open downloaded items on the UI thread for Mac (Closed)

Created:
11 years, 4 months ago by Paul Godavari

Modified:
9 years, 7 months ago

Reviewers:
Mark Mentovai

CC:
chromium-reviews_googlegroups.com, Paul Godavari, Ben Goodger (Google)

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Show or open downloaded items on the UI thread for Mac. This is required because NSWorkspace, which is used for the open or show operation, must be called on the main thread. BUG=19447 (http://crbug.com/19447) TEST=Repro steps fully described in the bug report. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23667

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Total comments: 5

Patch Set 6 : '' #

Patch Set 7 : '' #

Patch Set 8 : '' #

Patch Set 9 : '' #

Created: 11 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+64 lines, -30 lines)			Patch
M	chrome/browser/download/download_file.h	View	6 7	1 chunk	+6 lines, -0 lines	0 comments	Download
M	chrome/browser/download/download_file.cc	View	1 2 3 4 5 6 7	2 chunks	+4 lines, -1 line	0 comments	Download
M	chrome/browser/download/download_manager.cc	View	1 2 3 4 5 6 7	2 chunks	+12 lines, -0 lines	0 comments	Download
M	chrome/browser/download/save_file_manager.h	View	3 4 5 6 7	1 chunk	+8 lines, -3 lines	0 comments	Download
M	chrome/browser/download/save_file_manager.cc	View	2 3 4 5 6 7	13 chunks	+16 lines, -14 lines	0 comments	Download
M	chrome/browser/download/save_package.cc	View	1 2 3 4 5 6 7	9 chunks	+15 lines, -9 lines	0 comments	Download
M	chrome/browser/renderer_host/save_file_resource_handler.cc	View	3 4 5 6 7	3 chunks	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 2 (0 generated)

Expand Messages | Collapse Messages

Mark Mentovai

11 years, 4 months ago (2009-08-18 02:58:15 UTC) #2

Excellent.  LGTM with these changes.

http://codereview.chromium.org/172074/diff/1027/33
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/172074/diff/1027/33#newcode526
Line 526: // Open a download, or show it in a file explorer window. We run on
this
You've killed the only user of this function on Mac OS X.  You should just wrap
the entire function in #if !defined(OS_MACOSX) (here and in download_file.h)
instead of doing the NOTREACHED thing.  That way, if someone tries to use it on
the Mac, it'll be an attention-grabbing linker error, and not an easier-to-miss
runtime warning.

http://codereview.chromium.org/172074/diff/1027/33#newcode539
Line 539: // Launches the selected download using ShellExecute 'open' verb. For
windows,
Same comment applies to this function.

http://codereview.chromium.org/172074/diff/1027/35
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/172074/diff/1027/35#newcode1282
Line 1282: // Mac required opening downloads on the UI thread.
required -> requires

http://codereview.chromium.org/172074/diff/1027/36
File chrome/browser/download/save_file_manager.cc (right):

http://codereview.chromium.org/172074/diff/1027/36#newcode512
Line 512: // Open a saved page package, show it in a Windows Explorer window.
#ifdef comment applies here as well.

http://codereview.chromium.org/172074/diff/1027/34
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/172074/diff/1027/34#newcode740
Line 740: platform_util::ShowItemInFolder(saved_main_file_path_);
Put a comment here about the Mac being different as you did in
download_manager.cc.

Expand Messages | Collapse Messages