Create the BackgroundFetchBatchManager and initiate a download.

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 06a82d4d31bbe50e3dc5b571bb0faabaf35dac1e..647b43b84f9470e8c64ad45ddb57f677723ba839 100644
--- a/content/browser/background_fetch/background_fetch_data_manager.cc
+++ b/content/browser/background_fetch/background_fetch_data_manager.cc
@@ -5,6 +5,7 @@
 #include "content/browser/background_fetch/background_fetch_data_manager.h"
 
 #include "content/browser/background_fetch/background_fetch_context.h"
+#include "content/browser/background_fetch/batch_request.h"
 #include "content/browser/background_fetch/fetch_request.h"
 
 namespace content {
@@ -16,20 +17,79 @@ BackgroundFetchDataManager::BackgroundFetchDataManager(
   // TODO(harkness) Read from persistent storage and recreate requests.
 }
 
-BackgroundFetchDataManager::~BackgroundFetchDataManager() {}
+BackgroundFetchDataManager::~BackgroundFetchDataManager() {
+  // TODO(harkness): Either require that the BackgroundFetchDataManager is
+  // always destructed after the BatchManager or do ref counting on the
+  // BatchData objects that are allocated.
+  for (auto pair : batch_map_) {

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

note: this'll go away because of std::unique_ptr<> usage, but note that you're
also making a lot of std::string copies (w/ allocations) here because this
copies all the pairs as iteration goes on.

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

Removed it.

+    delete pair.second;
+  }
+}
+
+BackgroundFetchDataManager::BatchData*
+BackgroundFetchDataManager::CreateRequest(
+    const BatchRequest& batch_request,
+    const std::vector<FetchRequest>& fetch_requests) {
+  BatchIdentifier id(batch_request.service_worker_registration_id(),
+                     batch_request.tag());
+  if (service_worker_tag_map_.find(id) != service_worker_tag_map_.end()) {
+    DLOG(ERROR) << "Origin " << batch_request.origin()
+                << " has already created a batch request with tag "
+                << batch_request.tag();
+    // TODO(harkness) Figure out how to return errors like this.
+    return nullptr;
+  }
 
-void BackgroundFetchDataManager::CreateRequest(
-    const FetchRequest& fetch_request) {
-  FetchIdentifier id(fetch_request.service_worker_registration_id(),
-                     fetch_request.tag());
-  if (fetch_map_.find(id) != fetch_map_.end()) {
-    DLOG(ERROR) << "Origin " << fetch_request.origin()
-                << " has already created a fetch request with tag "
-                << fetch_request.tag();
+  if (batch_map_.find(batch_request.guid()) != batch_map_.end()) {
+    DLOG(ERROR) << "Batch with UID " << batch_request.guid()
+                << " already exists.";
     // TODO(harkness) Figure out how to return errors like this.
-    return;
+    return nullptr;
   }
-  fetch_map_[id] = fetch_request;
+
+  service_worker_tag_map_[id] = batch_request.guid();
+  BatchData* batch_data = new BatchData(batch_request, fetch_requests);

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

new -> std::MakeUnique<>

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

Done.

+  batch_map_.insert(std::make_pair(batch_request.guid(), batch_data));
+  return batch_data;
+}
+
+BackgroundFetchDataManager::BatchData::BatchData(
+    const BatchRequest& batch_request,
+    const FetchRequests& fetch_requests)
+    : fetch_requests_(std::move(fetch_requests)) {}

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

How can we *move* a const& vector? Moving invalidates the original..

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

I've removed the const-ness of the vector for now, but you and I should chat
about how you think this should be done for the final version.

+
+BackgroundFetchDataManager::BatchData::~BatchData() {}
+
+BackgroundFetchDataManager::BatchData::BatchData() = default;
+
+// TODO(harkness): Provide more detail about status and where the returned data
+// is now available.
+bool BackgroundFetchDataManager::BatchData::FetchRequestComplete(
+    const std::string& fetch_guid) {
+  DCHECK(fetch_request_index_.find(fetch_guid) != fetch_request_index_.end());
+  fetch_requests_[fetch_request_index_[fetch_guid]].set_complete(true);
+  fetch_request_index_.erase(fetch_guid);

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

This does three lookups in fetch_request_index_. It also doesn't check whether
the index exists in fetch_requests_. What about the following, which perfectly
illustrates my dislike for maps:

    auto index_iter = fetch_request_index_.find(fetch_guid);
    DCHECK(auto_iter != fetch_request_index_.end());

    auto iter = fetch_requests_.find(index_iter.second);
    DCHECK(iter != fetch_requests_.end());

    iter.second.set_complete(true);

    fetch_request_index_.erase(iter_index);

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

Personally, I find the bracket syntax more readable for the map, which for very
rare operations like this is more important to me, but you're right that three
lookups is enough.

I've modified the map calls to use a single iterator lookup, but I've left the
vector calls as bracket lookups. I did add a check that the vector item is the
item we expect.

I'm open to changing these data structures if you feel that they're not a good
fit for my task. I was already considering changing the vector to some form of
list (in which case we wouldn't need the index map) but I was going to wait and
do that when I do the reading/writing from storage.

+  return next_fetch_request_ != fetch_requests_.size();
+}
+
+const FetchRequest&
+BackgroundFetchDataManager::BatchData::GetNextFetchRequest() {
+  DCHECK(next_fetch_request_ != fetch_requests_.size());

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

DCHECK_NE

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

Done.

+
+  const FetchRequest& next_request = fetch_requests_[next_fetch_request_];
+  DCHECK(!next_request.complete());
+  fetch_request_index_[next_request.guid()] = next_fetch_request_++;
+
+  return next_request;
+}
+
+bool BackgroundFetchDataManager::BatchData::IsComplete() const {
+  return ((next_fetch_request_ == fetch_requests_.size()) &&
+          fetch_request_index_.empty());
+}
+
+bool BackgroundFetchDataManager::BatchData::HasRequestsRemaining() const {
+  return next_fetch_request_ != fetch_requests_.size();
 }
 
 }  // namespace content