Create the BackgroundFetchBatchManager and initiate a download.

content/browser/background_fetch/background_fetch_data_manager.h

Index: content/browser/background_fetch/background_fetch_data_manager.h
diff --git a/content/browser/background_fetch/background_fetch_data_manager.h b/content/browser/background_fetch/background_fetch_data_manager.h
index 3244a5556bbee2716813fd61dae52e967c9a644f..e5f029cb16e94345ec425e5f94265e95a1b2d0ba 100644
--- a/content/browser/background_fetch/background_fetch_data_manager.h
+++ b/content/browser/background_fetch/background_fetch_data_manager.h
@@ -7,6 +7,8 @@
 
 #include <map>
 #include <string>
+#include <unordered_map>
+#include <vector>
 
 #include "base/macros.h"
 #include "content/browser/background_fetch/fetch_request.h"
@@ -16,6 +18,7 @@
 namespace content {
 
 class BackgroundFetchContext;
+class BatchRequest;
 
 // The BackgroundFetchDataManager keeps track of all of the outstanding requests
 // which are in process in the DownloadManager. When Chromium restarts, it is
@@ -27,9 +30,33 @@ class CONTENT_EXPORT BackgroundFetchDataManager {
       BackgroundFetchContext* background_fetch_context);
   ~BackgroundFetchDataManager();
 
+  // The BatchData class provides the interface from the BatchProcessor to the
+  // data storage.

[Peter Beverloo, 2017/02/24 02:05:16]

This comment doesn't really mean anything to me. What does this class do?

Can we avoid having this class by just having methods on the
BackgroundFetchDataManager that take an additional "registration" (or "batch")
argument for indicating what they're interested in using?

[harkness, 2017/02/24 11:47:11]

We could do that, but I like encapsulating the pieces that the BatchManager
talks to as separate from the interface the Context talks to. 

[Peter Beverloo, 2017/02/24 15:43:14]

That sounds like the sort of improvements we could punt as well :). In either
case, an inner class probably isn't the right answer for that - we usually only
use those for internal behaviour, delegates, observers and, in some cases, the
strategy pattern.

+  class BatchData {
+   public:
+    using FetchRequests = std::vector<FetchRequest>;

[Peter Beverloo, 2017/02/24 02:05:16]

This is a source of lots of potential copies too. Please try to avoid making
classes copyable, that's not a good semantic for a class to have.

[harkness, 2017/02/24 11:47:11]

Done.

+    BatchData();

[Peter Beverloo, 2017/02/24 02:05:16]

Please delete the argument-less constructor (it's unused) and add
DISALLOW_COPY_AND_ASSIGN().

[harkness, 2017/02/24 11:47:11]

Done.

+    BatchData(const BatchRequest& batch_request,
+              const FetchRequests& fetch_requests);
+    ~BatchData();
+    bool FetchRequestComplete(const std::string& fetch_uid);

[Peter Beverloo, 2017/02/24 02:05:16]

All of these are public methods, please document them.

[Peter Beverloo, 2017/02/24 02:05:16]

uid -> guid

[harkness, 2017/02/24 11:47:11]

Sorry, I had actually done that, but it looks like the patch set didn't upload.
Done now.

[harkness, 2017/02/24 11:47:11]

Done.

+    const FetchRequest& GetNextFetchRequest();
+    bool IsComplete() const;
+    bool HasRequestsRemaining() const;
+
+   private:
+    FetchRequests fetch_requests_;
+
+    // Map from fetch_uid to index in fetch_requests_. Only currently
+    // outstanding requests should be in this map.
+    std::unordered_map<std::string, int> fetch_request_index_;
+    unsigned long next_fetch_request_ = 0;

[Peter Beverloo, 2017/02/24 02:05:16]

nit: we try to avoid using unsigned types per the Google styleguide. Just use
`int` or `int64_t` if it can exceed 31 bits.

[harkness, 2017/02/24 11:47:11]

Updated to size_t.

+  };
+
   // Called by BackgroundFetchContext when a new request is started, this will
   // store all of the necessary metadata to track the request.
-  void CreateRequest(const FetchRequest& fetch_request);
+  BatchData* CreateRequest(const BatchRequest& batch_request,

[Peter Beverloo, 2017/02/24 02:05:16]

Document the memory model of the returned pointer.

[harkness, 2017/02/24 11:47:11]

Done.

+                           const std::vector<FetchRequest>& fetch_requests);
 
  private:
   // BackgroundFetchContext owns this BackgroundFetchDataManager, so the
@@ -37,8 +64,10 @@ class CONTENT_EXPORT BackgroundFetchDataManager {
   BackgroundFetchContext* background_fetch_context_;
 
   // Map from <sw_registration_id, tag> to the FetchRequest for that tag.
-  using FetchIdentifier = std::pair<int64_t, std::string>;
-  std::map<FetchIdentifier, FetchRequest> fetch_map_;
+  using BatchIdentifier = std::pair<int64_t, std::string>;
+  std::map<BatchIdentifier, std::string> service_worker_tag_map_;
+
+  std::map<std::string, BatchData*> batch_map_;

[Peter Beverloo, 2017/02/24 02:05:16]

This must be std::unique_ptr<BatchData>. Manual memory management is a no-go
these days..

[Peter Beverloo, 2017/02/24 02:05:16]

Document what the index is. Do we depend on ordering? If we don't, use
std::unordered_map<>.

[harkness, 2017/02/24 11:47:11]

Done.

[harkness, 2017/02/24 11:47:11]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(BackgroundFetchDataManager);
 };