[Offline pages] Introduction of MarkAttemptStartedTask with tests

components/offline_pages/background/mark_attempt_started_task.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/mark_attempt_started_task.h"
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/time/time.h"
+
+namespace offline_pages {
+
+MarkAttemptStartedTask::MarkAttemptStartedTask(
+    RequestQueueStore* store,
+    int64_t request_id,
+    const RequestQueueStore::UpdateCallback& callback)
+    : store_(store),
+      request_id_(request_id),
+      callback_(callback),
+      weak_ptr_factory_(this) {}
+
+MarkAttemptStartedTask::~MarkAttemptStartedTask() {}
+
+void MarkAttemptStartedTask::Run() {
+  ReadRequest();
+}
+
+void MarkAttemptStartedTask::ReadRequest() {
+  std::vector<int64_t> request_ids{request_id_};
+  store_->GetRequestsByIds(
+      request_ids, base::Bind(&MarkAttemptStartedTask::MarkAttemptStarted,
+                              weak_ptr_factory_.GetWeakPtr()));
+}
+
+void MarkAttemptStartedTask::MarkAttemptStarted(
+    std::unique_ptr<UpdateRequestsResult> read_result) {
+  if (read_result->store_state != StoreState::LOADED ||
+      read_result->item_statuses.front().second != ItemActionStatus::SUCCESS ||
+      read_result->updated_items.size() != 1) {
+    CompleteWithResult(std::move(read_result));
+    return;
+  }
+
+  // TODO(petewil): Is there any extra verification we want to do here?

[Pete Williamson, 2016/10/14 17:02:35]

I can't think of any verification for starting a task.  When we finish a task,
it would be good to check that the task actually got started, and DCHECK if it
wasn't.

[fgorski, 2016/10/14 20:37:19]

This is up to the caller to check/decide/report. We return that information as a
valid result. DCHECK would be more appropriate if the function was called with
incorrect parameters. In the case stated by you nothing like that happens.
Simply some thing failed and informs the caller about that.

Removing a TODO

+  read_result->updated_items[0].MarkAttemptStarted(base::Time::Now());

[Pete Williamson, 2016/10/14 17:02:35]

I'm a bit surprised that updated_items isn't const, and we aren't making a copy
(I'm fine with it, just a bit surprised).

[fgorski, 2016/10/14 20:37:18]

We are given our own result, that we are consuming here.
There is nothing that needs to be const about it, and nothing else will use it.
At the end of this method the result will be deleted.

+  store_->UpdateRequests(read_result->updated_items,
+                         base::Bind(&MarkAttemptStartedTask::CompleteWithResult,
+                                    weak_ptr_factory_.GetWeakPtr()));
+}
+
+void MarkAttemptStartedTask::CompleteWithResult(
+    std::unique_ptr<UpdateRequestsResult> result) {
+  callback_.Run(std::move(result));
+  TaskComplete();
+}
+
+}  // namespace offline_pages