Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1360)

Issue 3127008: Preliminary work on resuming downloads whose connections have expired.

Created:
9 years, 9 months ago by ahendrickson
Modified:
9 years, 8 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, ben+cc_chromium.org, John Grabowski, Paul Godavari, brettw-cc_chromium.org, pam+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., Erik does not do reviews, Aaron Boodman, davemoore+watch_chromium.org
Visibility:
Public.

Description

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!)

Patch Set 1 #

Patch Set 2 : First successful restart of an interrupted download. #

Patch Set 3 : Resuming interrupted download works from download shelf. #

Total comments: 13

Patch Set 4 : Can now restart downloads multiple times. #

Patch Set 5 : Fixed some presubmit warnings. #

Patch Set 6 : Merged with trunk. #

Patch Set 7 : Changes due to CL comments. #

Patch Set 8 : Merged with trunk. Updated per issue 3273013. #

Patch Set 9 : Renamed ReDownloadUrl to RestartDownloadUrl. #

Patch Set 10 : Merged with Pawel's reversions in trunk. #

Patch Set 11 : Fix for merge bug, and cleanup. #

Patch Set 12 : virtual void OnDownloadOpened(DownloadItem* download); #

Patch Set 13 : Cleanup per Chris' comments on CL #3273013, and linux build fixes. #

Patch Set 14 : Fixed DCHECK sense bug. #

Patch Set 15 : Fixed deletion of PRESUBMIT.py due to a case change. #

Patch Set 16 : Added command-line flag to enable restarting of downloads. #

Patch Set 17 : Merged with trunk. #

Patch Set 18 : Merged with trunk. #

Patch Set 19 : Fixed nits, and automation provider notification for interrupted downloads. #

Patch Set 20 : Merged with trunk. #

Patch Set 21 : New fix for interruped download notification for automation provider. #

Patch Set 22 : Waiting to send download automation error message until after other downloads are canceled. #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+1091 lines, -236 lines) Patch
M base/file_util_win.cc View 4 5 6 7 1 chunk +11 lines, -2 lines 0 comments Download
M chrome/app/generated_resources.grd View 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/automation/automation_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/automation/automation_provider_observers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +11 lines, -5 lines 0 comments Download
M chrome/browser/automation/automation_provider_observers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +35 lines, -1 line 1 comment Download
M chrome/browser/chromeos/dom_ui/imageburner_ui.h View 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/download_item_cell.mm View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/download_item_controller.mm View 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/cocoa/download_item_mac.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/cocoa/download_item_mac.mm View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/dom_ui/downloads_dom_handler.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/dom_ui/filebrowse_ui.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/download/base_file.h View 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +4 lines, -0 lines 2 comments Download
M chrome/browser/download/base_file.cc View 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +10 lines, -0 lines 1 comment Download
M chrome/browser/download/download_file_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +7 lines, -1 line 0 comments Download
M chrome/browser/download/download_file_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +68 lines, -28 lines 1 comment Download
M chrome/browser/download/download_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +21 lines, -1 line 0 comments Download
M chrome/browser/download/download_item.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 9 chunks +45 lines, -8 lines 0 comments Download
M chrome/browser/download/download_item_model.cc View 1 2 3 4 5 6 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/download/download_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +10 lines, -0 lines 0 comments Download
M chrome/browser/download/download_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 21 chunks +146 lines, -35 lines 0 comments Download
M chrome/browser/download/download_shelf.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 6 chunks +10 lines, -4 lines 0 comments Download
M chrome/browser/download/download_uitest.cc View 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/download/download_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +24 lines, -0 lines 0 comments Download
M chrome/browser/download/download_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 8 chunks +140 lines, -2 lines 0 comments Download
M chrome/browser/download/drag_download_file.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/gtk/download_item_gtk.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/gtk/download_item_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/history/download_create_info.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/history/download_create_info.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/history/download_database.cc View 2 chunks +17 lines, -4 lines 0 comments Download
M chrome/browser/renderer_host/download_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +58 lines, -5 lines 0 comments Download
M chrome/browser/renderer_host/resource_dispatcher_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 9 chunks +42 lines, -7 lines 0 comments Download
M chrome/browser/renderer_host/resource_dispatcher_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 48 chunks +322 lines, -120 lines 0 comments Download
M chrome/browser/renderer_host/resource_dispatcher_host_request_info.h View 1 2 3 4 5 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/resource_dispatcher_host_request_info.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/renderer_host/resource_message_filter.cc View 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/views/download_item_view.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/views/download_item_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +25 lines, -1 line 0 comments Download
M chrome/browser/views/download_shelf_view.cc View 1 chunk +9 lines, -2 lines 0 comments Download
M chrome/common/chrome_switches.h View 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/test/ui_test_utils.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M net/base/file_stream_win.cc View 4 5 2 chunks +8 lines, -1 line 0 comments Download
M net/url_request/url_request.cc View 3 4 5 1 chunk +3 lines, -2 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Paweł Hajdan Jr.
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)) { ...
9 years, 9 months ago (2010-08-23 17:16:48 UTC) #1
ahendrickson
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 ...
9 years, 8 months ago (2010-09-03 20:14:35 UTC) #2
ahendrickson
Ping.
9 years, 8 months ago (2010-09-09 21:30:42 UTC) #3
Paweł Hajdan Jr.
Is this CL replaced now by http://codereview.chromium.org/3273013/show ? If not, please just let me know ...
9 years, 8 months ago (2010-09-30 16:40:11 UTC) #4
hendrickson_a
No, I'm has become the second of 2 halves. I need to keep it around. ...
9 years, 8 months ago (2010-09-30 17:19:31 UTC) #5
hendrickson_a
One will depend on the other, when the first is committed. Andy On Thu, Sep ...
9 years, 8 months ago (2010-09-30 17:19:50 UTC) #6
Paweł Hajdan Jr.
9 years, 8 months ago (2010-10-01 09:03:55 UTC) #7
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

Powered by Google App Engine
This is Rietveld 408576698