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)

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

nit: rename to user_requested, please.

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

Is it OK to do that later?  was_user_requested is in a lot of places.  I'll
start a separate changelist for it.

     : 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;
 }
 
 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