Added DownloadItem::Observer to JobController.

Added DownloadItem::Observer to JobController.

Previously the JobController would submit requests to the DownloadManager,
but then it would never do anything when the requests completed/failed.
This CL implements DownloadItem::Observer on JobController, which means
that the JobController will be notified of changes to the download.

Implementation of actions taken based on the notifications is still in
progress, but this provides a basic implementation and a framework for
testing the actions going forward.

BUG=692621
Review-Url: https://codereview.chromium.org/2727253002
Cr-Commit-Position: refs/heads/master@{#456288}
Committed: https://chromium.googlesource.com/chromium/src/+/dfe1dfe7bc684f54d0242262fd98ecde0afb29e0

[harkness, 2017-03-02 13:53:06]

harkness@chromium.org changed reviewers:
+ awdf@chromium.org

[harkness, 2017-03-02 13:53:06]

This has the initial hookup of JobController observing downloads that were
started. Next CL will have passing the final results back to the Context and
into an event dispatcher.

[harkness, 2017-03-07 17:26:22]

harkness@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2017-03-08 14:58:20]

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:47: void
JobComplete(const std::string& job_guid);
Could we instead give the JobController a base::callback that it calls when a
job is complete? That would allow you to make this private, and would help
testing down the road too.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:31:
StopObservations();
We should either call this from the destructor, or from the Shutdown() method,
not both. (Shutdown() should semantically invalidate this class already.)

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:38:
job_data_.reset(nullptr);
nit: drop `nullptr`

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:86: default:
Let's drop `default`, and instead add empty cases for the other DownloadStates.
This will mean that we get compile-time coverage for making sure we handle
everything.

case DownloadItem::DownloadState::IN_PROGRESS:
case DownloadItem::DownloadState::MAX_DOWNLOAD_STATE:
  break;

(I'd personally also s/return/break/ here and above.)

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:117:
DownloadUrlParameters::OnStartedCallback on_started_callback =
nit: I'd use `auto` here. The variable name is clear enough, and the compiler
will warn us if there's a type mismatch. In fact, I'd just inline it in the call
below.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:122: // Send
the fetch request to the DownloadManager.
nit: I don't think this adds any value.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.h (right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:28: : public
content::DownloadItem::Observer {
nit: you are in `content::`

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:72:
std::unordered_map<std::string, std::string> download_guid_map_;
Let's figure out a way to reduce the number of maps. There's a lot of them, all
are very prone to making unintentional copies, and we may be able to find better
data structures to achieve the same goal.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:34:
class MockDownloadItemWithValues : public MockDownloadItem {
WithValues -> WithGuid?

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:49:
params->callback().Run(&download_item_, DOWNLOAD_INTERRUPT_REASON_NONE);
Should this perhaps be a PostTask() to make sure we don't rely on synchronous
behaviour?

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:53:
return &download_item_;
DCHECK on the `guid`?

[harkness, 2017-03-10 11:37:05]

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

[commit-bot: I haz the power, 2017-03-10 11:37:27]

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-03-10 12:19:57]

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

[commit-bot: I haz the power, 2017-03-10 12:19:58]

Dry run: This issue passed the CQ dry run.

[harkness, 2017-03-10 13:33:54]

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_context.h:47: void
JobComplete(const std::string& job_guid);
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> Could we instead give the JobController a base::callback that it calls when a
> job is complete? That would allow you to make this private, and would help
> testing down the road too.

I agree with using callbacks, but this hadn't been hooked up at all, so I've
just removed it for now. When I add it back it will be with callbacky goodness.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:31:
StopObservations();
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> We should either call this from the destructor, or from the Shutdown() method,
> not both. (Shutdown() should semantically invalidate this class already.)

Done.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:38:
job_data_.reset(nullptr);
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> nit: drop `nullptr`

Done.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:86: default:
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> Let's drop `default`, and instead add empty cases for the other
DownloadStates.
> This will mean that we get compile-time coverage for making sure we handle
> everything.
> 
> case DownloadItem::DownloadState::IN_PROGRESS:
> case DownloadItem::DownloadState::MAX_DOWNLOAD_STATE:
>   break;
> 
> (I'd personally also s/return/break/ here and above.)

Done.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:117:
DownloadUrlParameters::OnStartedCallback on_started_callback =
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> nit: I'd use `auto` here. The variable name is clear enough, and the compiler
> will warn us if there's a type mismatch. In fact, I'd just inline it in the
call
> below.

Done.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:122: // Send
the fetch request to the DownloadManager.
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> nit: I don't think this adds any value.

Removed.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.h (right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:28: : public
content::DownloadItem::Observer {
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> nit: you are in `content::`

Done.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.h:72:
std::unordered_map<std::string, std::string> download_guid_map_;
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> Let's figure out a way to reduce the number of maps. There's a lot of them,
all
> are very prone to making unintentional copies, and we may be able to find
better
> data structures to achieve the same goal.

As discussed in person, we're going to switch to using linear time lookup
structures on the assumption that we expect there to be small numbers of them
and the overhead of the more complex structure would swamp any performance gain.

I'll investigate flat_set and converting the maps to that in the future.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:34:
class MockDownloadItemWithValues : public MockDownloadItem {
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> WithValues -> WithGuid?

We'll be adding more state to this item, so I'd rather keep the name generic.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:49:
params->callback().Run(&download_item_, DOWNLOAD_INTERRUPT_REASON_NONE);
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> Should this perhaps be a PostTask() to make sure we don't rely on synchronous
> behaviour?

Good idea.

https://codereview.chromium.org/2727253002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:53:
return &download_item_;
On 2017/03/08 14:58:19, Peter Beverloo wrote:
> DCHECK on the `guid`?

Done.

[Peter Beverloo, 2017-03-10 13:40:52]

lgtm

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:30:
BackgroundFetchJobController::~BackgroundFetchJobController() {}
nit: revert

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:35:
StopObservations();
nit: consider inlining

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:105: if
(item)
When would this be NULL?

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:55:
DCHECK(guid == download_item_.GetGuid());
DCHECK_EQ

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_request_info.cc (right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_request_info.cc:25:
download_guid_(request.download_guid_) {}
nit: complete_ is missing

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_request_info.h (right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_request_info.h:35: }
qq: will we be able to delete one of the GUIDs? This'll get confusing.

[harkness, 2017-03-10 14:21:01]

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:30:
BackgroundFetchJobController::~BackgroundFetchJobController() {}
On 2017/03/10 13:40:52, Peter Beverloo wrote:
> nit: revert

oops, done.

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:35:
StopObservations();
On 2017/03/10 13:40:52, Peter Beverloo wrote:
> nit: consider inlining

Done.

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:105: if
(item)
On 2017/03/10 13:40:52, Peter Beverloo wrote:
> When would this be NULL?

It really shouldn't be unless there's a coding mistake, but there is code in
DownloadManagerImpl that could return nullptr. Would you rather this was a
DCHECK?

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:55:
DCHECK(guid == download_item_.GetGuid());
On 2017/03/10 13:40:52, Peter Beverloo wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_request_info.cc (right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_request_info.cc:25:
download_guid_(request.download_guid_) {}
On 2017/03/10 13:40:52, Peter Beverloo wrote:
> nit: complete_ is missing

Done.

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_request_info.h (right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_request_info.h:35: }
On 2017/03/10 13:40:52, Peter Beverloo wrote:
> qq: will we be able to delete one of the GUIDs? This'll get confusing.

This is an issue with the DownloadManager. It creates a new guid when we send a
request, and all communication with the DownloadManager is via that guid. I've
discussed with David finding a way that we can request a guid rather than being
handed one.

[Peter Beverloo, 2017-03-10 14:25:11]

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:105: if
(item)
On 2017/03/10 14:21:00, harkness wrote:
> On 2017/03/10 13:40:52, Peter Beverloo wrote:
> > When would this be NULL?
> 
> It really shouldn't be unless there's a coding mistake, but there is code in
> DownloadManagerImpl that could return nullptr. Would you rather this was a
> DCHECK?

If it only happens when *we* make a coding mistake (or when the download manager
changes an assumption we make), then a DCHECK is appropriate.

[harkness, 2017-03-11 20:38:49]

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2727253002/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:105: if
(item)
On 2017/03/10 14:25:11, Peter Beverloo wrote:
> On 2017/03/10 14:21:00, harkness wrote:
> > On 2017/03/10 13:40:52, Peter Beverloo wrote:
> > > When would this be NULL?
> > 
> > It really shouldn't be unless there's a coding mistake, but there is code in
> > DownloadManagerImpl that could return nullptr. Would you rather this was a
> > DCHECK?
> 
> If it only happens when *we* make a coding mistake (or when the download
manager
> changes an assumption we make), then a DCHECK is appropriate.

Technically, the download_manager.h interface doesn't document when it could
return null. I've added a TODO to follow up on whether it should be a DCHECK and
sent a mail to David.

[harkness, 2017-03-11 20:39:12]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-11 20:39:12]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2727253002/#ps100001
(title: "Added TODO")

[commit-bot: I haz the power, 2017-03-11 20:39:17]

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-03-11 22:46:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489264752003000,
"parent_rev": "245b456aaef39270b7bdc3d06cfc7d9fedd94913", "commit_rev":
"dfe1dfe7bc684f54d0242262fd98ecde0afb29e0"}

[commit-bot: I haz the power, 2017-03-11 22:47:39]

Description was changed from

==========
Added DownloadItem::Observer to JobController.

Previously the JobController would submit requests to the DownloadManager,
but then it would never do anything when the requests completed/failed.
This CL implements DownloadItem::Observer on JobController, which means
that the JobController will be notified of changes to the download.

Implementation of actions taken based on the notifications is still in
progress, but this provides a basic implementation and a framework for
testing the actions going forward.

BUG=692621
==========

to

==========
Added DownloadItem::Observer to JobController.

Previously the JobController would submit requests to the DownloadManager,
but then it would never do anything when the requests completed/failed.
This CL implements DownloadItem::Observer on JobController, which means
that the JobController will be notified of changes to the download.

Implementation of actions taken based on the notifications is still in
progress, but this provides a basic implementation and a framework for
testing the actions going forward.

BUG=692621

Review-Url: https://codereview.chromium.org/2727253002
Cr-Commit-Position: refs/heads/master@{#456288}
Committed:
https://chromium.googlesource.com/chromium/src/+/dfe1dfe7bc684f54d0242262fd98...
==========

[commit-bot: I haz the power, 2017-03-11 22:47:40]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/dfe1dfe7bc684f54d0242262fd98...