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

Issue 105853002: Implement a background downloader using BITS in Windows Chrome. (Closed)

Created:
6 years, 1 month ago by Sorin Jianu
Modified:
6 years, 1 month ago
CC:
chromium-reviews
Visibility:
Public.

Description

Implement a background downloader using BITS in Windows Chrome. BUG=304354 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239347

Patch Set 1 #

Total comments: 9

Patch Set 2 : trybots and Josh's feedback. #

Total comments: 67

Patch Set 3 : cpu's feedback #

Total comments: 2

Patch Set 4 : removed useless helpers, fixed timer assert. #

Total comments: 2

Patch Set 5 : More fixes after cpu's review. #

Patch Set 6 : Fixed ref counting. #

Patch Set 7 : Removed BITS observer. #

Patch Set 8 : Fixed typos. #

Total comments: 4

Patch Set 9 : Fixed error codes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+888 lines, -211 lines) Patch
A chrome/browser/component_updater/background_downloader_win.h View 1 2 3 4 5 6 1 chunk +71 lines, -0 lines 0 comments Download
A chrome/browser/component_updater/background_downloader_win.cc View 1 2 3 4 5 6 7 8 1 chunk +534 lines, -0 lines 0 comments Download
M chrome/browser/component_updater/component_updater_configurator.cc View 6 chunks +12 lines, -1 line 0 comments Download
M chrome/browser/component_updater/component_updater_service.h View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/component_updater/component_updater_service.cc View 1 2 5 chunks +16 lines, -10 lines 0 comments Download
M chrome/browser/component_updater/component_updater_utils.h View 1 chunk +61 lines, -58 lines 0 comments Download
M chrome/browser/component_updater/component_updater_utils.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/component_updater/crx_downloader.h View 1 2 2 chunks +37 lines, -18 lines 0 comments Download
M chrome/browser/component_updater/crx_downloader.cc View 1 2 3 4 5 1 chunk +109 lines, -81 lines 0 comments Download
M chrome/browser/component_updater/test/component_updater_service_unittest.h View 1 1 chunk +4 lines, -23 lines 0 comments Download
M chrome/browser/component_updater/test/component_updater_service_unittest.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/component_updater/test/crx_downloader_unittest.cc View 3 chunks +8 lines, -7 lines 0 comments Download
M chrome/browser/component_updater/url_fetcher_downloader.h View 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/browser/component_updater/url_fetcher_downloader.cc View 1 2 3 chunks +16 lines, -9 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 chunks +4 lines, -2 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
Sorin Jianu
There is one important case I want to address in a future CL: when the ...
6 years, 1 month ago (2013-12-04 23:45:08 UTC) #1
waffles
lgtm, modulo the switch statement comment. The other comments are fluff. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): ...
6 years, 1 month ago (2013-12-05 01:15:07 UTC) #2
Sorin Jianu
Thank you! Btw, the Linux try bot failed. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_updater/background_downloader_win.cc#newcode38 chrome/browser/component_updater/background_downloader_win.cc:38: // ...
6 years, 1 month ago (2013-12-05 01:43:07 UTC) #3
cpu_(ooo_6.6-7.5)
starting the review, more to come. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component_updater/background_downloader_win.cc#newcode32 chrome/browser/component_updater/background_downloader_win.cc:32: // BITS job ...
6 years, 1 month ago (2013-12-05 19:32:43 UTC) #4
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component_updater/background_downloader_win.cc#newcode152 chrome/browser/component_updater/background_downloader_win.cc:152: } seems we should have here the logic of ...
6 years, 1 month ago (2013-12-05 21:16:31 UTC) #5
cpu_(ooo_6.6-7.5)
how does it work if at 3pm we check and queue a BITs update for ...
6 years, 1 month ago (2013-12-05 21:22:00 UTC) #6
Sorin Jianu
Thank you Carlos, let's have another look. All comments answered, and many of them acted ...
6 years, 1 month ago (2013-12-05 22:34:35 UTC) #7
Sorin Jianu
On 2013/12/05 21:22:00, cpu wrote: > how does it work if at 3pm we check ...
6 years, 1 month ago (2013-12-05 22:43:31 UTC) #8
cpu_(ooo_6.6-7.5)
Also could the observer be solely held by the reference keept by the job? Btw, ...
6 years, 1 month ago (2013-12-06 00:12:51 UTC) #9
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/105853002/diff/60001/chrome/browser/component_updater/crx_downloader.cc File chrome/browser/component_updater/crx_downloader.cc (right): https://codereview.chromium.org/105853002/diff/60001/chrome/browser/component_updater/crx_downloader.cc#newcode54 chrome/browser/component_updater/crx_downloader.cc:54: urls.push_back(url); but we still have this method, so if ...
6 years, 1 month ago (2013-12-06 00:23:19 UTC) #10
cpu_(ooo_6.6-7.5)
done with review.
6 years, 1 month ago (2013-12-06 00:25:55 UTC) #11
Sorin Jianu
I hope I addressed all the comments, many of them overlapped in flight. Thank you ...
6 years, 1 month ago (2013-12-06 01:37:35 UTC) #12
Sorin Jianu
On 2013/12/06 00:12:51, cpu wrote: > Also could the observer be solely held by the ...
6 years, 1 month ago (2013-12-06 02:25:56 UTC) #13
Sorin Jianu
https://codereview.chromium.org/105853002/diff/40001/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/40001/chrome/browser/component_updater/background_downloader_win.cc#newcode639 chrome/browser/component_updater/background_downloader_win.cc:639: job_observer->AddRef(); On 2013/12/06 00:12:52, cpu wrote: > you need ...
6 years, 1 month ago (2013-12-06 03:00:57 UTC) #14
cpu_(ooo_6.6-7.5)
lgtm. Excellent job btw.
6 years, 1 month ago (2013-12-06 18:41:46 UTC) #15
cpu_(ooo_6.6-7.5)
As for the questions above, I am not too concerned and mostly agree including no ...
6 years, 1 month ago (2013-12-06 18:48:03 UTC) #16
Sorin Jianu
Josh, I removed the observer, let's take another look please.
6 years, 1 month ago (2013-12-06 21:33:16 UTC) #17
waffles
lgtm, except the two hr = job_->Cancels. https://codereview.chromium.org/105853002/diff/140001/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/140001/chrome/browser/component_updater/background_downloader_win.cc#newcode384 chrome/browser/component_updater/background_downloader_win.cc:384: hr = ...
6 years, 1 month ago (2013-12-06 22:08:56 UTC) #18
Sorin Jianu
Done both, thank you JOsh! https://codereview.chromium.org/105853002/diff/140001/chrome/browser/component_updater/background_downloader_win.cc File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/140001/chrome/browser/component_updater/background_downloader_win.cc#newcode384 chrome/browser/component_updater/background_downloader_win.cc:384: hr = job_->Cancel(); On ...
6 years, 1 month ago (2013-12-06 23:18:03 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/105853002/160001
6 years, 1 month ago (2013-12-06 23:31:38 UTC) #20
commit-bot: I haz the power
The commit queue went berserk retrying too often for a seemingly flaky test on builder ...
6 years, 1 month ago (2013-12-07 15:14:02 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/105853002/160001
6 years, 1 month ago (2013-12-07 16:16:04 UTC) #22
commit-bot: I haz the power
6 years, 1 month ago (2013-12-07 21:54:41 UTC) #23
Message was sent while issue was closed.
Change committed as 239347

Powered by Google App Engine
This is Rietveld 408576698