Change database scheme - add state and start tracking

components/offline_pages/background/save_page_request_unittest.cc

Index: components/offline_pages/background/save_page_request_unittest.cc
diff --git a/components/offline_pages/background/save_page_request_unittest.cc b/components/offline_pages/background/save_page_request_unittest.cc
index 1cccc248f5490c009cc11930e3efc4ff0a0325cf..1485257684e6141050f5fd55e667c14753b681e0 100644
--- a/components/offline_pages/background/save_page_request_unittest.cc
+++ b/components/offline_pages/background/save_page_request_unittest.cc
@@ -32,7 +32,7 @@ TEST_F(SavePageRequestTest, CreatePendingReqeust) {
   ASSERT_EQ(creation_time, request.creation_time());
   ASSERT_EQ(creation_time, request.activation_time());
   ASSERT_EQ(base::Time(), request.last_attempt_time());
-  ASSERT_EQ(0, request.attempt_count());
+  ASSERT_EQ(0, request.completed_attempt_count());

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

check for state and started count here as well, please.

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

Done.

 }
 
 TEST_F(SavePageRequestTest, StartAndCompleteRequest) {
@@ -53,7 +53,9 @@ TEST_F(SavePageRequestTest, StartAndCompleteRequest) {
 
   // Attempt time, attempt count and status will though.
   ASSERT_EQ(start_time, request.last_attempt_time());
-  ASSERT_EQ(1, request.attempt_count());
+  ASSERT_EQ(1, request.started_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::PRERENDERING,
+            request.request_state());
 
   request.MarkAttemptCompleted();
 
@@ -65,16 +67,15 @@ TEST_F(SavePageRequestTest, StartAndCompleteRequest) {
   ASSERT_EQ(activation_time, request.activation_time());
 
   // Last attempt time and status are updated.
-  ASSERT_EQ(base::Time(), request.last_attempt_time());
-  ASSERT_EQ(1, request.attempt_count());
+  ASSERT_EQ(base::Time(), request.last_attempt_time()); // TODO: flaky?

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

what is happening? Could you explain it?

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

Sure - Sorry, I meant to investigate this before uploading the change, and
missed the TODO that I left for myself.

My concern here was that if time passes between when we set last_attempt_time
and when we verify that the time might not be what we expect.  It was related to
not being sure if we were using the real clock here or a test clock.  If we used
a real clock, this would be flaky if any of the test takes more than 0 time to
complete.  I wanted to make sure I investigated and that we were using a test
clock.

Looking at it, I don't see a test clock in use, so I'm not sure why this test
always passes unless it always happens very fast and never loses the time slice.

[Pete Williamson, 2016/08/08 18:53:12]

Ah, I see it is comparing to base::Time(), not base::Time::Now().  In that case,
the code is safe, comment removed.

+  ASSERT_EQ(1, request.completed_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());
 }
 
 TEST_F(SavePageRequestTest, StartAndAbortRequest) {
   base::Time creation_time = base::Time::Now();
   SavePageRequest request(kRequestId, kUrl, kClientId, creation_time,
                           kUserRequested);
-  int start_attempt_count = 2;
-  request.set_attempt_count(start_attempt_count);
 
   base::Time start_time = creation_time + base::TimeDelta::FromHours(3);
   request.MarkAttemptStarted(start_time);
@@ -87,7 +88,9 @@ TEST_F(SavePageRequestTest, StartAndAbortRequest) {
 
   // Attempt time and attempt count will though.
   ASSERT_EQ(start_time, request.last_attempt_time());
-  ASSERT_EQ(start_attempt_count + 1, request.attempt_count());
+  ASSERT_EQ(1, request.started_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::PRERENDERING,
+            request.request_state());
 
   request.MarkAttemptAborted();
 
@@ -97,9 +100,10 @@ TEST_F(SavePageRequestTest, StartAndAbortRequest) {
   ASSERT_EQ(kClientId, request.client_id());
   ASSERT_EQ(creation_time, request.creation_time());
 
-  // Last attempt time is updated and attempt count decremented.
+  // Last attempt time is updated and completed attempt count did not rise.
   ASSERT_EQ(base::Time(), request.last_attempt_time());
-  ASSERT_EQ(start_attempt_count, request.attempt_count());
+  ASSERT_EQ(0, request.completed_attempt_count());
+  ASSERT_EQ(SavePageRequest::RequestState::AVAILABLE, request.request_state());

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

This is unrelated to this line, but affects this file:
Can we have a scenario when we MarkAttemptPaused(), please?
* From AVAILABLE (with no attempts), if that is possible
* From AVAILABLE, which has been attempted in the past
* From PRERENDERING, (make sure to check last_attempt_time_).

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

Tests for the state changes are coming in next changelist (I've already coded
it, but I'm waiting for this to land before sending it out for review so we
don't have to review code twice)

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

Acknowledged.

 }
 
 }  // namespace offline_pages