[Offline pages] Adding capability to query a subset of requests based on IDs

components/offline_pages/background/request_queue_in_memory_store.cc

 // Copyright 2016 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 "components/offline_pages/background/request_queue_in_memory_store.h"
 
+#include <unordered_set>
+
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/offline_pages/background/save_page_request.h"
 
 namespace offline_pages {
 
 RequestQueueInMemoryStore::RequestQueueInMemoryStore() {}
 
 RequestQueueInMemoryStore::~RequestQueueInMemoryStore() {}
 
 void RequestQueueInMemoryStore::GetRequests(
     const GetRequestsCallback& callback) {
   std::vector<std::unique_ptr<SavePageRequest>> result_requests;
   for (const auto& id_request_pair : requests_) {
     std::unique_ptr<SavePageRequest> request(
         new SavePageRequest(id_request_pair.second));
     result_requests.push_back(std::move(request));
   }
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::Bind(callback, true, base::Passed(std::move(result_requests))));
 }
 
+void RequestQueueInMemoryStore::GetRequestsByIds(
+    const std::vector<int64_t>& request_ids,
+    const UpdateCallback& callback) {

[Pete Williamson, 2016/10/11 22:16:21]

I think the name of the callback should have Get in it instead of Update. 
Alternatively, UpdateCallback could be renamed to something more generic.

[fgorski, 2016/10/12 20:51:29]

Yes, but I'd prefer to do that separately.

+  std::unique_ptr<UpdateRequestsResult> result(
+      new UpdateRequestsResult(state()));
+
+  ItemActionStatus status;
+  std::unordered_set<int64_t> processed_ids;
+  for (const auto& request_id : request_ids) {
+    if (!processed_ids.insert(request_id).second)

[Pete Williamson, 2016/10/11 22:16:21]

Why do we keep a list of processed ids?  Maybe add the same comment you had in
the SQL file.

[fgorski, 2016/10/12 20:51:29]

Defensive programming for cases where someone specified the same ID multiple
times. This code prevents that.

This is explicitly commented on SQL side:
// Make sure not to include the same request multiple times, preserving the
// order of non-duplicated IDs in the result.

[Pete Williamson, 2016/10/12 22:23:36]

Right, I understand why, I just think we should add the comment here too.

[fgorski, 2016/10/12 23:05:30]

Done. Sorry, I misread your comment.

+      continue;
+    RequestsMap::iterator iter = requests_.find(request_id);
+    if (iter != requests_.end()) {
+      status = ItemActionStatus::SUCCESS;
+      result->updated_items.push_back(iter->second);
+    } else {
+      status = ItemActionStatus::NOT_FOUND;
+    }
+    result->item_statuses.push_back(std::make_pair(request_id, status));
+  }
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, base::Passed(&result)));
+}
+
 void RequestQueueInMemoryStore::AddRequest(const SavePageRequest& request,
                                            const AddCallback& callback) {
   RequestsMap::iterator iter = requests_.find(request.request_id());
   ItemActionStatus status;
   if (iter == requests_.end()) {
     requests_.insert(iter, std::make_pair(request.request_id(), request));
     status = ItemActionStatus::SUCCESS;
   } else {
     status = ItemActionStatus::ALREADY_EXISTS;
   }
@@ skipping 55 matching lines @@
   requests_.clear();
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                 base::Bind(callback, true));
 }
 
 StoreState RequestQueueInMemoryStore::state() const {
   return StoreState::LOADED;
 }
 
 }  // namespace offline_pages