Change database scheme - add state and start tracking

components/offline_pages/background/save_page_request.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/save_page_request.h"
 
 namespace offline_pages {
 
 SavePageRequest::SavePageRequest(int64_t request_id,
                                  const GURL& url,
                                  const ClientId& client_id,
                                  const base::Time& creation_time,
                                  const bool was_user_requested)
     : request_id_(request_id),
       url_(url),
       client_id_(client_id),
       creation_time_(creation_time),
       activation_time_(creation_time),
-      attempt_count_(0),
-      user_requested_(was_user_requested) {}
+      started_attempt_count_(0),
+      completed_attempt_count_(0),
+      user_requested_(was_user_requested),
+      state_(RequestState::AVAILABLE) {}
 
 SavePageRequest::SavePageRequest(int64_t request_id,
                                  const GURL& url,
                                  const ClientId& client_id,
                                  const base::Time& creation_time,
                                  const base::Time& activation_time,
                                  const bool user_requested)
     : request_id_(request_id),
       url_(url),
       client_id_(client_id),
       creation_time_(creation_time),
       activation_time_(activation_time),
-      attempt_count_(0),
-      user_requested_(user_requested) {}
+      started_attempt_count_(0),
+      completed_attempt_count_(0),
+      user_requested_(user_requested),
+      state_(RequestState::AVAILABLE) {}
 
 SavePageRequest::SavePageRequest(const SavePageRequest& other)
     : request_id_(other.request_id_),
       url_(other.url_),
       client_id_(other.client_id_),
       creation_time_(other.creation_time_),
       activation_time_(other.activation_time_),
-      attempt_count_(other.attempt_count_),
+      started_attempt_count_(other.started_attempt_count_),
+      completed_attempt_count_(other.completed_attempt_count_),
       last_attempt_time_(other.last_attempt_time_),
-      user_requested_(other.user_requested_) {}
+      user_requested_(other.user_requested_),
+      state_(other.state_) {}
 
 SavePageRequest::~SavePageRequest() {}
 
 void SavePageRequest::MarkAttemptStarted(const base::Time& start_time) {
   DCHECK_LE(activation_time_, start_time);
   // TODO(fgorski): As part of introducing policy in GetStatus, we can make a
   // check here to ensure we only start tasks in status pending, and bail out in
   // other cases.
   last_attempt_time_ = start_time;
-  ++attempt_count_;
+  ++started_attempt_count_;
+  state_ = RequestState::PRERENDERING;

[dougarnett, 2016/08/19 20:21:26]

I don't understand why this internally sets a specifically named PRERENDERING
state (vs. some more generic IN_PROGRESS or LOADING). This method doesn't know
the implementation for loading so it is guessing. I think it should either be a
generally named state or else the specific state would need to be passed in. 

[Pete Williamson, 2016/08/22 16:33:29]

The thinking was that we might someday have other states such as SAVING_PAGE, or
other types of pre-renderer.  So, we can pass in the state to the
MarkAttemptStarted method in a future change, or we could wait until we
introduce more states to make the change.

 }
 
 void SavePageRequest::MarkAttemptCompleted() {
-  last_attempt_time_ = base::Time();
+  ++completed_attempt_count_;
+  state_ = RequestState::AVAILABLE;
 }
 
 void SavePageRequest::MarkAttemptAborted() {
-  DCHECK_GT(attempt_count_, 0);
-  last_attempt_time_ = base::Time();
-  // TODO(dougarnett): Would be safer if we had two persisted counters
-  // (attempts_started and attempts_completed) rather just one with decrement.
-  --attempt_count_;
+  DCHECK_GT(started_attempt_count_, 0);
+  // We intentinally do not increment the completed_attempt_count_, since this
+  // was killed before it completed, so we could use the phone or browser for
+  // other things.
+  state_ = RequestState::AVAILABLE;
+}
+
+void SavePageRequest::MarkAttemptPaused() {
+  state_ = RequestState::PAUSED;
 }
 
 }  // namespace offline_pages