[IOS Reading List] Change handling of distillation error.

ios/chrome/browser/reading_list/reading_list_download_service.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ios/chrome/browser/reading_list/reading_list_download_service.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_enumerator.h"
@@ skipping 234 matching lines @@
     if (!trimmed_title.empty())
       reading_list_model_->SetEntryTitle(url, trimmed_title);
 
     const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
     if (entry)
       UMA_HISTOGRAM_COUNTS_100("ReadingList.Download.Failures",
                                entry->FailedDownloadCounter());
     UMA_HISTOGRAM_ENUMERATION("ReadingList.Download.Status", SUCCESS,
                               STATUS_MAX);
 
-  } else if (success == URLDownloader::ERROR_RETRY) {
-    reading_list_model_->SetEntryDistilledState(url,
-                                                ReadingListEntry::WILL_RETRY);
-    ScheduleDownloadEntry(url);
-
+  } else if (success == URLDownloader::ERROR) {
     const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
+    bool will_retry = false;
     if (entry) {
-      if (entry->FailedDownloadCounter() < kNumberOfFailsBeforeStop) {
+      // Add this failure to the total failure count.
+      if (entry->FailedDownloadCounter() + 1 < kNumberOfFailsBeforeStop) {
         UMA_HISTOGRAM_ENUMERATION("ReadingList.Download.Status", RETRY,
                                   STATUS_MAX);
+        will_retry = true;
       } else {
         UMA_HISTOGRAM_ENUMERATION("ReadingList.Download.Status", FAILURE,
                                   STATUS_MAX);
       }
     }
+    if (will_retry) {

[gambard, 2017/02/17 14:07:47]

Why this is not merged with the metrics?
Is there a reason to schedule the download/set the state of a non-existing
entry?

[Olivier, 2017/02/17 14:27:15]

It is not merged precisely to not retry if entry does not exist :)
Merged using && precedence

+      reading_list_model_->SetEntryDistilledState(url,
+                                                  ReadingListEntry::WILL_RETRY);
+      ScheduleDownloadEntry(url);
+    } else {
+      reading_list_model_->SetEntryDistilledState(url, ReadingListEntry::ERROR);
+    }
 
-  } else if (success == URLDownloader::ERROR_PERMANENT) {
-    reading_list_model_->SetEntryDistilledState(url, ReadingListEntry::ERROR);
-    UMA_HISTOGRAM_ENUMERATION("ReadingList.Download.Status", FAILURE,
-                              STATUS_MAX);
+  } else {
+    // Catch if another status is added.

[gambard, 2017/02/17 14:07:47]

Use a switch?

[Olivier, 2017/02/17 14:27:15]

Done.

+    NOTREACHED();
   }
 }
 
 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_) {
       ScheduleDownloadEntry(url);
     }
   }
   if (type == net::NetworkChangeNotifier::CONNECTION_WIFI) {
     for (auto& url : url_to_download_wifi_) {
       ScheduleDownloadEntry(url);
     }
   }
 }