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..fefcebeb043ea3d178a0bc1155e5a0d80d000845 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,28 @@ 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;
 }
 
 void SavePageRequest::MarkAttemptCompleted() {
   last_attempt_time_ = base::Time();
+  ++completed_attempt_count_;
+  state_ = RequestState::AVAILABLE;
 }
 
 void SavePageRequest::MarkAttemptAborted() {
-  DCHECK_GT(attempt_count_, 0);
+  DCHECK_GT(started_attempt_count_, 0);
+  // We intentinally  do not increment the  completed_attempt_count_, since this

[fgorski, 2016/08/08 17:48:05]

typo in: intentinally 
extra space after "intentionally" and "the" in this line, and after "to" in the
next.

[Pete Williamson, 2016/08/08 18:44:29]

Done.

+  // was killed before it had a chance to  run to completion so we could use the

[fgorski, 2016/08/08 17:48:05]

how about: chance to complete. Phone was used for other memory/CPU intensive
task? 

[chili, 2016/08/08 18:17:01]

even shorter: "since this was killed before it completed"?

[Pete Williamson, 2016/08/08 18:44:29]

Done.

[Pete Williamson, 2016/08/08 18:44:29]

Done.

+  // phone or browser for other things.
   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_;
+  state_ = RequestState::AVAILABLE;
+}
+
+void SavePageRequest::MarkAttemptPaused() {
+  DCHECK_GT(started_attempt_count_, 0);

[fgorski, 2016/08/08 17:48:05]

Would it not be possible to pause a task before it gets a chance to run? ... I
think notification appears earlier than we actually run the background loading.

[Pete Williamson, 2016/08/08 18:44:28]

Good catch, removed DCHECK

+  state_ = RequestState::PAUSED;

[fgorski, 2016/08/08 17:48:05]

can we mark last_attempt_time_ = base::Time(), since we are coming out of
pending when paused in cases this happens.

Alternative solution would be to never clear last_attempt_pending_ and use it
for sorting the requests in the picker.

[Pete Williamson, 2016/08/08 18:44:28]

In the normal case the request has not been attempted, so I left
last_attempt_time alone.  If it has been attempted, the last_attempt_time was
set when we attempted it, so we also leave it alone.

Now that we have state, we don't need to rely on the last attempt time being
cleared to show that the request is not in progress.

[fgorski, 2016/08/08 22:35:33]

Then why clearing it in cases where the attempt was completed or aborted?
In my mind either way is good (clean it or not), as long as code paths are
consistent.

[Pete Williamson, 2016/08/08 22:56:56]

OK, I chose to stop clearing it and rely solely on state.

 }
 
 }  // namespace offline_pages