Change database scheme - add state and start tracking

components/offline_pages/background/save_page_request.cc

Index: components/offline_pages/background/save_page_request.cc
diff --git a/components/offline_pages/background/save_page_request.cc b/components/offline_pages/background/save_page_request.cc
index 2ed90bd8d5ae8c8ae102f8fa39e66f4a66a57cff..3d5944427c673ec15fc82231313d8ac6ecffaedb 100644
--- a/components/offline_pages/background/save_page_request.cc
+++ b/components/offline_pages/background/save_page_request.cc
@@ -16,8 +16,10 @@ SavePageRequest::SavePageRequest(int64_t request_id,
       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,
@@ -30,8 +32,10 @@ SavePageRequest::SavePageRequest(int64_t request_id,
       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_),
@@ -39,9 +43,11 @@ SavePageRequest::SavePageRequest(const SavePageRequest& other)
       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() {}
 
@@ -51,19 +57,25 @@ void SavePageRequest::MarkAttemptStarted(const base::Time& start_time) {
   // 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