Modified cancel and interrupt processing to avoid race with history.

Modified cancel and interrupt processing to avoid race with history.

Avoid racing with the history callback by unilaterally removing
DownloadItem from queues on cancel/interrupt.  This keeps the
state<->queue correspondence cleaner, and avoids leaving things on 
queues during shutdown.  It might also fix 85408; we'll see :-}.

BUG=85408
TEST=

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

[Randy Smith (Not in Mondays), 2011-06-30 23:05:13]

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1053: // Put up a Select File
dialog when the file is downloaded, due to its MIME
These comments about select file dialog appearing due to MIME type are incorrect
and should be fixed throughout this file.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1073:
active_downloads[0]->Remove();
Comment: Confirm the file can be successfully removed from the select file
dialog blocked state.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1084: // Check state.
Remove comment.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1103:
browser()->profile()->GetDownloadManager()->GetInProgressDownloads(
Consider making a download manager accessor.  A bit tricky because the browser()
we actually use isn't always browser().

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1116: active_downloads.clear();
Comment: Confirm that a download error before entry into history deletes the
download.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1585:
IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerous) {
Comment explaining test.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1609: // See if accepting it
does the right thing.
Specify what the right thing is.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1626:
IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousFileError) {
Comment describing test.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1670:
IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousDecline) {
Comment explaining test.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1694: // See if declining it
does the right thing.
Specify what the right thing is.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1708: // correct things happen.
More specific about what the correct things are.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_history.h:57: void RemoveEntry(int64
db_handle);
Go over DownloadHistory interface; how "strange" is the use of db_handle?  Any
way to avoid that?  Maybe switch the whole thing over to db_handle?  Explore
options.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.cc:351:
download_util::RecordDownloadCount(download_util::CANCELLED_COUNT);
This recording is incorrect--it should be only in the callers.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.cc:391: // Note that a cancel that occurs
before a downoad item is persisted
"download"

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.cc:568: void DownloadItem::Remove() {
We need a new UMA count here for removals (or maybe to add to the cancel count
if we actually cancel it).

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.h:168: // it will remove the download
completely from the system.
Change comment to indicate both behaviors (before and after persistence) and
phrase the difference as "before all download attributes have been finalized").

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.h:184: // it will be completely removed.
Make this comment align with the Cancel comment.

Long term, we may want to go through all the hoops for a download before we
accept a network error, so that interrupted downloads can always be resumed. 
I'm not sure what this means for dealing with file errors while trying to
determine the file name.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:879: // Silently ignores request if
db handled indicates that the download
"handled" -> "handle"

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:1170: // history system (implicitly
treating is as a failure in download
"is" -> "it"

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:1173: // we need to have resolved
for history system insertion.
Not quite true--could have come from FileSelectionCancelled.  Update comment for
accuracy?

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:1178: // when it's next read in.
"on next browser startup".

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.h:131: // from all internal queues, and
takes care of the off-thread aspects
Not all internal queues, just the ones representing activity.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.h:136: // Called from DownloadItem when
the download is being removed.
A bit more detail as to what it does.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager_unittest.cc:519:
EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
Update implementation of this function to use GetActiveDownloads?

http://codereview.chromium.org/7294013/diff/1/content/browser/net/url_request...
File content/browser/net/url_request_slow_download_job.h (right):

http://codereview.chromium.org/7294013/diff/1/content/browser/net/url_request...
content/browser/net/url_request_slow_download_job.h:39: static const char
kErrorFinishDownloadUrl[];
Document the functionality somewhere.

[Randy Smith (Not in Mondays), 2011-07-05 20:28:43]

Incorporated comments from self-review.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1053: // Put up a Select File
dialog when the file is downloaded, due to its MIME
On 2011/06/30 23:05:13, rdsmith wrote:
> These comments about select file dialog appearing due to MIME type are
incorrect
> and should be fixed throughout this file.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1073:
active_downloads[0]->Remove();
On 2011/06/30 23:05:13, rdsmith wrote:
> Comment: Confirm the file can be successfully removed from the select file
> dialog blocked state.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1084: // Check state.
On 2011/06/30 23:05:13, rdsmith wrote:
> Remove comment.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1103:
browser()->profile()->GetDownloadManager()->GetInProgressDownloads(
On 2011/06/30 23:05:13, rdsmith wrote:
> Consider making a download manager accessor.  A bit tricky because the
browser()
> we actually use isn't always browser().

Created new and used existing accessors for what I'm actually using the download
manager for.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1116: active_downloads.clear();
On 2011/06/30 23:05:13, rdsmith wrote:
> Comment: Confirm that a download error before entry into history deletes the
> download.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1585:
IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerous) {
On 2011/06/30 23:05:13, rdsmith wrote:
> Comment explaining test.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1609: // See if accepting it
does the right thing.
On 2011/06/30 23:05:13, rdsmith wrote:
> Specify what the right thing is.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1626:
IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousFileError) {
On 2011/06/30 23:05:13, rdsmith wrote:
> Comment describing test.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1670:
IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousDecline) {
On 2011/06/30 23:05:13, rdsmith wrote:
> Comment explaining test.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1694: // See if declining it
does the right thing.
On 2011/06/30 23:05:13, rdsmith wrote:
> Specify what the right thing is.

Just deleted comment; initial comment has all needed info.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_browsertest.cc:1708: // correct things happen.
On 2011/06/30 23:05:13, rdsmith wrote:
> More specific about what the correct things are.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_history.h:57: void RemoveEntry(int64
db_handle);
On 2011/06/30 23:05:13, rdsmith wrote:
> Go over DownloadHistory interface; how "strange" is the use of db_handle?  Any
> way to avoid that?  Maybe switch the whole thing over to db_handle?  Explore
> options.

Hmmm.  AddEntry and UpdateEntry sorta need the download item, because that's
what the entry is created based on (though we could hoist DownloadHistoryInfo up
a level if we wanted).  UpdateDownloadPath could get transferred over.  I think
I'll leave it like it is (a bit of a mess) until we've updated the ID behavior.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.cc:351:
download_util::RecordDownloadCount(download_util::CANCELLED_COUNT);
On 2011/06/30 23:05:13, rdsmith wrote:
> This recording is incorrect--it should be only in the callers.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.cc:391: // Note that a cancel that occurs
before a downoad item is persisted
On 2011/06/30 23:05:13, rdsmith wrote:
> "download"

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.cc:568: void DownloadItem::Remove() {
On 2011/06/30 23:05:13, rdsmith wrote:
> We need a new UMA count here for removals (or maybe to add to the cancel count
> if we actually cancel it).

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.h:168: // it will remove the download
completely from the system.
On 2011/06/30 23:05:13, rdsmith wrote:
> Change comment to indicate both behaviors (before and after persistence) and
> phrase the difference as "before all download attributes have been
finalized").
> 

Changed phrasing, left assymetric specification.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_item.h:184: // it will be completely removed.
On 2011/06/30 23:05:13, rdsmith wrote:
> Make this comment align with the Cancel comment.
> 
> Long term, we may want to go through all the hoops for a download before we
> accept a network error, so that interrupted downloads can always be resumed. 
> I'm not sure what this means for dealing with file errors while trying to
> determine the file name.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:879: // Silently ignores request if
db handled indicates that the download
On 2011/06/30 23:05:13, rdsmith wrote:
> "handled" -> "handle"

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:1170: // history system (implicitly
treating is as a failure in download
On 2011/06/30 23:05:13, rdsmith wrote:
> "is" -> "it"

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:1173: // we need to have resolved
for history system insertion.
On 2011/06/30 23:05:13, rdsmith wrote:
> Not quite true--could have come from FileSelectionCancelled.  Update comment
for
> accuracy?

FileSelectionCancelled is part of resolving the file name.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.cc:1178: // when it's next read in.
On 2011/06/30 23:05:13, rdsmith wrote:
> "on next browser startup".

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.h:131: // from all internal queues, and
takes care of the off-thread aspects
On 2011/06/30 23:05:13, rdsmith wrote:
> Not all internal queues, just the ones representing activity.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager.h:136: // Called from DownloadItem when
the download is being removed.
On 2011/06/30 23:05:13, rdsmith wrote:
> A bit more detail as to what it does.

Done.

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/7294013/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_manager_unittest.cc:519:
EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
On 2011/06/30 23:05:13, rdsmith wrote:
> Update implementation of this function to use GetActiveDownloads?

Nah; it takes an argument, which would be hard to manage using
GetActiveDownloads.

http://codereview.chromium.org/7294013/diff/1/content/browser/net/url_request...
File content/browser/net/url_request_slow_download_job.h (right):

http://codereview.chromium.org/7294013/diff/1/content/browser/net/url_request...
content/browser/net/url_request_slow_download_job.h:39: static const char
kErrorFinishDownloadUrl[];
On 2011/06/30 23:05:13, rdsmith wrote:
> Document the functionality somewhere.

Done.

[Randy Smith (Not in Mondays), 2011-07-05 21:48:10]

PTAL:
* Andy: Primary reviewer (Pawel's off, so go through it carefully).
* Ben: For backup for Andy, and because you're doing queue modification work as
well.
* Pawel: When you get a chance, I won't hold commit since you're off.
* Will: url_request_slow_download_job.*
* Brett: Rubber-stamp for content/browser (Will's actually reviewer for this,
it's all in url_request_slow_download_job.*)

Thanks!

[willchan no longer on Chromium, 2011-07-06 06:20:46]

I'm a bit overloaded right now, replacing myself with eroman in hopes he can get
to this sooner.

[brettw, 2011-07-06 16:44:13]

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:34: // Return whether this
is the finish or error URL
Nit: Need period.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:37:
URLRequestSlowDownloadJob::kFinishDownloadUrl, url.spec().c_str()) ||
This statement is almost impossible to read. I'd probably split the || apart
into two statements and do an early return if the first one passes.

[benjhayden, 2011-07-06 18:12:06]

2 nits but LGTM.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_history.cc:125: if (db_handle <=
kUninitializedHandle)
Do you want to start using IsFakeHandle(db_handle) everywhere?
My browser pref id work will obviate GetNextFakeDbHandle, so it's up to you.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_history.cc:146: return db_handle <= 0;
db_handle <= kUninitializedHandle;

[eroman, 2011-07-06 22:19:01]

lgtm for content/browser/net/*

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:38: LowerCaseEqualsASCII(
Why use lowercase URL comparison throughout rather than straight up string
comparison?

Note that since url.spec() is already canonicalized, the scheme and host are
necessarily going to be lowercased. (The case of the path will be preserved
though).

Unless the test usage has good reason to pass different cased paths, I suggest
not messing with it and simplify this to "==" comparison.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:77: kPendingRequests.end();
++it) {
The following comment predates this change.... but why is this variable named
like a constant? This is really just a global variable.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:96: if
(LowerCaseEqualsASCII(kFinishDownloadUrl, request_->url().spec().c_str()))
Same comment as before: I think we should use exact string comparison throughout
here.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:99: if
(LowerCaseEqualsASCII(kErrorFinishDownloadUrl,
ditto

[Randy Smith (Not in Mondays), 2011-07-07 22:02:23]

Next round; PTAL.

Added Achuith for perspective on the correct arguments to CheckDownloadUI() for
the new download browser tests.  Achuith: The particular tests I'm currently
pinging off the try bots are DownloadDangerous, DownloadDangerousDecline,
DownloadDangerousFileError, and DownloadInterrupted.  I'm concerned because I'm
convincing myself that passing "FilePath()" as the last argument of
CheckDownloadUI() is the right thin g in a couple of cases, but that disables
the filename check.  Could you scan the new CheckDownloadUI() calls in general
and that pattern in specific for problems?  Thanks much.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_history.cc:125: if (db_handle <=
kUninitializedHandle)
On 2011/07/06 18:12:06, benjhayden wrote:
> Do you want to start using IsFakeHandle(db_handle) everywhere?
> My browser pref id work will obviate GetNextFakeDbHandle, so it's up to you.

I think of IsFakeHandle as an interface to DownloadHistory, so I'm not worried
about using it inside DownloadHistory.  It's so the code outside of
DownloadHistory don't have to know about how fake DB handles are created.

Having said that, it appears I've created this without using it (probably cruft
from an intermediate commit) so I'm going to remove it.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_history.cc:146: return db_handle <= 0;
On 2011/07/06 18:12:06, benjhayden wrote:
> db_handle <= kUninitializedHandle;

Done.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:34: // Return whether this
is the finish or error URL
On 2011/07/06 16:44:13, brettw wrote:
> Nit: Need period.

Done.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:37:
URLRequestSlowDownloadJob::kFinishDownloadUrl, url.spec().c_str()) ||
On 2011/07/06 16:44:13, brettw wrote:
> This statement is almost impossible to read. I'd probably split the || apart
> into two statements and do an early return if the first one passes.

Done (though it's easier to read with the change requested by Eric).

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:38: LowerCaseEqualsASCII(
On 2011/07/06 22:19:01, eroman wrote:
> Why use lowercase URL comparison throughout rather than straight up string
> comparison?
> 
> Note that since url.spec() is already canonicalized, the scheme and host are
> necessarily going to be lowercased. (The case of the path will be preserved
> though).
> 
> Unless the test usage has good reason to pass different cased paths, I suggest
> not messing with it and simplify this to "==" comparison.

There's no good reason except for compatibility with what was already there;
I'll change it.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:77: kPendingRequests.end();
++it) {
On 2011/07/06 22:19:01, eroman wrote:
> The following comment predates this change.... but why is this variable named
> like a constant? This is really just a global variable.

Done.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:96: if
(LowerCaseEqualsASCII(kFinishDownloadUrl, request_->url().spec().c_str()))
On 2011/07/06 22:19:01, eroman wrote:
> Same comment as before: I think we should use exact string comparison
throughout
> here.

Done.

http://codereview.chromium.org/7294013/diff/9002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:99: if
(LowerCaseEqualsASCII(kErrorFinishDownloadUrl,
On 2011/07/06 22:19:01, eroman wrote:
> ditto

Done.

[achuithb, 2011-07-08 19:21:50]

On 2011/07/07 22:02:23, rdsmith wrote:
> Next round; PTAL.
> 
> Added Achuith for perspective on the correct arguments to CheckDownloadUI()
for
> the new download browser tests.  Achuith: The particular tests I'm currently
> pinging off the try bots are DownloadDangerous, DownloadDangerousDecline,
> DownloadDangerousFileError, and DownloadInterrupted.  I'm concerned because
I'm
> convincing myself that passing "FilePath()" as the last argument of
> CheckDownloadUI() is the right thin g in a couple of cases, but that disables
> the filename check.  Could you scan the new CheckDownloadUI() calls in general
> and that pattern in specific for problems?  Thanks much.

LGTM on the CheckDownloadUI changes. It looks like all the FilePath() cases are
legitimate instances of when the file name is unknown/non-existant.

[brettw, 2011-07-08 19:48:22]

content LGTM (didn't check the details).

http://codereview.chromium.org/7294013/diff/16001/content/browser/net/url_req...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7294013/diff/16001/content/browser/net/url_req...
content/browser/net/url_request_slow_download_job.cc:35: static bool
IsCompletionUrl(GURL url) {
This should probably take a const ref to avoid a copy.

[ahendrickson, 2011-07-11 20:05:48]

LGTM, with minor issues.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:346:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Could you add a DCHECK that target_state is either CANCELLED or INTERRUPTED?

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:405: // History insertion is the point
at which we have finalized download
Why not put this in StopInternal()?

For a call from Remove(), you could set db_handle_ to
DownloadHistory::kUninitializedHandle first.  Or, if you still needed to call
AssertQueueStateConsistent() afterwards, add a flag to prevent it from removing
the download.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:411: return;
Nit: Redundant.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:539: return;
Nit: Redundant.

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:517: HistoryService* hs =
DCHECK(manager)

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:545: result_valid_ = true;
Add a break statement after this line?

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:593: // opening for all
downloads created on the specified download manager.
This can't be used twice in a row, right?  For the first one it would work, but
for the second you would get the first download as well as the second as being
opened?

Not that this is necessarily a problem, but it would be nice to have a comment
if that is the case.

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:879: // Silently ignores request if
db handle indicates that the download
Add

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

now that it's called from another file.

[Randy Smith (Not in Mondays), 2011-07-13 21:11:38]

I've incoporated all comments and gotten all LGTMs, so everyone's done unless I
have to do a major merge.  But it isn't going in right away, as I'm having
extended struggles getting the try jobs to complete (what I believe are real
problems, just not reproducible locally :-{).

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:517: HistoryService* hs =
On 2011/07/11 20:05:48, ahendrickson wrote:
> DCHECK(manager)

Done.

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:545: result_valid_ = true;
On 2011/07/11 20:05:48, ahendrickson wrote:
> Add a break statement after this line?

Done.

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:593: // opening for all
downloads created on the specified download manager.
On 2011/07/11 20:05:48, ahendrickson wrote:
> This can't be used twice in a row, right?  For the first one it would work,
but
> for the second you would get the first download as well as the second as being
> opened?
> 
> Not that this is necessarily a problem, but it would be nice to have a comment
> if that is the case.

Not quite--if there are *any* instances of this class *anywhere* in the system,
all new downloads added to the specified download manager will have opening
disabled.  So it stacks fine (in the sense that getting hit with a sledgehammer
twice isn't all that different than getting hit with it once).  I think the key
point is that this makes a permanent state change in DownloadItems added to the
download manager while it's active.  I'll try to express that better in the
comment.

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7294013/diff/16001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:879: // Silently ignores request if
db handle indicates that the download
On 2011/07/11 20:05:48, ahendrickson wrote:
> Add
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> 
> now that it's called from another file.

Done.

http://codereview.chromium.org/7294013/diff/16001/content/browser/net/url_req...
File content/browser/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/7294013/diff/16001/content/browser/net/url_req...
content/browser/net/url_request_slow_download_job.cc:35: static bool
IsCompletionUrl(GURL url) {
On 2011/07/08 19:48:22, brettw wrote:
> This should probably take a const ref to avoid a copy.

Done.

[Randy Smith (Not in Mondays), 2011-07-14 19:03:23]

I'd like to request one more round of review, primarily from Andy (downloads
expert) and Brett (I want an opinion on the Chromishness of my solution to the
problem).  Achuith, there have been a couple of mods to CheckDownloadUI calls,
but I think they're trivial--you can review or not as you wish.

Apologies for the large number of changes shown; I switched trunk backing
versions, and I used rebase, so I can't even go back and put it in two different
patch sets :-{.  The real changes are all in download_browsertest.cc and 
download_manager.*.  I added an accessor, for testing only, on download manager
to allow specification of a different SelectFileDialog than the default, and
created a mock class to null out the dialog.  Andy, I'd like your opinion on
that entire change; Brett, I'm mostly interested in whether you think this type
of mocking accessor on a central class is ok by Chrome standard patterns.  PTAL.

[achuithb, 2011-07-14 22:00:43]

I didn't look at the CheckDownloadUI calls in detail, but they look ok. I don't
understand the DownloadInterrupted failure on the linux_chromeos trybot. Let me
know if you need me to take a closer look.

[Randy Smith (Not in Mondays), 2011-07-15 19:59:20]

On 2011/07/14 22:00:43, achuith.bhandarkar wrote:
> I didn't look at the CheckDownloadUI calls in detail, but they look ok. I
don't
> understand the DownloadInterrupted failure on the linux_chromeos trybot. Let
me
> know if you need me to take a closer look.

On ChromeOS, should I expect a download to be shown in the download panel if the
download hasn't yet completed?  Should I expect it to have the correct name, or
will .crdownload be appended?  My current guess is that I should see the file
with .crdownload appended, and that's why I'm getting my current failure.  Does
that make sense to you?

[ahendrickson, 2011-07-15 20:09:57]

Still LGTM.

[Randy Smith (Not in Mondays), 2011-07-15 20:49:08]

Incorporated missed comments from Andy.  Waiting for LGTM from Brett.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:346:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/07/11 20:05:48, ahendrickson wrote:
> Could you add a DCHECK that target_state is either CANCELLED or INTERRUPTED?

Sorry, I don't know how I missed these.  Done.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:405: // History insertion is the point
at which we have finalized download
On 2011/07/11 20:05:48, ahendrickson wrote:
> Why not put this in StopInternal()?
> 
> For a call from Remove(), you could set db_handle_ to
> DownloadHistory::kUninitializedHandle first.  Or, if you still needed to call
> AssertQueueStateConsistent() afterwards, add a flag to prevent it from
removing
> the download.

I don't feel comfortable doing this, both because I'm uncomfortable playing
games with the db_handle to signal to a function that it shouldn't do part of
what it would normally do (primary) and because the "delete self but continue on
in self's methods" pattern is a dangerous one, and I'd rather not push the
delete even further down the call stack *and* make it conditional so that
callers have more of a chance to keep using a deleted items members.  So unless
you feel strongly about this, I'll pass.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:411: return;
On 2011/07/11 20:05:48, ahendrickson wrote:
> Nit: Redundant.

I see why I did it (a more neurotic version of the above, worrying about code
being inserted after the curly brace in the future that would then access the
deleted items member variables), but you're quite right.  Replaced with a
comment.

http://codereview.chromium.org/7294013/diff/9002/chrome/browser/download/down...
chrome/browser/download/download_item.cc:539: return;
On 2011/07/11 20:05:48, ahendrickson wrote:
> Nit: Redundant.

Done.

[achuith, 2011-07-15 21:08:59]

The download will be shown in the download panel during the download and it
has the correct name (not .crdownload). Let me know if you want me to run
something specific on my setup.


> On ChromeOS, should I expect a download to be shown in the download panel
> if the
> download hasn't yet completed?  Should I expect it to have the correct
> name, or
> will .crdownload be appended?  My current guess is that I should see the
> file
> with .crdownload appended, and that's why I'm getting my current failure.
>  Does
> that make sense to you?
>
>
>
>
http://codereview.chromium.**org/7294013/<http://codereview.chromium.org/7294...
>

[brettw, 2011-07-15 22:36:41]

LGTM rubberstamp

[Randy Smith (Not in Mondays), 2011-07-17 21:03:24]

On 2011/07/15 21:08:59, achuith wrote:
> The download will be shown in the download panel during the download and it
> has the correct name (not .crdownload). Let me know if you want me to run
> something specific on my setup.

Achuith: Could you take a look at the latest patch set and the results of the
browser_tests on the following try bot run:  

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

"The dog did nothing in the night; that was the strange thing."  The tests pass,
but I'm passing in the .crdownload version of the file.  Does this result make
sense to you?  If so, I'll take out the log statements and leave in the
.crdownload transformation.  It basically makes sense to me since the comparison
in CheckDownlodUI is against full_path(), which will be a .crdownload file name
until the download completes.

[Randy Smith (Not in Mondays), 2011-07-17 21:04:21]

On 2011/07/17 21:03:24, rdsmith wrote:
> On 2011/07/15 21:08:59, achuith wrote:
> > The download will be shown in the download panel during the download and it
> > has the correct name (not .crdownload). Let me know if you want me to run
> > something specific on my setup.
> 
> Achuith: Could you take a look at the latest patch set and the results of the
> browser_tests on the following try bot run:  
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
> 
> "The dog did nothing in the night; that was the strange thing."  The tests
pass,
> but I'm passing in the .crdownload version of the file.  Does this result make
> sense to you?  If so, I'll take out the log statements and leave in the
> .crdownload transformation.  It basically makes sense to me since the
comparison
> in CheckDownlodUI is against full_path(), which will be a .crdownload file
name
> until the download completes.

Note that the patch set matching this try bot run is PS8 "Hack to fix chromeos
run."  But I'll try not to upload another patchset until I hear from you.

[achuithb, 2011-07-17 23:17:36]

> > Achuith: Could you take a look at the latest patch set and the results of
the
> > browser_tests on the following try bot run:  
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
> > 
> > "The dog did nothing in the night; that was the strange thing."  The tests
> pass,
> > but I'm passing in the .crdownload version of the file.  Does this result
make
> > sense to you?  If so, I'll take out the log statements and leave in the
> > .crdownload transformation.  It basically makes sense to me since the
> comparison
> > in CheckDownlodUI is against full_path(), which will be a .crdownload file
> name
> > until the download completes.
> 
> Note that the patch set matching this try bot run is PS8 "Hack to fix chromeos
> run."  But I'll try not to upload another patchset until I hear from you.

Now that I think about it, it does make sense. There was an earlier version that
stripped the .crdownload from the filename, but that was done in javascript (and
it's not done any more). Sorry for the confusion.

[Randy Smith (Not in Mondays), 2011-07-18 01:38:23]

This is the final version which I'll commit from (modulo syncing up to TOT,
which I'm sure I'll need to do).  I did a few comment tweaks in the PS8-PS9
transition; Andy, you're welcome to review those or not as you prefer.  (I'll
treat them as TBR and submit a second patch if you want changes--they're only
comments.)

[commit-bot: I haz the power, 2011-07-18 17:15:12]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 302: Found

[Randy Smith (Not in Mondays), 2011-09-10 01:17:11]

I've regenerated this CL at http://codereview.chromium.org/7796014 and am
closing this one; interested folks are welcome to follow the pointer.  (I came
up with a smaller review list for that CL than this one had, but anyone who
wants to review it is welcome to.)