Make the BackgroundFetchJobController a per-job object

content/browser/background_fetch/background_fetch_context.cc

Index: content/browser/background_fetch/background_fetch_context.cc
diff --git a/content/browser/background_fetch/background_fetch_context.cc b/content/browser/background_fetch/background_fetch_context.cc
index 2fde2c68454d1fabccfca1805a53360911a1802a..ce0b6e5e385b3a6d3aaf4a1820b38ceb91e5ff4c 100644
--- a/content/browser/background_fetch/background_fetch_context.cc
+++ b/content/browser/background_fetch/background_fetch_context.cc
@@ -18,8 +18,9 @@ BackgroundFetchContext::BackgroundFetchContext(
     BrowserContext* browser_context,
     StoragePartition* storage_partition,
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
-    : service_worker_context_(service_worker_context),
-      background_fetch_job_controller_(browser_context, storage_partition),
+    : browser_context_(browser_context),
+      storage_partition_(storage_partition),
+      service_worker_context_(service_worker_context),
       background_fetch_data_manager_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // TODO(harkness): BackgroundFetchContext should have
@@ -39,18 +40,38 @@ void BackgroundFetchContext::Init() {
 
 void BackgroundFetchContext::Shutdown() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&BackgroundFetchContext::ShutdownOnIO, this));
+}
+
+void BackgroundFetchContext::ShutdownOnIO() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Call Shutdown on all pending job controllers to give them a chance to flush
+  // any status to the DataManager.

[Peter Beverloo, 2017/03/08 14:27:04]

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.

[harkness, 2017/03/09 13:33:25]

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.

[Peter Beverloo, 2017/03/09 15:18:57]

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

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

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.

+  for (auto& job : job_map_)
+    job.second->Shutdown();
 }
 
 void BackgroundFetchContext::CreateRequest(
     const BackgroundFetchJobInfo& job_info,
     std::vector<BackgroundFetchRequestInfo>& request_infos) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_GE(1U, request_infos.size());
+
   // Inform the data manager about the new download.
   BackgroundFetchJobData* job_data =
       background_fetch_data_manager_.CreateRequest(job_info, request_infos);
+
   // If job_data is null, the DataManager will have logged an error.
-  if (job_data)
-    background_fetch_job_controller_.ProcessJob(job_info.guid(), job_data);
+  if (job_data) {
+    // Create a controller which drives the processing of the job. It will use
+    // the JobData to get information about individual requests for the job.
+    job_map_[job_info.guid()] = base::MakeUnique<BackgroundFetchJobController>(
+        job_info.guid(), browser_context_, storage_partition_, job_data);
+  }
 }
 
 }  // namespace content