Rearrange Should*Download delegate calls to be more useful and intuitive.

Rearrange Should*Download delegate calls to be more useful and intuitive.

This involves killing the code in ShouldOpenDownload (vestigal), renaming
ShouldCompleteDownload to ShouldOpenDownload, and creating a new
ShouldCompleteDownload delegate method to allow blocking download completion 
before final rename.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108141

[Randy Smith (Not in Mondays), 2011-10-28 18:05:05]

Asanka: Could you review the download portions of this?  (*/browser/download/*).
 
John: Could you review everything else?  It should be close to a rubber stamp.
Noe': You're cc'd FYI, but if you have any concerns you can certainly block
review (I couldn't figure out if you had a chromium.org account).

[Randy Smith (Not in Mondays), 2011-10-28 18:06:13]

Whoops, one more reviewer: Mihai, could you confirm that the removal of the
previously existent ShouldOpenDownload functionality won't cause any grief?  I'm
also ccing Sam in case he wants to weigh in.

[jam, 2011-10-28 18:28:29]

rubberstamp lgtm, you know this code much better than me :)

[asanka, 2011-10-28 19:22:19]

*/browser/download/* LGTM

http://codereview.chromium.org/8414007/diff/12/content/browser/download/downl...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/8414007/diff/12/content/browser/download/downl...
content/browser/download/download_manager.cc:498: <<
download->DebugString(false);
Very minor nit: Should this logging happen after calling
ShouldCompleteDownload()?

[Mihai Parparita -not on Chrome, 2011-10-28 19:23:47]

Sam is a much more qualified reviewer than I am (I haven't looked at how
extensions use the download system at all).

[Sam Kerner (Chrome), 2011-10-28 19:32:47]

LGTM

[Randy Smith (Not in Mondays), 2011-10-29 23:10:39]

Thanks, all!

http://codereview.chromium.org/8414007/diff/12/content/browser/download/downl...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/8414007/diff/12/content/browser/download/downl...
content/browser/download/download_manager.cc:498: <<
download->DebugString(false);
On 2011/10/28 19:22:19, asanka wrote:
> Very minor nit: Should this logging happen after calling
> ShouldCompleteDownload()?

Yeah, I think so.  Done.

[commit-bot: I haz the power, 2011-10-31 15:58:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8414007/4002

[commit-bot: I haz the power, 2011-10-31 15:58:10]

Can't apply patch for file
chrome/browser/download/chrome_download_manager_delegate.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/download/chrome_download_manager_delegate.cc
Hunk #1 FAILED at 147.
Hunk #2 succeeded at 179 (offset 16 lines).
Hunk #3 succeeded at 344 (offset 22 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/download/chrome_download_manager_delegate.cc.rej

[commit-bot: I haz the power, 2011-11-01 17:32:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8414007/9001

[commit-bot: I haz the power, 2011-11-01 18:50:25]

Change committed as 108141