Make the BackgroundFetchJobController a per-job object

content/browser/background_fetch/background_fetch_data_manager.cc

Index: content/browser/background_fetch/background_fetch_data_manager.cc
diff --git a/content/browser/background_fetch/background_fetch_data_manager.cc b/content/browser/background_fetch/background_fetch_data_manager.cc
index 5daeb24a9f1fa97c78b7653dfd09de0cecd76f0e..5289aeb94811598d94129a7a03fd76b328977571 100644
--- a/content/browser/background_fetch/background_fetch_data_manager.cc
+++ b/content/browser/background_fetch/background_fetch_data_manager.cc
@@ -6,7 +6,6 @@
 
 #include "base/memory/ptr_util.h"
 #include "content/browser/background_fetch/background_fetch_context.h"
-#include "content/browser/background_fetch/background_fetch_job_info.h"
 #include "content/browser/background_fetch/background_fetch_request_info.h"
 
 namespace content {
@@ -22,7 +21,7 @@ BackgroundFetchDataManager::~BackgroundFetchDataManager() = default;
 
 BackgroundFetchJobData* BackgroundFetchDataManager::CreateRequest(
     const BackgroundFetchJobInfo& job_info,
-    BackgroundFetchRequestInfos request_infos) {
+    BackgroundFetchRequestInfos& request_infos) {

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

nit: why?

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

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? 

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

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.

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

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.

   JobIdentifier id(job_info.service_worker_registration_id(), job_info.tag());
   // Ensure that this is not a duplicate request.
   if (service_worker_tag_map_.find(id) != service_worker_tag_map_.end()) {
@@ -32,19 +31,32 @@ BackgroundFetchJobData* BackgroundFetchDataManager::CreateRequest(
     // TODO(harkness) Figure out how to return errors like this.
     return nullptr;
   }
-  if (batch_map_.find(job_info.guid()) != batch_map_.end()) {
-    DVLOG(1) << "Job with UID " << job_info.guid() << " already exists.";
-    // TODO(harkness) Figure out how to return errors like this.
-    return nullptr;
-  }
 
   // Add the request to our maps and return a JobData to track the individual
   // files in the request.
   service_worker_tag_map_[id] = job_info.guid();
-  // TODO(harkness): When a job is complete, remove the JobData from the map.
-  batch_map_[job_info.guid()] =
-      base::MakeUnique<BackgroundFetchJobData>(std::move(request_infos));
-  return batch_map_[job_info.guid()].get();
+  WriteJobToStorage(job_info, request_infos);
+  // TODO(harkness): Remove data when the job is complete.
+
+  return new BackgroundFetchJobData(ReadRequestsFromStorage(job_info.guid()));
+}
+
+void BackgroundFetchDataManager::WriteJobToStorage(
+    const BackgroundFetchJobInfo& job_info,
+    BackgroundFetchRequestInfos& request_infos) {
+  // TODO(harkness): Replace these maps with actually writing to storage.
+  // TODO(harkness): Check for job_guid clash.
+  job_map_[job_info.guid()] = job_info;
+  request_map_[job_info.guid()] = std::move(request_infos);
+}
+
+// TODO(harkness): This should be changed to read (and cache) small numbers of
+// the RequestInfos instead of returning all of them.
+BackgroundFetchRequestInfos BackgroundFetchDataManager::ReadRequestsFromStorage(
+    const std::string& job_guid) {
+  BackgroundFetchRequestInfos infos = std::move(request_map_[job_guid]);
+  request_map_.erase(job_guid);

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

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?

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

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

+  return infos;
 }
 
 }  // namespace content