Reading list downloader retries on recoverable error

ios/chrome/browser/reading_list/reading_list_download_service.cc

Index: ios/chrome/browser/reading_list/reading_list_download_service.cc
diff --git a/ios/chrome/browser/reading_list/reading_list_download_service.cc b/ios/chrome/browser/reading_list/reading_list_download_service.cc
index aacdee7fc76be033206e1f5eed0528629004a73a..f97335be1dced9c84ee33d517cdeef75d4073d7c 100644
--- a/ios/chrome/browser/reading_list/reading_list_download_service.cc
+++ b/ios/chrome/browser/reading_list/reading_list_download_service.cc
@@ -11,13 +11,24 @@
 #include "base/memory/ptr_util.h"
 #include "ios/chrome/browser/reading_list/reading_list_entry.h"
 #include "ios/chrome/browser/reading_list/reading_list_model.h"
+#include "ios/web/public/web_thread.h"
+
+namespace {
+// Number of time the download must fail before the download occurs only in
+// wifi.
+const int kNumberOfFailsBeforeWifiOnly = 5;
+// Number of time the download must fail before we give up trying to download
+// it.
+const int kNumberOfFailsBeforeStop = 7;
+}  // namespace
 
 ReadingListDownloadService::ReadingListDownloadService(
     ReadingListModel* reading_list_model,
     dom_distiller::DomDistillerService* distiller_service,
     PrefService* prefs,
     base::FilePath chrome_profile_path)
-    : reading_list_model_(reading_list_model) {
+    : reading_list_model_(reading_list_model),
+      had_connection_(!net::NetworkChangeNotifier::IsOffline()) {
   DCHECK(reading_list_model);
   url_downloader_ = base::MakeUnique<URLDownloader>(
       distiller_service, prefs, chrome_profile_path,
@@ -25,9 +36,12 @@ ReadingListDownloadService::ReadingListDownloadService(
                  base::Unretained(this)),
       base::Bind(&ReadingListDownloadService::OnDeleteEnd,
                  base::Unretained(this)));
+  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
 }
 
-ReadingListDownloadService::~ReadingListDownloadService() {}
+ReadingListDownloadService::~ReadingListDownloadService() {
+  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+}
 
 void ReadingListDownloadService::Initialize() {
   reading_list_model_->AddObserver(this);
@@ -61,14 +75,14 @@ void ReadingListDownloadService::ReadingListWillAddUnreadEntry(
     const ReadingListModel* model,
     const ReadingListEntry& entry) {
   DCHECK_EQ(reading_list_model_, model);
-  DownloadEntry(entry);
+  ScheduleDownloadEntry(entry);
 }
 
 void ReadingListDownloadService::ReadingListWillAddReadEntry(
     const ReadingListModel* model,
     const ReadingListEntry& entry) {
   DCHECK_EQ(reading_list_model_, model);
-  DownloadEntry(entry);
+  ScheduleDownloadEntry(entry);
 }
 
 void ReadingListDownloadService::DownloadAllEntries() {
@@ -77,21 +91,73 @@ void ReadingListDownloadService::DownloadAllEntries() {
   for (size_t i = 0; i < size; i++) {
     const ReadingListEntry& entry =
         reading_list_model_->GetUnreadEntryAtIndex(i);
-    this->DownloadEntry(entry);
+    this->ScheduleDownloadEntry(entry);
   }
   size = reading_list_model_->read_size();
   for (size_t i = 0; i < size; i++) {
     const ReadingListEntry& entry = reading_list_model_->GetReadEntryAtIndex(i);
-    this->DownloadEntry(entry);
+    this->ScheduleDownloadEntry(entry);
   }
 }
 
+void ReadingListDownloadService::ScheduleDownloadEntry(
+    const ReadingListEntry& entry) {
+  DCHECK(reading_list_model_->loaded());
+  if (entry.DistilledState() == ReadingListEntry::ERROR ||
+      entry.DistilledState() == ReadingListEntry::PROCESSED)
+    return;
+
+  web::WebThread::PostDelayedTask(
+      web::WebThread::UI, FROM_HERE,
+      base::Bind(&ReadingListDownloadService::DownloadEntryFromURL,
+                 base::Unretained(this), entry.URL()),

[gambard, 2016/09/20 15:55:05]

I am not really sure about this, in particular of what happens if the class is
destroyed before the callback fires.

[noyau (Ping after 24h), 2016/09/21 11:08:06]

You're right not to be sure :)

See the use of base::CancelableTaskTracker in URLDownloader for an example of
use.

[gambard, 2016/09/22 13:52:46]

Using GetWeakPtr() should be OK according to
https://cs.chromium.org/chromium/src/docs/callback.md

+      entry.TimeUntilNextTry());
+}
+
+void ReadingListDownloadService::DownloadEntryFromURL(const GURL& url) {
+  auto download_callback =
+      base::Bind(&ReadingListDownloadService::ScheduleDownloadEntry,
+                 base::Unretained(this));
+  reading_list_model_->CallbackEntryURL(url, download_callback);
+}
+
 void ReadingListDownloadService::DownloadEntry(const ReadingListEntry& entry) {
   DCHECK(reading_list_model_->loaded());
-  if (entry.DistilledState() != ReadingListEntry::ERROR) {
+  if (entry.DistilledState() == ReadingListEntry::ERROR ||
+      entry.DistilledState() == ReadingListEntry::PROCESSED)
+    return;
+
+  if (net::NetworkChangeNotifier::IsOffline()) {
+    // There is no connection, save it for download only if we did not exceed
+    // the maximaxum number of tries.
+    if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeWifiOnly)
+      url_to_download_cellular_.push_back(entry.URL());
+    if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeStop)
+      url_to_download_wifi_.push_back(entry.URL());
+    return;
+  }
+
+  // There is a connection.
+  if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeWifiOnly) {
+    // Try to download the page, whatever the connection.
     reading_list_model_->SetEntryDistilledState(entry.URL(),
                                                 ReadingListEntry::PROCESSING);
     url_downloader_->DownloadOfflineURL(entry.URL());
+
+  } else if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeStop) {
+    // Try to download the page only if the connection is wifi.
+    if (net::NetworkChangeNotifier::GetConnectionType() ==
+        net::NetworkChangeNotifier::CONNECTION_WIFI) {
+      // The connection is wifi, download the page.
+      reading_list_model_->SetEntryDistilledState(entry.URL(),
+                                                  ReadingListEntry::PROCESSING);
+      url_downloader_->DownloadOfflineURL(entry.URL());
+
+    } else {
+      // The connection is not wifi, save it for download when the connection
+      // changes to wifi.
+      url_to_download_wifi_.push_back(entry.URL());
+    }
   }
 }
 
@@ -111,9 +177,12 @@ void ReadingListDownloadService::OnDownloadEnd(
        success == URLDownloader::DOWNLOAD_EXISTS) &&
       distilled_url.is_valid()) {
     reading_list_model_->SetEntryDistilledURL(url, distilled_url);
+
   } else if (success == URLDownloader::ERROR_RETRY) {
     reading_list_model_->SetEntryDistilledState(url,
                                                 ReadingListEntry::WILL_RETRY);
+    DownloadEntryFromURL(url);
+
   } else if (success == URLDownloader::ERROR_PERMANENT) {
     reading_list_model_->SetEntryDistilledState(url, ReadingListEntry::ERROR);
   }
@@ -122,3 +191,23 @@ void ReadingListDownloadService::OnDownloadEnd(
 void ReadingListDownloadService::OnDeleteEnd(const GURL& url, bool success) {
   // Nothing to update as this is only called when deleting reading list entries
 }
+
+void ReadingListDownloadService::OnConnectionTypeChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  if (type == net::NetworkChangeNotifier::CONNECTION_NONE) {
+    had_connection_ = false;
+    return;
+  }
+
+  if (!had_connection_) {
+    had_connection_ = true;
+    for (auto& url : url_to_download_cellular_) {
+      DownloadEntryFromURL(url);
+    }
+  }
+  if (type == net::NetworkChangeNotifier::CONNECTION_WIFI) {
+    for (auto& url : url_to_download_wifi_) {
+      DownloadEntryFromURL(url);
+    }
+  }
+}