Fix auto-fetch hanging notification

The underlying problem was that RequestCoordinator was reporting that a request
had been added, but in fact it was rejected. This led auto-fetch code to believe
there was an outstanding request remaining.

Bug: 1003053
Change-Id: I5f1554481405ca63a050c119ef624053e58f3acf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1801318Reviewed-by: Peter Williamson <petewil@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697293}

components/offline_pages/core/background/request_coordinator.cc

@@ -545,22 +545,26 @@ void RequestCoordinator::AddRequestResultCallback(
     RequestAvailability availability,
     AddRequestResult result,
     const SavePageRequest& request) {
-  NotifyAdded(request);
-  // Inform the scheduler that we have an outstanding task.
-  scheduler_->Schedule(GetTriggerConditions(kUserRequest));
-
-  if (availability == RequestAvailability::DISABLED_FOR_OFFLINER) {
-    // Mark attempt started (presuming it is disabled for background offliner
-    // because foreground offlining is happening).
-    queue_->MarkAttemptStarted(
-        request.request_id(),
-        base::BindOnce(&RequestCoordinator::MarkAttemptDone,
-                       weak_ptr_factory_.GetWeakPtr(), request.request_id(),
-                       request.client_id().name_space));
-  } else if (request.user_requested()) {
-    StartImmediatelyIfConnected();
+  if (result == AddRequestResult::SUCCESS) {
+    NotifyAdded(request);
+    // Inform the scheduler that we have an outstanding task.
+    scheduler_->Schedule(GetTriggerConditions(kUserRequest));
+
+    if (availability == RequestAvailability::DISABLED_FOR_OFFLINER) {
+      // Mark attempt started (presuming it is disabled for background offliner
+      // because foreground offlining is happening).
+      queue_->MarkAttemptStarted(
+          request.request_id(),
+          base::BindOnce(&RequestCoordinator::MarkAttemptDone,
+                         weak_ptr_factory_.GetWeakPtr(), request.request_id(),
+                         request.client_id().name_space));
+    } else if (request.user_requested()) {
+      StartImmediatelyIfConnected();
+    }
+  } else {
+    event_logger_.RecordAddRequestFailed(request.client_id().name_space,
+                                         result);
   }
-
   std::move(save_page_later_callback).Run(result);
 }
 

components/offline_pages/core/background/request_coordinator_event_logger.cc

@@ -4,6 +4,10 @@
 
 #include "components/offline_pages/core/background/request_coordinator_event_logger.h"
 
+#include <string>
+#include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
+
 namespace offline_pages {
 
 namespace {
@@ -33,7 +37,7 @@ static std::string BackgroundSavePageResultToString(
       return "DOWNLOAD_THROTTLED";
     default:
       NOTREACHED();
-      return std::to_string(static_cast<int>(result));
+      return base::NumberToString(static_cast<int>(result));
   }
 }
 
@@ -47,7 +51,7 @@ static std::string UpdateRequestResultToString(UpdateRequestResult result) {
       return "REQUEST_DOES_NOT_EXIST";
     default:
       NOTREACHED();
-      return std::to_string(static_cast<int>(result));
+      return base::NumberToString(static_cast<int>(result));
   }
 }
 
@@ -57,7 +61,7 @@ void RequestCoordinatorEventLogger::RecordOfflinerResult(
     const std::string& name_space,
     Offliner::RequestStatus new_status,
     int64_t request_id) {
-  std::string request_id_str = std::to_string(request_id);
+  std::string request_id_str = base::NumberToString(request_id);
   RecordActivity("Background save attempt for " + name_space + ":" +
                  request_id_str + " - " +
                  Offliner::RequestStatusToString(new_status));
@@ -67,7 +71,7 @@ void RequestCoordinatorEventLogger::RecordDroppedSavePageRequest(
     const std::string& name_space,
     RequestNotifier::BackgroundSavePageResult result,
     int64_t request_id) {
-  std::string request_id_str = std::to_string(request_id);
+  std::string request_id_str = base::NumberToString(request_id);
   RecordActivity("Background save request removed " + name_space + ":" +
                  request_id_str + " - " +
                  BackgroundSavePageResultToString(result));
@@ -80,4 +84,12 @@ void RequestCoordinatorEventLogger::RecordUpdateRequestFailed(
                  UpdateRequestResultToString(result));
 }
 
+void RequestCoordinatorEventLogger::RecordAddRequestFailed(
+    const std::string& name_space,
+    AddRequestResult result) {
+  RecordActivity(
+      base::StrCat({"Add request failed for ", name_space, " - code ",
+                    base::NumberToString(static_cast<int>(result))}));
+}
+
 }  // namespace offline_pages

components/offline_pages/core/background/request_coordinator_event_logger.h

@@ -31,6 +31,9 @@ class RequestCoordinatorEventLogger : public OfflineEventLogger {
 
   void RecordUpdateRequestFailed(const std::string& name_space,
                                  UpdateRequestResult result);
+
+  void RecordAddRequestFailed(const std::string& name_space,
+                              AddRequestResult result);
 };
 
 }  // namespace offline_pages

components/offline_pages/core/background/request_coordinator_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/logging.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/system/sys_info.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
@@ -80,17 +81,11 @@ class BoolCallbackResult {
 
 class ObserverStub : public RequestCoordinator::Observer {
  public:
-  ObserverStub()
-      : added_called_(false),
-        completed_called_(false),
-        changed_called_(false),
-        network_progress_called_(false),
-        network_progress_bytes_(0),
-        last_status_(RequestCoordinator::BackgroundSavePageResult::SUCCESS),
-        state_(SavePageRequest::RequestState::OFFLINING) {}
+  ObserverStub() { Clear(); }
 
   void Clear() {
     added_called_ = false;
+    added_call_count_ = 0;
     completed_called_ = false;
     changed_called_ = false;
     network_progress_called_ = false;
@@ -101,6 +96,7 @@ class ObserverStub : public RequestCoordinator::Observer {
 
   void OnAdded(const SavePageRequest& request) override {
     added_called_ = true;
+    ++added_call_count_;
     state_ = request.request_state();
     pending_state_ = request.pending_state();
   }
@@ -125,6 +121,7 @@ class ObserverStub : public RequestCoordinator::Observer {
   }
 
   bool added_called() { return added_called_; }
+  int added_call_count() const { return added_call_count_; }
   bool completed_called() { return completed_called_; }
   bool changed_called() { return changed_called_; }
   bool network_progress_called() { return network_progress_called_; }
@@ -137,6 +134,7 @@ class ObserverStub : public RequestCoordinator::Observer {
 
  private:
   bool added_called_;
+  int added_call_count_;
   bool completed_called_;
   bool changed_called_;
   bool network_progress_called_;
@@ -1879,4 +1877,28 @@ TEST_F(RequestCoordinatorTest,
   EXPECT_EQ(PendingState::PENDING_ANOTHER_DOWNLOAD, observer().pending_state());
 }
 
+TEST_F(RequestCoordinatorTest, SavePageLaterRejectedDuplicateUrl) {
+  // Request the same URL twice using the 'disallow_duplicate_requests' option,
+  // and verify the second request is rejected.
+  EnableOfflinerCallback(false);
+
+  RequestCoordinator::SavePageLaterParams params;
+  params.url = kUrl1;
+  params.client_id = kClientId1;
+  params.add_options.disallow_duplicate_requests = true;
+  std::vector<AddRequestResult> results;
+  auto callback = base::BindLambdaForTesting(
+      [&](AddRequestResult result) { results.push_back(result); });
+
+  EXPECT_NE(0, coordinator()->SavePageLater(params, callback));
+  EXPECT_NE(0, coordinator()->SavePageLater(params, callback));
+  PumpLoop();
+
+  // Only one is added.
+  EXPECT_EQ(1, observer().added_call_count());
+  EXPECT_EQ(std::vector<AddRequestResult>(
+                {AddRequestResult::SUCCESS, AddRequestResult::DUPLICATE_URL}),
+            results);
+}
+
 }  // namespace offline_pages