Create the BackgroundFetchBatchManager and initiate a download.

content/browser/background_fetch/background_fetch_data_manager.cc

 // 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.
 
 #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 {
 
 BackgroundFetchDataManager::BackgroundFetchDataManager(
     BackgroundFetchContext* background_fetch_context)
     : background_fetch_context_(background_fetch_context) {
   DCHECK(background_fetch_context_);
   // 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;
+  }
+}
 
-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();
+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;
+    return nullptr;
   }
-  fetch_map_[id] = fetch_request;
+
+  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 nullptr;
+  }
+
+  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