[Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API.

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 63b970f2e0fa09268472f0aa855ef242effb7118..b7e4aaceeddb57aa467ed95e6a0b39058da88b19 100644
--- a/components/offline_pages/background/save_page_request_unittest.cc
+++ b/components/offline_pages/background/save_page_request_unittest.cc
@@ -92,10 +92,46 @@ TEST_F(SavePageRequestTest, StartAndCompleteRequest) {
   ASSERT_EQ(creation_time, request.creation_time());
   ASSERT_EQ(activation_time, request.activation_time());
 
-  // Last attempt time, status and attempt count are updated.
+  // Last attempt time and status are updated.

[Pete Williamson, 2016/08/03 20:44:56]

Should we have kept attempt count instead of status?

[dougarnett, 2016/08/03 23:33:51]

Not sure what you mean. Also, my change here is only making comment more
accurate for existing code.

[Pete Williamson, 2016/08/04 00:17:14]

Right, the comment says "last attempt time and status"  I think maybe it should
say "last attempt time and count"

[dougarnett, 2016/08/04 00:23:35]

but the count is not updated here - still 1 as in line 82

[Pete Williamson, 2016/08/04 00:27:16]

Ah, nevermind, I saw the line checking the count, and thought that it had been
updated.

   ASSERT_EQ(base::Time(), request.last_attempt_time());
   ASSERT_EQ(1, request.attempt_count());
   ASSERT_EQ(SavePageRequest::Status::PENDING, request.GetStatus(start_time));
 }
 
-}  // offline_pages
+TEST_F(SavePageRequestTest, StartAndAbortRequest) {
+  base::Time creation_time = base::Time::Now();
+  base::Time activation_time = creation_time + base::TimeDelta::FromHours(6);

[Pete Williamson, 2016/08/03 20:44:56]

Why are we setting activation_time to anything other than creation_time?  It
seems orthogonal to this particular test.

[dougarnett, 2016/08/03 23:33:51]

Done.

+  SavePageRequest request(kRequestId, kUrl, kClientId, creation_time,
+                          activation_time, kUserRequested);
+  int start_attempt_count = 2;
+  request.set_attempt_count(start_attempt_count);
+
+  base::Time start_time = activation_time + base::TimeDelta::FromHours(3);
+  request.MarkAttemptStarted(start_time);
+
+  // Most things don't change about the request.
+  ASSERT_EQ(kRequestId, request.request_id());
+  ASSERT_EQ(kUrl, request.url());
+  ASSERT_EQ(kClientId, request.client_id());
+  ASSERT_EQ(creation_time, request.creation_time());
+  ASSERT_EQ(activation_time, request.activation_time());
+
+  // Attempt time and attempt count will though.
+  ASSERT_EQ(start_time, request.last_attempt_time());
+  ASSERT_EQ(start_attempt_count + 1, request.attempt_count());
+
+  request.MarkAttemptAborted();
+
+  // Again, most things don't change about the request.
+  ASSERT_EQ(kRequestId, request.request_id());
+  ASSERT_EQ(kUrl, request.url());
+  ASSERT_EQ(kClientId, request.client_id());
+  ASSERT_EQ(creation_time, request.creation_time());
+  ASSERT_EQ(activation_time, request.activation_time());
+
+  // Last attempt time is updated and attempt count decremented.
+  ASSERT_EQ(base::Time(), request.last_attempt_time());
+  ASSERT_EQ(start_attempt_count, request.attempt_count());
+}
+
+}  // namespace offline_pages