Restructure URLRequestSlowDownloadJob to avoid races.

Restructure URLRequestSlowDownloadJob to avoid races.  

This is the believed cause of recent DownloadTest.*nownSize flakiness.

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

[Randy Smith (Not in Mondays), 2011-11-16 00:08:22]

Could you guys take a look at this?  Andy for changes to download_browsertest.cc
and because I'm modifying his changes, Eric because this is enough of a change
that I'd like to get the eyes of someone who understands URLRequestJob protocol
pretty thoroughly on it.

[ahendrickson, 2011-11-16 01:21:37]

On 2011/11/16 00:08:22, rdsmith wrote:
> Could you guys take a look at this?  Andy for changes to
download_browsertest.cc
> and because I'm modifying his changes, Eric because this is enough of a change
> that I'd like to get the eyes of someone who understands URLRequestJob
protocol
> pretty thoroughly on it.

LGTM.

[Randy Smith (Not in Mondays), 2011-11-17 16:51:21]

eroman: Ping?

[eroman, 2011-11-18 01:54:46]

lgtm

http://codereview.chromium.org/8468028/diff/4002/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/8468028/diff/4002/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:663: << "File string comparison
mismatch.  Expected: ----\n" << value
I believe EXPECT_EQ prints the mismatch, so shouldn't need to explicitly print
value/file_contents.

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

http://codereview.chromium.org/8468028/diff/4002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:99: // ReadRawData and
CheckDoneStatus implement together implement a state
nit: redundant "implement".

[Randy Smith (Not in Mondays), 2011-11-18 15:30:18]

Publishing comment incorporation FYI; no further review needed.  Thanks!

http://codereview.chromium.org/8468028/diff/4002/chrome/browser/download/down...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/8468028/diff/4002/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:663: << "File string comparison
mismatch.  Expected: ----\n" << value
On 2011/11/18 01:54:46, eroman wrote:
> I believe EXPECT_EQ prints the mismatch, so shouldn't need to explicitly print
> value/file_contents.

Done.

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

http://codereview.chromium.org/8468028/diff/4002/content/browser/net/url_requ...
content/browser/net/url_request_slow_download_job.cc:99: // ReadRawData and
CheckDoneStatus implement together implement a state
On 2011/11/18 01:54:46, eroman wrote:
> nit: redundant "implement".

Done.

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

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

[commit-bot: I haz the power, 2011-11-18 20:25:12]

Try job failure for 8468028-12001 (retry) on win_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-18 21:36:26]

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

[commit-bot: I haz the power, 2011-11-18 22:39:31]

Try job failure for 8468028-12001 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-21 20:59:36]

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

[commit-bot: I haz the power, 2011-11-22 00:08:25]

Change committed as 111046