Adds an observer for the request coordinator.

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 8f257a0490b68315e661c656d14a4f3a4166fcb1..691382c00791e9fecd7a96e6c2bff15569179dc1 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -77,7 +77,9 @@ bool RequestCoordinator::SavePageLater(
     return false;
   }
 
-  // TODO(petewil): We need a robust scheme for allocating new IDs.
+  // TODO(petewil): Use an int64_t random number for this, pass it to the
+  // offliner, and eventually use it as the offline_id - that way the client
+  // uses offline_id everywhere.
   static int64_t id = 0;
 
   // Build a SavePageRequest.
@@ -88,6 +90,7 @@ bool RequestCoordinator::SavePageLater(
   queue_->AddRequest(request,
                      base::Bind(&RequestCoordinator::AddRequestResultCallback,
                                 weak_ptr_factory_.GetWeakPtr()));
+  NotifyAdded(request);
   return true;
 }
 void RequestCoordinator::GetQueuedRequests(
@@ -119,9 +122,10 @@ void RequestCoordinator::GetQueuedRequestsCallback(
 
 void RequestCoordinator::RemoveRequests(
     const std::vector<ClientId>& client_ids) {
+  std::vector<int64_t> placeholder;
   queue_->RemoveRequestsByClientId(
       client_ids, base::Bind(&RequestCoordinator::UpdateMultipleRequestCallback,
-                             weak_ptr_factory_.GetWeakPtr()));
+                             weak_ptr_factory_.GetWeakPtr(), placeholder));
 }
 
 void RequestCoordinator::PauseRequests(
@@ -129,7 +133,7 @@ void RequestCoordinator::PauseRequests(
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::PAUSED,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestCallback,
-                 weak_ptr_factory_.GetWeakPtr()));
+                 weak_ptr_factory_.GetWeakPtr(), request_ids));
 }
 
 void RequestCoordinator::ResumeRequests(
@@ -137,14 +141,13 @@ void RequestCoordinator::ResumeRequests(
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::AVAILABLE,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestCallback,
-                 weak_ptr_factory_.GetWeakPtr()));
+                 weak_ptr_factory_.GetWeakPtr(), request_ids));
   // TODO: Should we also schedule a task, in case there is not one scheduled?
 }
 
 void RequestCoordinator::AddRequestResultCallback(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
-
   // Inform the scheduler that we have an outstanding task..
   scheduler_->Schedule(GetTriggerConditionsForUserRequest());
 }
@@ -164,12 +167,33 @@ void RequestCoordinator::UpdateRequestCallback(
 
 // Called in response to updating multiple requests in the request queue.
 void RequestCoordinator::UpdateMultipleRequestCallback(

[Dmitry Titov, 2016/08/11 03:41:55]

UpdateMultipleRequestCallback and SendNotifyChangeForRequestIdList can be merged
into a single method.

[Pete Williamson, 2016/08/13 03:20:34]

Done.

+    const std::vector<int64_t>& request_ids,
     RequestQueue::UpdateRequestResult result) {
   // If the request succeeded, nothing to do.  If it failed, we can't really do

[Dmitry Titov, 2016/08/11 03:41:55]

This comment seems needs updating, because "nothing to do" is not valid anymore.

[Pete Williamson, 2016/08/13 03:20:34]

Done.

   // much, so just log it.
   if (result != RequestQueue::UpdateRequestResult::SUCCESS) {
     DVLOG(1) << "Failed to update request attempt details. "
              << static_cast<int>(result);
+  } else {
+    SendNotifyChangeForRequestIdList(request_ids);
+  }
+}
+
+// Called in response to updating multiple requests in the request queue.
+void RequestCoordinator::NotifyChangedListCallback(
+    const std::vector<int64_t>& request_ids,
+    RequestQueue::GetRequestsResult result,
+    const std::vector<SavePageRequest>& requests) {
+  std::vector<ClientId> client_ids;
+
+  // For each item in request_ids, find it in requests and send a changed
+  // notification for that request.
+  std::unordered_set<int64_t> changed_ids(request_ids.begin(),
+                                          request_ids.end());
+  for (const SavePageRequest& request : requests) {
+    // If we find a match, send a notification.
+    if (changed_ids.count(request.request_id()) > 0)
+      NotifyChanged(request);
   }
 }
 
@@ -318,19 +342,28 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      updated_request.client_id()));
+    NotifyFailed(updated_request, SavePageStatus::FOREGROUND_CANCELED);
 
-  } else if (status == Offliner::RequestStatus::SAVED ||
-             request.completed_attempt_count() + 1 >=
-                 policy_->GetMaxCompletedTries()) {
-    // Remove the request from the queue if it either succeeded or exceeded the
-    // max number of retries.  The +1 represents the request that just
-    // completed. Since we call MarkAttemptCompleted within the if branches,
-    // the completed_attempt_count has not yet been updated when we are checking
-    // the if condition.
+  } else if (status == Offliner::RequestStatus::SAVED) {
+    // Remove the request from the queue if it succeeded.
+    queue_->RemoveRequest(
+        request.request_id(),
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr(), request.client_id()));
+    // TODO: We don't have an offline ID to return.  Modify the prerenderer
+    // callback to pass it.
+    NotifySucceeded(request, 0);
+  } else if (request.completed_attempt_count() + 1 >=
+             policy_->GetMaxCompletedTries()) {
+    // Remove from the request queue if we exceeeded max retries. The +1
+    // represents the request that just completed. Since we call
+    // MarkAttemptCompleted within the if branches, the completed_attempt_count
+    // has not yet been updated when we are checking the if condition.
     queue_->RemoveRequest(
         request.request_id(),
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr(), request.client_id()));
+    NotifyFailed(request, SavePageStatus::RETRY_COUNT_EXCEEDED);
   } else {
     // If we failed, but are not over the limit, update the request in the
     // queue.
@@ -340,6 +373,7 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      updated_request.client_id()));
+    NotifyChanged(updated_request);
   }
 
   // Determine whether we might try another request in this
@@ -375,6 +409,41 @@ RequestCoordinator::GetTriggerConditionsForUserRequest() {
   return trigger_conditions;
 }
 
+void RequestCoordinator::AddObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+}
+
+void RequestCoordinator::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void RequestCoordinator::NotifyAdded(const SavePageRequest& request) {
+  FOR_EACH_OBSERVER(Observer, observers_, OnAdded(request));
+}
+
+void RequestCoordinator::NotifySucceeded(const SavePageRequest& request,
+                                         int64_t offline_id) {
+  FOR_EACH_OBSERVER(Observer, observers_, OnSucceeded(request, offline_id));
+}
+
+void RequestCoordinator::NotifyFailed(const SavePageRequest& request,
+                                      SavePageStatus status) {
+  FOR_EACH_OBSERVER(Observer, observers_, OnFailed(request, status));
+}
+
+void RequestCoordinator::NotifyChanged(const SavePageRequest& request) {
+  FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
+}
+
+void RequestCoordinator::SendNotifyChangeForRequestIdList(
+    const std::vector<int64_t> request_ids) {
+  // Bind the request_ids onto our callback parameter list, the work will be
+  // done in the callback.
+  queue_->GetRequests(base::Bind(&RequestCoordinator::NotifyChangedListCallback,

[Dmitry Titov, 2016/08/11 03:41:55]

It is unfortunate that we need to go do the SQL query against a database just to
send notifications. In fact, there can be some requests there that are already
deleted from the database by the time we are fetching them. For example, in case
of RemoveRequests - are they first removed fmor DB, and then we try to fetch
them again to notify? Will it even work?

It would be better to keep the list of requests that were actually affected
passed through chain of callbacks and then notify based on that.

[Pete Williamson, 2016/08/13 03:20:34]

Done.

+                                 weak_ptr_factory_.GetWeakPtr(), request_ids));
+}
+
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());