Preliminary work on resuming downloads whose connections have expired.

Handle errors during download gracefully, and enable resuming the download after such errors.

- Added a OnDownloadInterrupted() member function to the DownloadItem::Observer class, to complement OnDownloadFileCompleted().

BUG=42625, 35278
TEST=Navigate to http://build.chromium.org/buildbot/continuous/linux64/2010-08-20/56863/
Download the ZIP file, then pause it before it's finished.
Wait 10 minutes (until the server times us out) then resume the download -- it should show up as interrupted.
Resume downloading again, and the download should continue until it's done.

Alternative method: Run a small server, using the code below, and retrieve any file with a '.zip' extension from it. 

========== BEGIN CODE =========== 

#!/usr/bin/python 

import BaseHTTPServer 
import time 

class SlowHandler(BaseHTTPServer.BaseHTTPRequestHandler): 
def do_GET(self): 
print self.headers 
self.send_response(200) 
self.send_header('Content-Type', 'application/zip') 
self.send_header('Content-Length', '1000000') 
self.end_headers() 
content_length = 0 
while content_length < 10000: 
output_string = 'HAPPY HAPPY JOY JOY\r\n' 
self.wfile.write(output_string) 
content_length += len(output_string) 
time.sleep(0.05) 

def CreateAndRunServer(): 
server = BaseHTTPServer.HTTPServer(('', 4970), SlowHandler) 
server.serve_forever() 

if __name__ == '__main__': 
CreateAndRunServer() 

========== END CODE =========== 

This uses http://docs.python.org/library/basehttpserver.html#BaseHTTPServer.HTTPServer 

(Thanks, Chris!)

[Paweł Hajdan Jr., 2010-08-23 17:16:48]

http://codereview.chromium.org/3127008/diff/4001/5010
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/3127008/diff/4001/5010#newcode209
chrome/browser/download/download_item.cc:209: if ((state_ == COMPLETE) ||
(state_ == INTERRUPTED)) {
This is going to be a mess. We should have some method that effectively returns
!(state_ == COMPLETE || state_ == INTERRUPTED). Maybe IsPending? But then there
is still state REMOVING.

http://codereview.chromium.org/3127008/diff/4001/5010#newcode239
chrome/browser/download/download_item.cc:239: is_interrupted_ = true;
Isn't is_interrupted just equivalent to (state_ == INTERRUPTED)? If it is, let's
simplify it. If it isn't, then it seems wrong.

http://codereview.chromium.org/3127008/diff/4001/5011
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/3127008/diff/4001/5011#newcode38
chrome/browser/download/download_item.h:38: enum DownloadState {
We should document each state here. Difference between interrupted and cancelled
isn't obvious, and REMOVING should probably be last state.

http://codereview.chromium.org/3127008/diff/4001/5014
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/3127008/diff/4001/5014#newcode313
chrome/browser/download/download_manager.h:313: static void
OnRestartDownloadRequest(ResourceDispatcherHost* rdh,
Do we need this in the header? Can it be moved to an anonymous namespace?

http://codereview.chromium.org/3127008/diff/4001/5015
File chrome/browser/download/download_shelf.cc (right):

http://codereview.chromium.org/3127008/diff/4001/5015#newcode53
chrome/browser/download/download_shelf.cc:53: if (download_->is_paused() ||
download_->is_interrupted())
We should move this || logic to download_. is_resumable()?

http://codereview.chromium.org/3127008/diff/4001/5015#newcode72
chrome/browser/download/download_shelf.cc:72: return ((download_->state() ==
DownloadItem::IN_PROGRESS) ||
Similar thing here.

http://codereview.chromium.org/3127008/diff/4001/5016
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/3127008/diff/4001/5016#newcode287
chrome/browser/download/download_util.cc:287: static const double PI =
3.141592653589793;
Don't we have this in some standard header?

http://codereview.chromium.org/3127008/diff/4001/5017
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/3127008/diff/4001/5017#newcode177
chrome/browser/download/download_util.h:177: void
ReDownloadUrl(ResourceDispatcherHost* rdh,
I'd prefer Restart instead of Re.

[ahendrickson, 2010-09-03 20:14:35]

http://codereview.chromium.org/3127008/diff/4001/5010
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/3127008/diff/4001/5010#newcode239
chrome/browser/download/download_item.cc:239: is_interrupted_ = true;
Fixed.

On 2010/08/23 17:16:48, Paweł Hajdan Jr. wrote:
> Isn't is_interrupted just equivalent to (state_ == INTERRUPTED)? If it is,
let's
> simplify it. If it isn't, then it seems wrong.

http://codereview.chromium.org/3127008/diff/4001/5011
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/3127008/diff/4001/5011#newcode38
chrome/browser/download/download_item.h:38: enum DownloadState {
I'm worried about changing the value of the individual states, in case they are
persisted by value.

On 2010/08/23 17:16:48, Paweł Hajdan Jr. wrote:
> We should document each state here. Difference between interrupted and
cancelled
> isn't obvious, and REMOVING should probably be last state.

http://codereview.chromium.org/3127008/diff/4001/5014
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/3127008/diff/4001/5014#newcode313
chrome/browser/download/download_manager.h:313: static void
OnRestartDownloadRequest(ResourceDispatcherHost* rdh,
Removed.

On 2010/08/23 17:16:48, Paweł Hajdan Jr. wrote:
> Do we need this in the header? Can it be moved to an anonymous namespace?

http://codereview.chromium.org/3127008/diff/4001/5016
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/3127008/diff/4001/5016#newcode287
chrome/browser/download/download_util.cc:287: static const double PI =
3.141592653589793;
Nope.

On 2010/08/23 17:16:48, Paweł Hajdan Jr. wrote:
> Don't we have this in some standard header?

http://codereview.chromium.org/3127008/diff/4001/5017
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/3127008/diff/4001/5017#newcode177
chrome/browser/download/download_util.h:177: void
ReDownloadUrl(ResourceDispatcherHost* rdh,
Will do.

On 2010/08/23 17:16:48, Paweł Hajdan Jr. wrote:
> I'd prefer Restart instead of Re.

[ahendrickson, 2010-09-09 21:30:42]

Ping.

[Paweł Hajdan Jr., 2010-09-30 16:40:11]

Is this CL replaced now by http://codereview.chromium.org/3273013/show ?

If not, please just let me know and I'll review it tomorrow.

Otherwise, please close this.

[hendrickson_a, 2010-09-30 17:19:31]

No, I'm has become the second of 2 halves.

I need to keep it around.

Andy

On Thu, Sep 30, 2010 at 12:40 PM,  <phajdan.jr@chromium.org> wrote:
> Is this CL replaced now by http://codereview.chromium.org/3273013/show ?
>
> If not, please just let me know and I'll review it tomorrow.
>
> Otherwise, please close this.
>
> http://codereview.chromium.org/3127008/show
>

[hendrickson_a, 2010-09-30 17:19:50]

One will depend on the other, when the first is committed.

Andy

On Thu, Sep 30, 2010 at 12:40 PM,  <phajdan.jr@chromium.org> wrote:
> Is this CL replaced now by http://codereview.chromium.org/3273013/show ?
>
> If not, please just let me know and I'll review it tomorrow.
>
> Otherwise, please close this.
>
> http://codereview.chromium.org/3127008/show
>

[Paweł Hajdan Jr., 2010-10-01 09:03:55]

I'm not sure if you expected me to review this (please be explicit whether you
do want me to review this now), so I've added a few comments.

My general impression is that it's quite hard to review, and it's a mix of
earlier versions of http://codereview.chromium.org/3273013/show possibly with
some other changes.

I'd suggest that we review the first patch
(http://codereview.chromium.org/3273013/show) and land it, and only then return
to this CL (after it is rebased on top of trunk).

If you prefer a preliminary review of this CL, could you point me to the places
I should focus on?

http://codereview.chromium.org/3127008/diff/74001/75004
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/3127008/diff/74001/75004#newcode1152
chrome/browser/automation/automation_provider_observers.cc:1152: void
AutomationProviderDownloadItemObserver::OnDownloadInterrupted(
This is broken, as said in http://codereview.chromium.org/3273013/show

http://codereview.chromium.org/3127008/diff/74001/75013
File chrome/browser/download/base_file.cc (right):

http://codereview.chromium.org/3127008/diff/74001/75013#newcode52
chrome/browser/download/base_file.cc:52: bool opened = Open();
I think it would be cleaner to just expose Open then.

nit: just return Open() is simpler.

http://codereview.chromium.org/3127008/diff/74001/75014
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/3127008/diff/74001/75014#newcode30
chrome/browser/download/base_file.h:30: bool ReOpen();
Please add comment.

http://codereview.chromium.org/3127008/diff/74001/75014#newcode56
chrome/browser/download/base_file.h:56: bool MatchesUrlAndPath(const GURL& url,
const FilePath& path) const;
This is wrong, as said in http://codereview.chromium.org/3273013/show

http://codereview.chromium.org/3127008/diff/74001/75015
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/3127008/diff/74001/75015#newcode84
chrome/browser/download/download_file_manager.cc:84: DownloadFileMap::iterator
item = downloads_.find(info->download_id);
This is wrong, as said (and I think fixed) in
http://codereview.chromium.org/3273013/show