Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(154)

Issue 11571025: Initial CL for Downloads resumption. (Closed)

Created:
8 years ago by Randy Smith (Not in Mondays)
Modified:
7 years, 11 months ago
CC:
chromium-reviews, asanka, tfarina, jam, Randy Smith (Not in Mondays), joi+watch-content_chromium.org, arv (Not doing code reviews), darin-cc_chromium.org, rdsmith+dwatch_chromium.org, stuartmorgan+watch_chromium.org, benjhayden
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : Builds and runs all tests. #

Patch Set 3 : Sync'd to r173798 #

Patch Set 4 : Ready for review. #

Total comments: 6

Patch Set 5 : Incorporated Andy's comments. #

Patch Set 6 : Fix try bot problems. #

Total comments: 26

Patch Set 7 : Incorporated Asanka's comments. #

Patch Set 8 : Merged to r174772. #

Patch Set 9 : Removed CanResumeDownload (unused) and updated comment in RDH::BeginDownload. #

Total comments: 63

Patch Set 10 : Incorporated comments. #

Patch Set 11 : Sync'd to r175222. #

Patch Set 12 : Incorporated comments. #

Total comments: 14

Patch Set 13 : Made resume test work on Windows. #

Patch Set 14 : Incorporated Ben's comments. #

Patch Set 15 : Sync'd to r176084. #

Total comments: 2

Patch Set 16 : Incorporated Pawel's comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+998 lines, -151 lines) Patch
M chrome/app/generated_resources.grd View 2 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/download/download_item_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/download/download_item_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/plugins/plugin_installer.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/download/base_file.cc View 1 2 3 4 5 6 1 chunk +18 lines, -0 lines 0 comments Download
M content/browser/download/base_file_unittest.cc View 1 2 3 2 chunks +7 lines, -2 lines 0 comments Download
M content/browser/download/download_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +105 lines, -3 lines 0 comments Download
M content/browser/download/download_create_info.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/download/download_file.h View 1 1 chunk +3 lines, -3 lines 0 comments Download
M content/browser/download/download_item_factory.h View 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/download/download_item_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +30 lines, -11 lines 0 comments Download
M content/browser/download/download_item_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 30 chunks +277 lines, -37 lines 0 comments Download
M content/browser/download/download_item_impl_delegate.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +6 lines, -0 lines 0 comments Download
M content/browser/download/download_item_impl_delegate.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/download/download_item_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 27 chunks +208 lines, -33 lines 0 comments Download
M content/browser/download/download_manager_impl.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +9 lines, -3 lines 0 comments Download
M content/browser/download/download_manager_impl.cc View 1 2 3 4 5 6 7 8 9 10 10 chunks +59 lines, -21 lines 0 comments Download
M content/browser/download/download_manager_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +8 lines, -8 lines 0 comments Download
M content/browser/download/download_net_log_parameters.h View 1 2 3 2 chunks +8 lines, -1 line 0 comments Download
M content/browser/download/download_net_log_parameters.cc View 1 chunk +16 lines, -0 lines 0 comments Download
M content/browser/download/download_request_handle.h View 1 1 chunk +1 line, -1 line 0 comments Download
M content/browser/download/download_resource_handler.h View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M content/browser/download/download_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 6 chunks +49 lines, -19 lines 0 comments Download
M content/browser/loader/buffered_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.h View 1 2 3 4 5 6 3 chunks +5 lines, -0 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +4 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/download_interrupt_reason_values.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/browser/download_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/browser/download_url_parameters.h View 3 chunks +10 lines, -0 lines 0 comments Download
M content/public/browser/resource_dispatcher_host.h View 1 2 3 4 5 6 7 8 3 chunks +12 lines, -6 lines 0 comments Download
M content/public/common/content_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/test/mock_download_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M content/public/test/mock_download_manager.h View 1 chunk +2 lines, -0 lines 0 comments Download
M net/tools/testserver/testserver.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +106 lines, -0 lines 0 comments Download

Messages

Total messages: 27 (0 generated)
Randy Smith (Not in Mondays)
[This is not a request for review. ] Sending out a quick note to make ...
8 years ago (2012-12-14 16:32:19 UTC) #1
Randy Smith (Not in Mondays)
Andy, Asanka: Could you take a look at this? I've tried to do something relatively ...
7 years, 12 months ago (2012-12-26 23:20:26 UTC) #2
ahendrickson
LGTM, with some nits. Although I liked the Interrupt/Resume notifications . . . https://codereview.chromium.org/11571025/diff/7001/content/browser/download/download_browsertest.cc File ...
7 years, 11 months ago (2012-12-27 17:10:26 UTC) #3
Randy Smith (Not in Mondays)
(WRT the extra notifications) Yeah, but they were redundant with the state transitions, would have ...
7 years, 11 months ago (2012-12-27 17:51:00 UTC) #4
ahendrickson
On 2012/12/27 17:51:00, rdsmith wrote: https://codereview.chromium.org/11571025/diff/7001/content/browser/download/download_item_impl.cc#newcode1575 > content/browser/download/download_item_impl.cc:1575: if (mode == > RESUME_MODE_IMMEDIATE_RESTART) { > ...
7 years, 11 months ago (2012-12-27 18:31:32 UTC) #5
asanka
I'm assuming that issues with persistence and resuming history downloads are TBD. https://codereview.chromium.org/11571025/diff/22001/content/browser/download/base_file.cc File content/browser/download/base_file.cc ...
7 years, 11 months ago (2012-12-28 22:01:42 UTC) #6
Randy Smith (Not in Mondays)
Yep, persistence and resuming history downloads are TBD, currently blocked on https://codereview.chromium.org/11363222/. https://codereview.chromium.org/11571025/diff/22001/content/browser/download/base_file.cc File content/browser/download/base_file.cc ...
7 years, 11 months ago (2012-12-29 17:16:15 UTC) #7
Randy Smith (Not in Mondays)
Pawel: Could you take a look at net/tools/testserver/testserver.py? There's some reasonably large changes there, so ...
7 years, 11 months ago (2013-01-04 21:39:42 UTC) #8
asanka
A couple of nits and questions, but LGTM. https://codereview.chromium.org/11571025/diff/22001/content/browser/download/base_file.cc File content/browser/download/base_file.cc (right): https://codereview.chromium.org/11571025/diff/22001/content/browser/download/base_file.cc#newcode299 content/browser/download/base_file.cc:299: int64 ...
7 years, 11 months ago (2013-01-04 22:54:16 UTC) #9
Paweł Hajdan Jr.
https://codereview.chromium.org/11571025/diff/61001/net/tools/testserver/testserver.py File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/11571025/diff/61001/net/tools/testserver/testserver.py#newcode1645 net/tools/testserver/testserver.py:1645: for ele in query.split('&'): Are you doing query string ...
7 years, 11 months ago (2013-01-04 22:56:58 UTC) #10
benjhayden
https://codereview.chromium.org/11571025/diff/61001/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/11571025/diff/61001/content/browser/download/download_item_impl.cc#newcode354 content/browser/download/download_item_impl.cc:354: UpdateObservers(); Do we want to UpdateObservers even if !IsInProgress() ...
7 years, 11 months ago (2013-01-06 15:46:03 UTC) #11
Randy Smith (Not in Mondays)
I've at least responded to all comments. Let me know what you think. https://codereview.chromium.org/11571025/diff/61001/content/browser/download/download_item_impl.cc File ...
7 years, 11 months ago (2013-01-07 20:54:10 UTC) #12
benjhayden
https://codereview.chromium.org/11571025/diff/61001/content/browser/download/download_resource_handler.cc File content/browser/download/download_resource_handler.cc (right): https://codereview.chromium.org/11571025/diff/61001/content/browser/download/download_resource_handler.cc#newcode181 content/browser/download/download_resource_handler.cc:181: if (status == 200) { // Continue with download: ...
7 years, 11 months ago (2013-01-07 21:14:33 UTC) #13
Randy Smith (Not in Mondays)
Ben's comments incorporated. Looking for comments/LGTM from Ben & Pawel. Andy, could you also take ...
7 years, 11 months ago (2013-01-08 16:07:02 UTC) #14
benjhayden
LGTM, few nits. https://codereview.chromium.org/11571025/diff/86001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/11571025/diff/86001/chrome/browser/about_flags.cc#newcode829 chrome/browser/about_flags.cc:829: "enable-download-restarts", restarts or resumption? https://codereview.chromium.org/11571025/diff/86001/content/browser/download/download_item_impl.cc File ...
7 years, 11 months ago (2013-01-08 16:41:16 UTC) #15
Randy Smith (Not in Mondays)
Haven't gotten to Ben's last set of comments yet; those are still on my plate. ...
7 years, 11 months ago (2013-01-08 19:31:12 UTC) #16
ahendrickson
On 2013/01/08 16:07:02, rdsmith wrote: > Andy, could you also take a look at the ...
7 years, 11 months ago (2013-01-08 21:53:52 UTC) #17
Randy Smith (Not in Mondays)
Ben: PTAL, let me know if it still LGTY. Pawel: PTAL at testserver.py. https://codereview.chromium.org/11571025/diff/86001/chrome/browser/about_flags.cc File ...
7 years, 11 months ago (2013-01-09 22:34:37 UTC) #18
Randy Smith (Not in Mondays)
Looking for stamps: * bauerb: chrome/browser/plugins/plugin_installer.cc * sky: content/browser/loader/*, content/browser/renderer_host/render_message_filter.cc, content/public/tests/* * avi: contet/public/browser/*, content/public/common/*
7 years, 11 months ago (2013-01-10 19:14:52 UTC) #19
Avi (use Gerrit)
content/public/browser/*, content/public/common/* lgtm
7 years, 11 months ago (2013-01-10 19:19:08 UTC) #20
Bernhard Bauer
You have my axe^H^H^Hrubberstamp. LGTM!
7 years, 11 months ago (2013-01-10 19:54:48 UTC) #21
sky
LGTM
7 years, 11 months ago (2013-01-10 21:31:53 UTC) #22
Paweł Hajdan Jr.
LGTM with a comment. https://codereview.chromium.org/11571025/diff/93001/net/tools/testserver/testserver.py File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/11571025/diff/93001/net/tools/testserver/testserver.py#newcode1690 net/tools/testserver/testserver.py:1690: # Hack alert/explanation: Without writing ...
7 years, 11 months ago (2013-01-10 23:50:21 UTC) #23
benjhayden
lgtm
7 years, 11 months ago (2013-01-11 13:59:38 UTC) #24
Randy Smith (Not in Mondays)
Thanks! https://codereview.chromium.org/11571025/diff/93001/net/tools/testserver/testserver.py File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/11571025/diff/93001/net/tools/testserver/testserver.py#newcode1690 net/tools/testserver/testserver.py:1690: # Hack alert/explanation: Without writing a single byte, ...
7 years, 11 months ago (2013-01-11 19:28:33 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/11571025/101001
7 years, 11 months ago (2013-01-11 19:29:52 UTC) #26
commit-bot: I haz the power
7 years, 11 months ago (2013-01-12 15:57:15 UTC) #27
Message was sent while issue was closed.
Change committed as 176530

Powered by Google App Engine
This is Rietveld 408576698