Add the download component and initial setup

Add the download component and initial setup

- Add the BackgroundDownloadService interface and associated files.  Set
up the test hooks and tie it into the proper Chrome and
component_unittest targets.

BUG=717180
Review-Url: https://codereview.chromium.org/2851303003
Cr-Commit-Position: refs/heads/master@{#469371}
Committed: https://chromium.googlesource.com/chromium/src/+/620a4a8839bc2ebc46d923c33ef2435f6b8cb6e1

[David Trainor- moved to gerrit, 2017-05-01 20:52:41]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 20:53:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Trainor- moved to gerrit, 2017-05-01 21:10:51]

dtrainor@chromium.org changed reviewers:
+ jochen@chromium.org, xingliu@chromium.org

[David Trainor- moved to gerrit, 2017-05-01 21:10:52]

ptal thanks!

[xingliu, 2017-05-01 23:56:55]

lgtm with nits.

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_constants.cc:144: FPL("Download Service/persisted");
nit: "/" This might not work on windows. Not sure, but all path with a separator
here seems to be platform specified.

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_constants.h:52: extern const base::FilePath::CharType
kDownloadServiceTemporaryStorageDirname[];
Shall we remove it or define this in chrome_constants.cc?

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/download_params.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/download_params.h:38: BATTERY_SENSITIVE = 0,
nit: Can we put BATTERY_INSENSITIVE before BATTERY_SENSITIVE? Since network
requirement and priority are sorted from loose to strict.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/download_params.h:89: net::HttpRequestHeaders
request_headers;
nit: Not necessary in this patch, but shall we avoid to copy DownloadParam in
the future because of |request_headers|?

[jochen (gone - plz use gerrit), 2017-05-02 09:58:22]

jochen@chromium.org changed reviewers:
+ peter@chromium.org, rdsmith@chromium.org

[jochen (gone - plz use gerrit), 2017-05-02 09:58:23]

+peter

this is supposed to also support Background Fetch (according to the design doc).
Would you be a good reviewer for this?

+rdsmith

does this back sense for downloads?

[Randy Smith (Not in Mondays), 2017-05-02 12:25:00]

Description was changed from

==========
Add the download component and initial setup

- Add the BackgroundDownloadService interface and associated files.  Set
up the test hooks and tie it into the proper Chrome and
component_unittest targets.

BUG=717180
==========

to

==========
Add the download component and initial setup

- Add the BackgroundDownloadService interface and associated files.  Set
up the test hooks and tie it into the proper Chrome and
component_unittest targets.

BUG=717180
==========

[Randy Smith (Not in Mondays), 2017-05-02 12:27:38]

On 2017/05/02 09:58:23, jochen wrote:
> +peter
> 
> this is supposed to also support Background Fetch (according to the design
doc).
> Would you be a good reviewer for this?
> 
> +rdsmith
> 
> does this back sense for downloads?

Jochen: I (and +Asanka, whose familiarity with downloads is more recent) have
been lightly following this at the design doc level, think it's ok, and
dtrainor@ is now in charge of downloads.  So I don't think this needs a lot of
review from us.  Let me know if there's some more detailed review you think is
needed.

[Randy Smith (Not in Mondays), 2017-05-02 12:28:21]

rdsmith@chromium.org changed reviewers:
- rdsmith@chromium.org

[Peter Beverloo, 2017-05-02 16:12:21]

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_constants.cc:144: FPL("Download Service/persisted");
On 2017/05/01 23:56:54, xingliu wrote:
> nit: "/" This might not work on windows. Not sure, but all path with a
separator
> here seems to be platform specified.

+1. Best to just give the "Download Service" directory to the Service and have
it append "persisted" (and/or "temporary") itself?

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_constants.h:52: extern const base::FilePath::CharType
kDownloadServiceTemporaryStorageDirname[];
On 2017/05/01 23:56:54, xingliu wrote:
> Shall we remove it or define this in chrome_constants.cc?

+1 to removing, it's unused

https://codereview.chromium.org/2851303003/diff/20001/components/download/DEPS
File components/download/DEPS (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/DEP...
components/download/DEPS:2: "-content",
So how will you start the actual download? More specific DEPS deeper down?

https://codereview.chromium.org/2851303003/diff/20001/components/download/int...
File components/download/internal/background_download_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/int...
components/download/internal/background_download_service_impl_unittest.cc:11: }
I'd just remove this file. The name of the test doesn't match the class it'd
hypothetically be testing, and it doesn't do anything anyway.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/background_download_service.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/background_download_service.h:30: class
BackgroundDownloadService : public KeyedService {
Why did you decide to go with BackgroundDownloadService and not DownloadService
per the design document?

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/client.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:32: void OnServiceInitialized(
Are the `virtual` keywords missing everywhere?

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:33: const std::vector<std::string>&
outstanding_download_guids);
= 0?

It'd be good to have either everything be pure virtual, or nothing.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:40: const GURL& final_url,
Why not an std::vector<GURL> with the entire URL chain? It's available from
DownloadItem::GetUrlChain() and at least Background Fetch will consume it.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:41: const scoped_refptr<const
net::HttpResponseHeaders> headers) = 0;
const&?

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:43: void OnDownloadUpdated(const
std::string& guid,
docs? Especially the frequency would be interesting to know.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:54: // retries or a specific number of bytes
wasted.
"or a specific number of bytes wasted" what does that mean?

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:60: // time (hours).
define "time (hours)"

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:65: };
+virtual dtor

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/clients.h:13: // DownloadService.DownloadClients in
histograms.xml.
nit: mention that it's append only and that existing values should not be
changed.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/download_params.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/download_params.h:29: OPTIMISTIC = 1,
nit: the user would use this value as SchedulingParams::OPTIMISTIC, which does
not convey that it's just about the network requirements. Usually we use one of
these two:

  1) enum class
  2) prefix with type, NETWORK_REQUIREMENTS_OPTIMISTIC

[Peter Beverloo, 2017-05-02 16:16:47]

https://codereview.chromium.org/2851303003/diff/20001/components/download/int...
File components/download/internal/background_download_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/int...
components/download/internal/background_download_service_impl_unittest.cc:11: }
On 2017/05/02 16:12:20, Peter Beverloo wrote:
> I'd just remove this file. The name of the test doesn't match the class it'd
> hypothetically be testing, and it doesn't do anything anyway.

Oh to add the target. Sure. Rename the test anyway? :)

[David Trainor- moved to gerrit, 2017-05-03 06:02:51]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[David Trainor- moved to gerrit, 2017-05-03 06:02:59]

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_constants.cc:144: FPL("Download Service/persisted");
On 2017/05/02 16:12:20, Peter Beverloo wrote:
> On 2017/05/01 23:56:54, xingliu wrote:
> > nit: "/" This might not work on windows. Not sure, but all path with a
> separator
> > here seems to be platform specified.
> 
> +1. Best to just give the "Download Service" directory to the Service and have
> it append "persisted" (and/or "temporary") itself?

Can do!  I was looking at Offline Page below which all had a '/'.

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/2851303003/diff/20001/chrome/common/chrome_co...
chrome/common/chrome_constants.h:52: extern const base::FilePath::CharType
kDownloadServiceTemporaryStorageDirname[];
On 2017/05/02 16:12:20, Peter Beverloo wrote:
> On 2017/05/01 23:56:54, xingliu wrote:
> > Shall we remove it or define this in chrome_constants.cc?
> 
> +1 to removing, it's unused

Thanks!  My bad I forgot to remove the header definition.

https://codereview.chromium.org/2851303003/diff/20001/components/download/DEPS
File components/download/DEPS (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/DEP...
components/download/DEPS:2: "-content",
On 2017/05/02 16:12:20, Peter Beverloo wrote:
> So how will you start the actual download? More specific DEPS deeper down?

Ah yeah components/download/content was going to allow the content deps.  I can
pull the DEPS file into each separate directory though.  That's much better.

https://codereview.chromium.org/2851303003/diff/20001/components/download/int...
File components/download/internal/background_download_service_impl_unittest.cc
(right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/int...
components/download/internal/background_download_service_impl_unittest.cc:11: }
On 2017/05/02 16:16:47, Peter Beverloo wrote:
> On 2017/05/02 16:12:20, Peter Beverloo wrote:
> > I'd just remove this file. The name of the test doesn't match the class it'd
> > hypothetically be testing, and it doesn't do anything anyway.
> 
> Oh to add the target. Sure. Rename the test anyway? :)

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/background_download_service.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/background_download_service.h:30: class
BackgroundDownloadService : public KeyedService {
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> Why did you decide to go with BackgroundDownloadService and not
DownloadService
> per the design document?

Will change to DownloadService once your other CL lands.  Thanks for renaming
that! :)

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/client.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:32: void OnServiceInitialized(
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> Are the `virtual` keywords missing everywhere?

O_o

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:33: const std::vector<std::string>&
outstanding_download_guids);
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> = 0?
> 
> It'd be good to have either everything be pure virtual, or nothing.

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:40: const GURL& final_url,
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> Why not an std::vector<GURL> with the entire URL chain? It's available from
> DownloadItem::GetUrlChain() and at least Background Fetch will consume it.

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:41: const scoped_refptr<const
net::HttpResponseHeaders> headers) = 0;
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> const&?

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:43: void OnDownloadUpdated(const
std::string& guid,
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> docs? Especially the frequency would be interesting to know.

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:54: // retries or a specific number of bytes
wasted.
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> "or a specific number of bytes wasted" what does that mean?

Failing only based on retry count is probably not the best decision.  We could
retry more than the default 3 times if we failed after 10 bytes.  But if we
failed after 1gb we might want to just give up even if we have another chance to
retry.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:60: // time (hours).
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> define "time (hours)"

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/client.h:65: };
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> +virtual dtor

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/clients.h:13: // DownloadService.DownloadClients in
histograms.xml.
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> nit: mention that it's append only and that existing values should not be
> changed.

Will do.  Histogram isn't added yet since there is no metric that will actually
touch it.  Will add that the moment we add the underlying DownloadService
implementation.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
File components/download/public/download_params.h (right):

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/download_params.h:29: OPTIMISTIC = 1,
On 2017/05/02 16:12:21, Peter Beverloo wrote:
> nit: the user would use this value as SchedulingParams::OPTIMISTIC, which does
> not convey that it's just about the network requirements. Usually we use one
of
> these two:
> 
>   1) enum class
>   2) prefix with type, NETWORK_REQUIREMENTS_OPTIMISTIC

ah enum class is nice thanks!

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/download_params.h:38: BATTERY_SENSITIVE = 0,
On 2017/05/01 23:56:54, xingliu wrote:
> nit: Can we put BATTERY_INSENSITIVE before BATTERY_SENSITIVE? Since network
> requirement and priority are sorted from loose to strict.

Done.

https://codereview.chromium.org/2851303003/diff/20001/components/download/pub...
components/download/public/download_params.h:89: net::HttpRequestHeaders
request_headers;
On 2017/05/01 23:56:54, xingliu wrote:
> nit: Not necessary in this patch, but shall we avoid to copy DownloadParam in
> the future because of |request_headers|?

Will that cause an issue?  I did add copy constructors to these structs though.

[commit-bot: I haz the power, 2017-05-03 06:03:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 06:06:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 06:06:43]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[David Trainor- moved to gerrit, 2017-05-03 06:09:21]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 06:09:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 06:13:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 06:13:05]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jochen (gone - plz use gerrit), 2017-05-03 10:39:35]

rubberstamp lgtm on adding the component. Please wait for peter@ to finish his
review as well

[Peter Beverloo, 2017-05-03 13:02:50]

lgtm % {Background,}DownloadService rename

https://codereview.chromium.org/2851303003/diff/60001/components/download/int...
File components/download/internal/BUILD.gn (right):

https://codereview.chromium.org/2851303003/diff/60001/components/download/int...
components/download/internal/BUILD.gn:10: static_library("internal") {
This target needs to depend on the public target too.

https://codereview.chromium.org/2851303003/diff/60001/components/download/pub...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2851303003/diff/60001/components/download/pub...
components/download/public/clients.h:23: BOUNDARY = 2,
You'd use this as download::BOUNDARY, which means it's super likely to collide
with some other value/enumeration. Using an `enum class` would require
static_cast<>s to get the integral values, so maybe prefix these instead?

DOWNLOAD_CLIENT_INVALID,
DOWNLOAD_CLIENT_OFFLINE_PAGE_PREFETCH,
DOWNLOAD_CLIENT_BOUNDARY

[David Trainor- moved to gerrit, 2017-05-03 16:47:05]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 16:47:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 17:04:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 17:04:15]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[David Trainor- moved to gerrit, 2017-05-03 17:42:04]

https://codereview.chromium.org/2851303003/diff/60001/components/download/int...
File components/download/internal/BUILD.gn (right):

https://codereview.chromium.org/2851303003/diff/60001/components/download/int...
components/download/internal/BUILD.gn:10: static_library("internal") {
On 2017/05/03 13:02:50, Peter Beverloo wrote:
> This target needs to depend on the public target too.

Done.

https://codereview.chromium.org/2851303003/diff/60001/components/download/pub...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2851303003/diff/60001/components/download/pub...
components/download/public/clients.h:23: BOUNDARY = 2,
On 2017/05/03 13:02:50, Peter Beverloo wrote:
> You'd use this as download::BOUNDARY, which means it's super likely to collide
> with some other value/enumeration. Using an `enum class` would require
> static_cast<>s to get the integral values, so maybe prefix these instead?
> 
> DOWNLOAD_CLIENT_INVALID,
> DOWNLOAD_CLIENT_OFFLINE_PAGE_PREFETCH,
> DOWNLOAD_CLIENT_BOUNDARY

Done.

[David Trainor- moved to gerrit, 2017-05-03 17:42:48]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 17:43:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Trainor- moved to gerrit, 2017-05-03 17:48:47]

dtrainor@chromium.org changed reviewers:
+ brettw@chromium.org, dcheng@chromium.org, rdsmith@chromium.org

[David Trainor- moved to gerrit, 2017-05-03 17:48:48]

brettw@: Can I get a review for adding url/ to DEPS?
dcheng@: Can I get a review for adding base/ to DEPS?
rdsmith@: Since you're on this already, can I get a review for adding net/ to
DEPS? :)

Thanks!

[commit-bot: I haz the power, 2017-05-03 18:05:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 18:05:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Randy Smith (Not in Mondays), 2017-05-03 18:09:00]

On 2017/05/03 17:48:48, David Trainor-ping if over 24h wrote:
> brettw@: Can I get a review for adding url/ to DEPS?
> dcheng@: Can I get a review for adding base/ to DEPS?
> rdsmith@: Since you're on this already, can I get a review for adding net/ to
> DEPS? :)

D'oh!!  :-} .  

DEPS -> /net LGTM.

> 
> Thanks!

[dcheng, 2017-05-04 00:19:27]

base DEPS lgtm

https://codereview.chromium.org/2851303003/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_service_factory.cc (right):

https://codereview.chromium.org/2851303003/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_service_factory.cc:43:
base::TaskPriority::BACKGROUND));
Based on CLs like https://codereview.chromium.org/2858073002/, I think this
should use the constexpr overload if possible: for example, I think this can be
written

base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(),
base::TaskPriority::BACKGROUND})

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
components/download/public/clients.h:17: enum DownloadClient {
enum class here as well?

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
File components/download/public/download_params.h (right):

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
components/download/public/download_params.h:118: // A unique GUID that
represents this download.  See |base::GenerateGUID()|.
Unrelated: I kind of wonder if we should just have a distinct type for GUID
rather than a std::string blob...

[David Trainor- moved to gerrit, 2017-05-04 00:47:25]

dtrainor@chromium.org changed reviewers:
+ mkwst@chromium.org

[David Trainor- moved to gerrit, 2017-05-04 00:47:26]

mkwst@ can you take a look for url/ DEPS?  Just realized brettw@ has it set up
that he's busy this week.  Sorry!

[Mike West, 2017-05-04 06:34:38]

//url DEPS LGTM.

[David Trainor- moved to gerrit, 2017-05-04 16:15:48]

https://codereview.chromium.org/2851303003/diff/100001/chrome/browser/downloa...
File chrome/browser/download/download_service_factory.cc (right):

https://codereview.chromium.org/2851303003/diff/100001/chrome/browser/downloa...
chrome/browser/download/download_service_factory.cc:43:
base::TaskPriority::BACKGROUND));
On 2017/05/04 00:19:26, dcheng wrote:
> Based on CLs like https://codereview.chromium.org/2858073002/, I think this
> should use the constexpr overload if possible: for example, I think this can
be
> written
> 
> base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(),
> base::TaskPriority::BACKGROUND})

Done.

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
File components/download/public/clients.h (right):

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
components/download/public/clients.h:17: enum DownloadClient {
On 2017/05/04 00:19:26, dcheng wrote:
> enum class here as well?

Done.

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
File components/download/public/download_params.h (right):

https://codereview.chromium.org/2851303003/diff/100001/components/download/pu...
components/download/public/download_params.h:118: // A unique GUID that
represents this download.  See |base::GenerateGUID()|.
On 2017/05/04 00:19:27, dcheng wrote:
> Unrelated: I kind of wonder if we should just have a distinct type for GUID
> rather than a std::string blob...

Yeah that would be great.  I was surprised we didn't have a type for this.  I
think G/UUID are used pretty extensively in Chrome as a std::string.  It would
be great if we didn't have to DCHECK(IsValidGuid()) or worry about case
everywhere I want to use it!

I have noticed some places have their own concept of one (BluetoothUUID although
that might be specialized, and the Android platform has the concept in Java as
well).

[David Trainor- moved to gerrit, 2017-05-04 16:15:56]

The CQ bit was checked by dtrainor@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 16:16:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[David Trainor- moved to gerrit, 2017-05-04 16:40:21]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-05-04 16:40:35]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-05-04 16:40:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
xingliu@chromium.org, peter@chromium.org, jochen@chromium.org,
rdsmith@chromium.org, dcheng@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2851303003/#ps140001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-05-04 16:41:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 17:34:53]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493916035959550,
"parent_rev": "f240cb6e78c2d189ec6baabde91165b19b27a190", "commit_rev":
"620a4a8839bc2ebc46d923c33ef2435f6b8cb6e1"}

[commit-bot: I haz the power, 2017-05-04 17:35:04]

Description was changed from

==========
Add the download component and initial setup

- Add the BackgroundDownloadService interface and associated files.  Set
up the test hooks and tie it into the proper Chrome and
component_unittest targets.

BUG=717180
==========

to

==========
Add the download component and initial setup

- Add the BackgroundDownloadService interface and associated files.  Set
up the test hooks and tie it into the proper Chrome and
component_unittest targets.

BUG=717180

Review-Url: https://codereview.chromium.org/2851303003
Cr-Commit-Position: refs/heads/master@{#469371}
Committed:
https://chromium.googlesource.com/chromium/src/+/620a4a8839bc2ebc46d923c33ef2...
==========

[commit-bot: I haz the power, 2017-05-04 17:35:07]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/620a4a8839bc2ebc46d923c33ef2...