Make the BackgroundFetchJobController a per-job object

Make the BackgroundFetchJobController a per-job object

In previous code, the BackgroundFetchJobController had information about
all jobs in progress. In this CL, multiple job controllers are instantiated,
and each controller monitors a single job. This allows the controller to
be later used as an Observer for the particular files in its job.

Also changed in this CL is the ownership model for the JobData. Instead of
the DataManager keeping ownership of the JobData, the ownership is passed
to the JobController. The Context then keeps ownership of all created
JobControllers and calls Shutdown on the controllers before the DataManager
is destroyed.

There is still temporary storage for the JobInfo and RequestInfos which will
eventually be written to permanent storage, but these temporary data
structures have been more explicitly called out as temporary.

BUG=692544
Review-Url: https://codereview.chromium.org/2724783002
Cr-Commit-Position: refs/heads/master@{#455869}
Committed: https://chromium.googlesource.com/chromium/src/+/35b3ce0d6309b6cc8ad83084aa0e0365ae1e2b5a

[harkness, 2017-03-01 11:23:12]

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

[harkness, 2017-03-01 11:23:12]

Let's talk about the ownership model changes on a whiteboard before you start
the code review. I think it will make things a lot easier.

[harkness, 2017-03-01 11:23:31]

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

[commit-bot: I haz the power, 2017-03-01 11:23:46]

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-01 12:07:22]

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

[commit-bot: I haz the power, 2017-03-01 12:07:23]

Dry run: This issue passed the CQ dry run.

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

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

[Peter Beverloo, 2017-03-08 14:27:05]

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:32:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
There is nothing that ensures that this gets deleted on the UI thread now -
which will lead to crashes. See platform_notification_context.h for how to make
sure deletion happens on the right thread.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:53: // any status
to the DataManager.
That does mean that such flushing must be synchronous (or they must keep
themselves alive), since the destructor will be called at some point after
running this method.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.h:57: // The Context
is owned by the BrowserContext via the StoragePartition.
Maybe s/The Context/|this|/ to remove ambiguity between "Context" and
"BrowserContext" on the line below, since they align nearly perfectly?

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:24:
BackgroundFetchRequestInfos& request_infos) {
nit: why?

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:58:
request_map_.erase(job_guid);
I don't really know what moving from a map entry means. It feels fragile, and
also isn't the contract that a final method would have - why would reading the
same job_guid twice fail?

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:34: //
ownership of the JobData created.
Then we should return an std::unique_ptr<>.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:40:
DCHECK(job_data_);
nit: this DCHECK should happen in the constructor - that way the rest of the
class can safely assume it's there.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:30:
BackgroundFetchJobData* job_data);
nit: pass as std::unique_ptr<>

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:46:
job_controller_.reset(new BackgroundFetchJobController(
MakeUnique<>

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:87:
BackgroundFetchJobData* job_data = new BackgroundFetchJobData(request_infos);
nit: std::unique_ptr<> here too.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_info.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.cc:27:
service_worker_registration_id_(other.service_worker_registration_id_) {}
request_guids are missing.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.h:27:
BackgroundFetchJobInfo(const BackgroundFetchJobInfo& other);
Can we really not std::unique_ptr<> this already?

[harkness, 2017-03-09 13:33:26]

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:32:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> There is nothing that ensures that this gets deleted on the UI thread now -
> which will lead to crashes. See platform_notification_context.h for how to
make
> sure deletion happens on the right thread.

Updated it to have DeleteOnUI.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:53: // any status
to the DataManager.
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> That does mean that such flushing must be synchronous (or they must keep
> themselves alive), since the destructor will be called at some point after
> running this method.

Yes, it will either mean that or that we do the flushing as best effort or that
we not flush at all and just re-construct on startup. The later might be the
best thing to do, since we're going to need code for that anyway.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.h:57: // The Context
is owned by the BrowserContext via the StoragePartition.
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> Maybe s/The Context/|this|/ to remove ambiguity between "Context" and
> "BrowserContext" on the line below, since they align nearly perfectly?

Good point! Done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:24:
BackgroundFetchRequestInfos& request_infos) {
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> nit: why?

I'm still trying to understand exactly what semantics I should be using when I
use std::move. In particular, I don't want to do extra copies moving the
request_infos around, but it's true that if I bury a std::move several calls
down and pass references, then it's not clear up at this level that the
parameter will no longer be valid after the call. 

I suppose if I do the copy into this function, then pass
std::move(request_infos) to WriteJobToStorage, that makes it clearer?

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:58:
request_map_.erase(job_guid);
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> I don't really know what moving from a map entry means. It feels fragile, and
> also isn't the contract that a final method would have - why would reading the
> same job_guid twice fail?

That's a good point. I'll just return a reference for now.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.h:34: //
ownership of the JobData created.
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> Then we should return an std::unique_ptr<>.

Done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.cc
(right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.cc:40:
DCHECK(job_data_);
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> nit: this DCHECK should happen in the constructor - that way the rest of the
> class can safely assume it's there.

Done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_controller.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller.h:30:
BackgroundFetchJobData* job_data);
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> nit: pass as std::unique_ptr<>

Done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File
content/browser/background_fetch/background_fetch_job_controller_unittest.cc
(right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:46:
job_controller_.reset(new BackgroundFetchJobController(
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> MakeUnique<>

Done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:87:
BackgroundFetchJobData* job_data = new BackgroundFetchJobData(request_infos);
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> nit: std::unique_ptr<> here too.

Done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_info.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.cc:27:
service_worker_registration_id_(other.service_worker_registration_id_) {}
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> request_guids are missing.

Done, and yeah, definitely time for unique_ptrs.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.h:27:
BackgroundFetchJobInfo(const BackgroundFetchJobInfo& other);
On 2017/03/08 14:27:04, Peter Beverloo wrote:
> Can we really not std::unique_ptr<> this already?

I will do it in my next CL. I'd rather not do it in this one because the rebase
would be painful.

[Peter Beverloo, 2017-03-09 15:18:58]

lgtm

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:53: // any status
to the DataManager.
On 2017/03/09 13:33:25, harkness wrote:
> On 2017/03/08 14:27:04, Peter Beverloo wrote:
> > That does mean that such flushing must be synchronous (or they must keep
> > themselves alive), since the destructor will be called at some point after
> > running this method.
> 
> Yes, it will either mean that or that we do the flushing as best effort or
that
> we not flush at all and just re-construct on startup. The later might be the
> best thing to do, since we're going to need code for that anyway.

Not flushing means potential data loss, that's not OK.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:24:
BackgroundFetchRequestInfos& request_infos) {
On 2017/03/09 13:33:25, harkness wrote:
> On 2017/03/08 14:27:04, Peter Beverloo wrote:
> > nit: why?
> 
> I'm still trying to understand exactly what semantics I should be using when I
> use std::move. In particular, I don't want to do extra copies moving the
> request_infos around, but it's true that if I bury a std::move several calls
> down and pass references, then it's not clear up at this level that the
> parameter will no longer be valid after the call. 
> 
> I suppose if I do the copy into this function, then pass
> std::move(request_infos) to WriteJobToStorage, that makes it clearer? 

Please just fall back to const& if you can't wrap your head around moving vs.
copying in this code, for example because it's still too volatile.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.h:27:
BackgroundFetchJobInfo(const BackgroundFetchJobInfo& other);
On 2017/03/09 13:33:25, harkness wrote:
> On 2017/03/08 14:27:04, Peter Beverloo wrote:
> > Can we really not std::unique_ptr<> this already?
> 
> I will do it in my next CL. I'd rather not do it in this one because the
rebase
> would be painful.

Ok.

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

https://codereview.chromium.org/2724783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:34: //
ownership of the JobData created.
nit: You can delete "The caller...JobData created." That's implied by the fact
that it returns a unique_ptr<>.

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

https://codereview.chromium.org/2724783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:28:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Does this live in its entirety on the IO thread now? Then just remove the
DCHECKs and document this in the header.

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

https://codereview.chromium.org/2724783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:87:
BackgroundFetchJobData* job_data = new BackgroundFetchJobData(request_infos);
Store this in a std::unique_ptr<> and std::move() it to InitializeJobController.
That allows you to drop the comment.

[harkness, 2017-03-09 18:35:19]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-09 18:35:20]

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/2724783002/#ps40001
(title: "nits and formatting.")

[harkness, 2017-03-09 18:35:31]

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:53: // any status
to the DataManager.
On 2017/03/09 15:18:57, Peter Beverloo wrote:
> On 2017/03/09 13:33:25, harkness wrote:
> > On 2017/03/08 14:27:04, Peter Beverloo wrote:
> > > That does mean that such flushing must be synchronous (or they must keep
> > > themselves alive), since the destructor will be called at some point after
> > > running this method.
> > 
> > Yes, it will either mean that or that we do the flushing as best effort or
> that
> > we not flush at all and just re-construct on startup. The later might be the
> > best thing to do, since we're going to need code for that anyway.
> 
> Not flushing means potential data loss, that's not OK.

As discussed in person, we should be able to avoid data loss by reconstructing
the state from the DownloadManager, as long as we make sure that we have flushed
before we tell the DownloadManager to remove the download metadata from the
download database.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:24:
BackgroundFetchRequestInfos& request_infos) {
On 2017/03/09 15:18:57, Peter Beverloo wrote:
> On 2017/03/09 13:33:25, harkness wrote:
> > On 2017/03/08 14:27:04, Peter Beverloo wrote:
> > > nit: why?
> > 
> > I'm still trying to understand exactly what semantics I should be using when
I
> > use std::move. In particular, I don't want to do extra copies moving the
> > request_infos around, but it's true that if I bury a std::move several calls
> > down and pass references, then it's not clear up at this level that the
> > parameter will no longer be valid after the call. 
> > 
> > I suppose if I do the copy into this function, then pass
> > std::move(request_infos) to WriteJobToStorage, that makes it clearer? 
> 
> Please just fall back to const& if you can't wrap your head around moving vs.
> copying in this code, for example because it's still too volatile.

As discussed in person, we're in an awkward position where once the storage is
available, the only in-memory copy of the data will live in the JobData, but
while we're in this intermediate state, we would have two in-memory copies of
the data, which is weird. Anyway, I changed it to use references and we'll fix
it when the storage is done.

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_job_info.h (right):

https://codereview.chromium.org/2724783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_job_info.h:27:
BackgroundFetchJobInfo(const BackgroundFetchJobInfo& other);
On 2017/03/09 15:18:57, Peter Beverloo wrote:
> On 2017/03/09 13:33:25, harkness wrote:
> > On 2017/03/08 14:27:04, Peter Beverloo wrote:
> > > Can we really not std::unique_ptr<> this already?
> > 
> > I will do it in my next CL. I'd rather not do it in this one because the
> rebase
> > would be painful.
> 
> Ok.

Acknowledged.

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

https://codereview.chromium.org/2724783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_data_manager.h:34: //
ownership of the JobData created.
On 2017/03/09 15:18:57, Peter Beverloo wrote:
> nit: You can delete "The caller...JobData created." That's implied by the fact
> that it returns a unique_ptr<>.

Done.

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

https://codereview.chromium.org/2724783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller.cc:28:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2017/03/09 15:18:57, Peter Beverloo wrote:
> Does this live in its entirety on the IO thread now? Then just remove the
> DCHECKs and document this in the header.

Done.

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

https://codereview.chromium.org/2724783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_job_controller_unittest.cc:87:
BackgroundFetchJobData* job_data = new BackgroundFetchJobData(request_infos);
On 2017/03/09 15:18:57, Peter Beverloo wrote:
> Store this in a std::unique_ptr<> and std::move() it to
InitializeJobController.
> That allows you to drop the comment.

Done.

[commit-bot: I haz the power, 2017-03-09 18:36:03]

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-09 19:07:25]

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

[commit-bot: I haz the power, 2017-03-09 19:07:26]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[harkness, 2017-03-09 19:56:26]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-09 19:56:26]

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/2724783002/#ps60001
(title: "Fix protection for DeleteOnIOThread template issue.")

[commit-bot: I haz the power, 2017-03-09 19:56:53]

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

[harkness, 2017-03-09 20:08:08]

The CQ bit was unchecked by harkness@chromium.org

[harkness, 2017-03-09 20:14:21]

The CQ bit was checked by harkness@chromium.org

[harkness, 2017-03-09 20:14:22]

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/2724783002/#ps80001
(title: "ACTUALLY fix the compile error")

[commit-bot: I haz the power, 2017-03-09 20:14:58]

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-09 21:36:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489090461711980,
"parent_rev": "bdef30a5605a2ad16b9231033887ce2307156e62", "commit_rev":
"35b3ce0d6309b6cc8ad83084aa0e0365ae1e2b5a"}

[commit-bot: I haz the power, 2017-03-09 21:37:39]

Description was changed from

==========
Make the BackgroundFetchJobController a per-job object

In previous code, the BackgroundFetchJobController had information about
all jobs in progress. In this CL, multiple job controllers are instantiated,
and each controller monitors a single job. This allows the controller to
be later used as an Observer for the particular files in its job.

Also changed in this CL is the ownership model for the JobData. Instead of
the DataManager keeping ownership of the JobData, the ownership is passed
to the JobController. The Context then keeps ownership of all created
JobControllers and calls Shutdown on the controllers before the DataManager
is destroyed.

There is still temporary storage for the JobInfo and RequestInfos which will
eventually be written to permanent storage, but these temporary data
structures have been more explicitly called out as temporary.

BUG=692544
==========

to

==========
Make the BackgroundFetchJobController a per-job object

In previous code, the BackgroundFetchJobController had information about
all jobs in progress. In this CL, multiple job controllers are instantiated,
and each controller monitors a single job. This allows the controller to
be later used as an Observer for the particular files in its job.

Also changed in this CL is the ownership model for the JobData. Instead of
the DataManager keeping ownership of the JobData, the ownership is passed
to the JobController. The Context then keeps ownership of all created
JobControllers and calls Shutdown on the controllers before the DataManager
is destroyed.

There is still temporary storage for the JobInfo and RequestInfos which will
eventually be written to permanent storage, but these temporary data
structures have been more explicitly called out as temporary.

BUG=692544

Review-Url: https://codereview.chromium.org/2724783002
Cr-Commit-Position: refs/heads/master@{#455869}
Committed:
https://chromium.googlesource.com/chromium/src/+/35b3ce0d6309b6cc8ad83084aa0e...
==========

[commit-bot: I haz the power, 2017-03-09 21:37:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/35b3ce0d6309b6cc8ad83084aa0e...