[Offline pages] Eliminating duplicate notifications, by fixing request queue store

* Successfully removed requests where returned in result as duplicates
* above was fixed with tests updated

BUG=641380
R=dewittj@chromium.org

Review-Url: https://codereview.chromium.org/2283813003
Cr-Commit-Position: refs/heads/master@{#414790}

components/offline_pages/background/request_queue_store_sql.cc

@@ -114,12 +114,18 @@ void BuildFailedResultList(const std::vector<int64_t>& request_ids,
         request_id, RequestQueue::UpdateRequestResult::STORE_FAILURE));
 }
 
-bool DeleteRequestById(sql::Connection* db, int64_t request_id) {
+RequestQueue::UpdateRequestResult DeleteRequestById(sql::Connection* db,
+                                                    int64_t request_id) {
   const char kSql[] =
       "DELETE FROM " REQUEST_QUEUE_TABLE_NAME " WHERE request_id=?";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, request_id);
-  return statement.Run();
+  if (!statement.Run())
+    return RequestQueue::UpdateRequestResult::STORE_FAILURE;
+  else if (db->GetLastChangeCount() == 0)
+    return RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+  else
+    return RequestQueue::UpdateRequestResult::SUCCESS;
 }
 
 bool ChangeRequestState(sql::Connection* db,
@@ -150,15 +156,11 @@ bool DeleteRequestsByIds(sql::Connection* db,
   // the queue of requests that got deleted.
   for (int64_t request_id : request_ids) {
     SavePageRequest request = GetOneRequest(db, request_id);
-    RequestQueue::UpdateRequestResult result;
-    if (DeleteRequestById(db, request_id)) {
-      result = RequestQueue::UpdateRequestResult::SUCCESS;
-      requests.push_back(request);
-    } else {
-      result = RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
-    }
+    RequestQueue::UpdateRequestResult result =
+        DeleteRequestById(db, request_id);
     results.push_back(std::make_pair(request_id, result));
-    requests.push_back(request);
+    if (result == RequestQueue::UpdateRequestResult::SUCCESS)
+      requests.push_back(request);
   }
 
   if (!transaction.Commit()) {

components/offline_pages/background/request_queue_store_unittest.cc

@@ -305,6 +305,7 @@ TYPED_TEST(RequestQueueStoreTest, RemoveRequests) {
             this->last_remove_results().at(0).second);
   ASSERT_EQ(RequestQueue::UpdateRequestResult::SUCCESS,
             this->last_remove_results().at(1).second);
+  ASSERT_EQ(2UL, this->last_requests().size());
   ASSERT_EQ(kRequestId, this->last_requests().at(0).request_id());
   this->ClearResults();
 
@@ -322,13 +323,13 @@ TYPED_TEST(RequestQueueStoreTest, RemoveRequests) {
   ASSERT_EQ(LastResult::kNone, this->last_result());
   this->PumpLoop();
   ASSERT_EQ(2ul, this->last_remove_results().size());
-  // Since the SQL statement returns true on a delete of an item that isn't
-  // present, SQL is returning SUCCESS, but the memory is returning
-  // REQUEST_DOES_NOT_EXIST, so we just check that the result is not failure.
-  ASSERT_NE(RequestQueue::UpdateRequestResult::STORE_FAILURE,
+  // When requests are missing, we expect the results to say so, but since they
+  // are missing, no requests should have been returned.
+  ASSERT_EQ(RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST,
             this->last_remove_results().at(0).second);
-  ASSERT_NE(RequestQueue::UpdateRequestResult::STORE_FAILURE,
+  ASSERT_EQ(RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST,
             this->last_remove_results().at(1).second);
+  ASSERT_EQ(0UL, this->last_requests().size());
 }
 
 TYPED_TEST(RequestQueueStoreTest, PauseAndResumeRequest) {