Create the BackgroundFetchBatchManager and initiate a download.

content/browser/background_fetch/background_fetch_data_manager.h

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_BACKGROUND_FETCH_BACKGROUND_FETCH_DATA_MANAGER_H_
 #define CONTENT_BROWSER_BACKGROUND_FETCH_BACKGROUND_FETCH_DATA_MANAGER_H_
 
 #include <map>
 #include <string>
+#include <unordered_map>
+#include <vector>
 
 #include "base/macros.h"
 #include "content/browser/background_fetch/fetch_request.h"
 #include "content/common/content_export.h"
 #include "url/origin.h"
 
 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
 // responsibile for reconnecting all the in progress downloads with an observer
 // which will keep the metadata up to date.
 class CONTENT_EXPORT BackgroundFetchDataManager {
  public:
   explicit 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
   // DataManager is guaranteed to be destructed before the Context.
   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);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_BACKGROUND_FETCH_BACKGROUND_FETCH_DATA_MANAGER_H_