Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771

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

ios/chrome/browser/reading_list/reading_list_download_service.cc

@@ -11,13 +11,25 @@
 #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()),
+      weak_ptr_factory_(this) {
   DCHECK(reading_list_model);
   url_downloader_ = base::MakeUnique<URLDownloader>(
       distiller_service, prefs, chrome_profile_path,
@@ -25,9 +37,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 +76,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 +92,79 @@ 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,
+                 weak_ptr_factory_.GetWeakPtr(), entry.URL()),
+      entry.TimeUntilNextTry());
+}
+
+void ReadingListDownloadService::ScheduleDownloadEntryFromURL(const GURL& url) {
+  auto download_callback =
+      base::Bind(&ReadingListDownloadService::ScheduleDownloadEntry,
+                 base::Unretained(this));
+  reading_list_model_->CallbackEntryURL(url, download_callback);
+}
+
+void ReadingListDownloadService::DownloadEntryFromURL(const GURL& url) {
+  auto download_callback = base::Bind(
+      &ReadingListDownloadService::DownloadEntry, 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 +184,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);
+    ScheduleDownloadEntryFromURL(url);
+
   } else if (success == URLDownloader::ERROR_PERMANENT) {
     reading_list_model_->SetEntryDistilledState(url, ReadingListEntry::ERROR);
   }
@@ -122,3 +198,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_) {
+      ScheduleDownloadEntryFromURL(url);
+    }
+  }
+  if (type == net::NetworkChangeNotifier::CONNECTION_WIFI) {
+    for (auto& url : url_to_download_wifi_) {
+      ScheduleDownloadEntryFromURL(url);
+    }
+  }
+}

ios/chrome/browser/reading_list/reading_list_download_service.h

@@ -9,6 +9,7 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "ios/chrome/browser/reading_list/reading_list_model_observer.h"
 #include "ios/chrome/browser/reading_list/url_downloader.h"
+#include "net/base/network_change_notifier.h"
 
 class GURL;
 class PrefService;
@@ -25,8 +26,10 @@ class DomDistillerService;
 // Any calls made to DownloadAllEntries/DownloadEntry before the model is
 // loaded will be ignored. When the model is loaded, DownloadAllEntries will be
 // called automatically.
-class ReadingListDownloadService : public KeyedService,
-                                   public ReadingListModelObserver {
+class ReadingListDownloadService
+    : public KeyedService,
+      public ReadingListModelObserver,
+      public net::NetworkChangeNotifier::ConnectionTypeObserver {
  public:
   ReadingListDownloadService(
       ReadingListModel* reading_list_model,
@@ -56,6 +59,18 @@ class ReadingListDownloadService : public KeyedService,
   // Tries to save offline versions of all entries in the reading list that are
   // not yet saved. Must only be called after reading list model is loaded.
   void DownloadAllEntries();
+  // Schedule a download of an offline version of the reading list entry,
+  // according to the delay of the entry. Must only be called after reading list
+  // model is loaded.
+  void ScheduleDownloadEntry(const ReadingListEntry& entry);
+  // Tries to save an offline version of the reading list entry corresponding to
+  // the |url| if it is not yet saved. Must only be called after reading list
+  // model is loaded.
+  void DownloadEntryFromURL(const GURL& url);
+  // Schedule a download of an offline version of the reading list entry
+  // associated to this |url|, according to the delay of the entry. Must only be
+  // called after reading list model is loaded.
+  void ScheduleDownloadEntryFromURL(const GURL& url);
   // Tries to save an offline version of the reading list entry if it is not yet
   // saved. Must only be called after reading list model is loaded.
   void DownloadEntry(const ReadingListEntry& entry);
@@ -71,8 +86,17 @@ class ReadingListDownloadService : public KeyedService,
   // Callback for entry deletion.
   void OnDeleteEnd(const GURL& url, bool success);
 
+  // net::NetworkChangeNotifier::ConnectionTypeObserver:
+  void OnConnectionTypeChanged(
+      net::NetworkChangeNotifier::ConnectionType type) override;
+
   ReadingListModel* reading_list_model_;
   std::unique_ptr<URLDownloader> url_downloader_;
+  std::vector<GURL> url_to_download_cellular_;
+  std::vector<GURL> url_to_download_wifi_;
+  bool had_connection_;
+
+  base::WeakPtrFactory<ReadingListDownloadService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ReadingListDownloadService);
 };

ios/chrome/browser/reading_list/reading_list_entry.cc

@@ -37,7 +37,10 @@ ReadingListEntry::ReadingListEntry(const GURL& url, const std::string& title)
 ReadingListEntry::ReadingListEntry(const GURL& url,
                                    const std::string& title,
                                    std::unique_ptr<net::BackoffEntry> backoff)
-    : url_(url), title_(title), distilled_state_(WAITING) {
+    : url_(url),
+      title_(title),
+      distilled_state_(WAITING),
+      failed_download_counter_(0) {
   if (backoff) {
     backoff_ = std::move(backoff);
   } else {
@@ -52,7 +55,8 @@ ReadingListEntry::ReadingListEntry(ReadingListEntry&& entry)
       title_(std::move(entry.title_)),
       distilled_url_(std::move(entry.distilled_url_)),
       distilled_state_(std::move(entry.distilled_state_)),
-      backoff_(std::move(entry.backoff_)) {}
+      backoff_(std::move(entry.backoff_)),
+      failed_download_counter_(std::move(entry.failed_download_counter_)) {}
 
 ReadingListEntry::~ReadingListEntry() {}
 
@@ -76,12 +80,17 @@ base::TimeDelta ReadingListEntry::TimeUntilNextTry() const {
   return backoff_->GetTimeUntilRelease();
 }
 
+int ReadingListEntry::FailedDownloadCounter() const {
+  return failed_download_counter_;
+}
+
 ReadingListEntry& ReadingListEntry::operator=(ReadingListEntry&& other) {
   url_ = std::move(other.url_);
   title_ = std::move(other.title_);
   distilled_url_ = std::move(other.distilled_url_);
   distilled_state_ = std::move(other.distilled_state_);
   backoff_ = std::move(other.backoff_);
+  failed_download_counter_ = std::move(other.failed_download_counter_);
   return *this;
 }
 
@@ -98,6 +107,7 @@ void ReadingListEntry::SetDistilledURL(const GURL& url) {
   distilled_url_ = url;
   distilled_state_ = PROCESSED;
   backoff_->Reset();
+  failed_download_counter_ = 0;
 }
 
 void ReadingListEntry::SetDistilledState(DistillationState distilled_state) {
@@ -108,6 +118,7 @@ void ReadingListEntry::SetDistilledState(DistillationState distilled_state) {
   if ((distilled_state == WILL_RETRY || distilled_state == ERROR) &&
       distilled_state_ != WILL_RETRY && distilled_state_ != ERROR) {
     backoff_->InformOfRequest(false);
+    failed_download_counter_++;
   }
 
   distilled_state_ = distilled_state;

ios/chrome/browser/reading_list/reading_list_entry.h

@@ -43,6 +43,10 @@ class ReadingListEntry {
   // The time before the next try. This is automatically increased when the
   // state is set to WILL_RETRY or ERROR from a non-error state.
   base::TimeDelta TimeUntilNextTry() const;
+  // The number of time chrome failed to download this entry. This is
+  // automatically increased when the state is set to WILL_RETRY or ERROR from a
+  // non-error state.
+  int FailedDownloadCounter() const;
 
   ReadingListEntry& operator=(ReadingListEntry&& other);
   bool operator==(const ReadingListEntry& other) const;
@@ -61,6 +65,7 @@ class ReadingListEntry {
   GURL distilled_url_;
   DistillationState distilled_state_;
   std::unique_ptr<net::BackoffEntry> backoff_;
+  int failed_download_counter_;
 
   DISALLOW_COPY_AND_ASSIGN(ReadingListEntry);
 };

ios/chrome/browser/reading_list/reading_list_entry_unittest.cc

@@ -130,3 +130,24 @@ TEST(ReadingListEntry, ResetTimeUntilNextTry) {
   e.SetDistilledState(ReadingListEntry::ERROR);
   EXPECT_EQ(1, e.TimeUntilNextTry().InSeconds());
 }
+
+// Tests that the failed download counter is incremented when the state change
+// from non-error to error.
+TEST(ReadingListEntry, FailedDownloadCounter) {
+  ReadingListEntry e(GURL("http://example.com"), "bar");
+
+  ASSERT_EQ(0, e.FailedDownloadCounter());
+
+  e.SetDistilledState(ReadingListEntry::ERROR);
+  EXPECT_EQ(1, e.FailedDownloadCounter());
+  e.SetDistilledState(ReadingListEntry::WILL_RETRY);
+  EXPECT_EQ(1, e.FailedDownloadCounter());
+
+  e.SetDistilledState(ReadingListEntry::PROCESSING);
+  EXPECT_EQ(1, e.FailedDownloadCounter());
+
+  e.SetDistilledState(ReadingListEntry::WILL_RETRY);
+  EXPECT_EQ(2, e.FailedDownloadCounter());
+  e.SetDistilledState(ReadingListEntry::ERROR);
+  EXPECT_EQ(2, e.FailedDownloadCounter());
+}