[Background Fetch] Add a pause state for the job details.

This solves the issue of auto-resuming notifications. OnItemUpdated was
being called after the notification was paused with an offline item
containing an out-of-sync status.

There is also a race condition between pausing the job (OIC) and the
download completing (DS), so new downlaods need to be throttled in case
the job is paused.

Bug: 855856
Change-Id: I537789e7b1607fc4ce69b9cc7593a6499152475c
Reviewed-on: https://chromium-review.googlesource.com/1241157
Commit-Queue: Rayan Kanso <rayankans@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Mugdha Lakhani <nator@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595083}

chrome/browser/background_fetch/background_fetch_browsertest.cc

@@ -144,6 +144,12 @@ class OfflineContentProviderObserver : public OfflineContentProvider::Observer {
     finished_processing_item_callback_ = std::move(callback);
   }
 
+  void set_delegate(BackgroundFetchDelegateImpl* delegate) {
+    delegate_ = delegate;
+  }
+
+  void PauseOnNextUpdate() { pause_ = true; }
+
   // OfflineContentProvider::Observer implementation:
   void OnItemsAdded(
       const OfflineContentProvider::OfflineItemList& items) override {
@@ -155,16 +161,34 @@ class OfflineContentProviderObserver : public OfflineContentProvider::Observer {
   void OnItemUpdated(const OfflineItem& item) override {
     if (item.state != offline_items_collection::OfflineItemState::IN_PROGRESS &&
         item.state != offline_items_collection::OfflineItemState::PENDING &&
-        finished_processing_item_callback_)
+        item.state != offline_items_collection::OfflineItemState::PAUSED &&
+        finished_processing_item_callback_) {
       std::move(finished_processing_item_callback_).Run(item);
+    }
+
+    if (pause_) {
+      if (item.state == offline_items_collection::OfflineItemState::PAUSED) {
+        Resume(item.id);
+        pause_ = false;
+      } else {
+        delegate_->PauseDownload(item.id);
+      }
+    }
+
     latest_item_ = item;
   }
 
   const OfflineItem& latest_item() const { return latest_item_; }
 
  private:
+  void Resume(const ContentId& id) {
+    delegate_->ResumeDownload(id, false /* has_user_gesture */);
+  }
+
   ItemsAddedCallback items_added_callback_;
   FinishedProcessingItemCallback finished_processing_item_callback_;
+  BackgroundFetchDelegateImpl* delegate_ = nullptr;
+  bool pause_ = false;
 
   OfflineItem latest_item_;
 
@@ -206,6 +230,13 @@ class BackgroundFetchBrowserTest : public InProcessBrowserTest {
         ->AddObserver(offline_content_provider_observer_.get());
 
     SetUpBrowser(browser());
+
+    BackgroundFetchDelegateImpl* delegate =
+        static_cast<BackgroundFetchDelegateImpl*>(
+            active_browser_->profile()->GetBackgroundFetchDelegate());
+    DCHECK(delegate);
+
+    offline_content_provider_observer_->set_delegate(delegate);
   }
 
   void SetUpBrowser(Browser* browser) {
@@ -642,6 +673,12 @@ IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
                        "New Failed Title!", base::CompareCase::SENSITIVE));
 }
 
+IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest, FetchCanBePausedAndResumed) {
+  offline_content_provider_observer_->PauseOnNextUpdate();
+  ASSERT_NO_FATAL_FAILURE(RunScriptAndCheckResultingMessage(
+      "RunFetchTillCompletion()", "backgroundfetchsuccess"));
+}
+
 IN_PROC_BROWSER_TEST_F(BackgroundFetchBrowserTest,
                        FetchRejectedWithoutPermission) {
   RevokeDownloadPermission();

chrome/browser/background_fetch/background_fetch_delegate_impl.cc

@@ -73,8 +73,7 @@ BackgroundFetchDelegateImpl::JobDetails::JobDetails(
     std::unique_ptr<content::BackgroundFetchDescription> fetch_description,
     const std::string& provider_namespace,
     bool is_off_the_record)
-    : cancelled(false),
-      offline_item(offline_items_collection::ContentId(
+    : offline_item(offline_items_collection::ContentId(
           provider_namespace,
           fetch_description->job_unique_id)),
       fetch_description(std::move(fetch_description)) {
@@ -125,6 +124,8 @@ void BackgroundFetchDelegateImpl::JobDetails::UpdateOfflineItem() {
     // response was an HTTP error, e.g. 404.
     offline_item.state = OfflineItemState::COMPLETE;
     offline_item.is_openable = true;
+  } else if (paused) {
+    offline_item.state = OfflineItemState::PAUSED;
   } else {
     offline_item.state = OfflineItemState::IN_PROGRESS;
   }
@@ -253,9 +254,6 @@ void BackgroundFetchDelegateImpl::DownloadUrl(
   DCHECK(job_details_map_.count(job_unique_id));
   DCHECK(!download_job_unique_id_map_.count(download_guid));
 
-  JobDetails& job_details = job_details_map_.find(job_unique_id)->second;
-  job_details.current_download_guids.insert(download_guid);
-
   download_job_unique_id_map_.emplace(download_guid, job_unique_id);
 
   download::DownloadParams params;
@@ -269,6 +267,22 @@ void BackgroundFetchDelegateImpl::DownloadUrl(
   params.traffic_annotation =
       net::MutableNetworkTrafficAnnotationTag(traffic_annotation);
 
+  JobDetails& job_details = job_details_map_.find(job_unique_id)->second;
+  if (job_details.paused) {
+    job_details.on_resume =
+        base::BindOnce(&BackgroundFetchDelegateImpl::StartDownload,
+                       GetWeakPtr(), job_unique_id, params);
+  } else {
+    StartDownload(job_unique_id, params);
+  }
+}
+
+void BackgroundFetchDelegateImpl::StartDownload(
+    const std::string& job_unique_id,
+    const download::DownloadParams& params) {
+  DCHECK(job_details_map_.count(job_unique_id));
+  JobDetails& job_details = job_details_map_.find(job_unique_id)->second;
+  job_details.current_download_guids.insert(params.guid);
   GetDownloadService()->StartDownload(params);
 }
 
@@ -520,12 +534,9 @@ void BackgroundFetchDelegateImpl::PauseDownload(
     return;
 
   JobDetails& job_details = job_details_iter->second;
+  job_details.paused = true;
   for (auto& download_guid : job_details.current_download_guids)
     GetDownloadService()->PauseDownload(download_guid);
-
-  // TODO(delphick): Mark overall download job as paused so that future
-  // downloads are not started until resume. (Initially not a worry because only
-  // one download will be scheduled at a time).
 }
 
 void BackgroundFetchDelegateImpl::ResumeDownload(
@@ -536,10 +547,12 @@ void BackgroundFetchDelegateImpl::ResumeDownload(
     return;
 
   JobDetails& job_details = job_details_iter->second;
+  job_details.paused = false;
   for (auto& download_guid : job_details.current_download_guids)
     GetDownloadService()->ResumeDownload(download_guid);
 
-  // TODO(delphick): Start new downloads that weren't started because of pause.
+  if (job_details.on_resume)
+    std::move(job_details.on_resume).Run();
 }
 
 void BackgroundFetchDelegateImpl::GetItemById(

chrome/browser/background_fetch/background_fetch_delegate_impl.h

@@ -128,8 +128,8 @@ class BackgroundFetchDelegateImpl
 
     void UpdateOfflineItem();
 
-    bool cancelled;
-    bool failed;
+    bool cancelled = false;
+    bool paused = false;
 
     // Set of DownloadService GUIDs that are currently downloading. They are
     // added by DownloadUrl and are removed when the download completes, fails
@@ -139,6 +139,8 @@ class BackgroundFetchDelegateImpl
     offline_items_collection::OfflineItem offline_item;
     std::unique_ptr<content::BackgroundFetchDescription> fetch_description;
 
+    base::OnceClosure on_resume;
+
    private:
     // Whether we should report progress of the job in terms of size of
     // downloads or in terms of the number of files being downloaded.
@@ -147,6 +149,10 @@ class BackgroundFetchDelegateImpl
     DISALLOW_COPY_AND_ASSIGN(JobDetails);
   };
 
+  // Starts a download according to |params| belonging to |job_unique_id|.
+  void StartDownload(const std::string& job_unique_id,
+                     const download::DownloadParams& params);
+
   // Updates the OfflineItem that controls the contents of download
   // notifications and notifies any OfflineContentProvider::Observer that was
   // registered with this instance.