Update reading list entry on download

ios/chrome/browser/reading_list/url_downloader.cc

Index: ios/chrome/browser/reading_list/url_downloader.cc
diff --git a/ios/chrome/browser/reading_list/url_downloader.cc b/ios/chrome/browser/reading_list/url_downloader.cc
index e0cc3448163ac89613264625a9c8f04265b317b8..07212143625ad8dbebe5e8a18e56e78b06a56a83 100644
--- a/ios/chrome/browser/reading_list/url_downloader.cc
+++ b/ios/chrome/browser/reading_list/url_downloader.cc
@@ -19,10 +19,6 @@
 
 namespace {
 char const kOfflineDirectory[] = "Offline";
-
-// TODO(crbug.com/629771): Handle errors & retrying of failed saves, including
-// distillation failure.
-
 }  // namespace
 
 // URLDownloader
@@ -31,7 +27,7 @@ URLDownloader::URLDownloader(
     dom_distiller::DomDistillerService* distiller_service,
     PrefService* prefs,
     base::FilePath chrome_profile_path,
-    const SuccessCompletion& download_completion,
+    const DownloadCompletion& download_completion,
     const SuccessCompletion& delete_completion)
     : distiller_service_(distiller_service),
       pref_service_(prefs),
@@ -70,12 +66,36 @@ void URLDownloader::DownloadOfflineURL(const GURL& url) {
   }
 }
 
-void URLDownloader::DownloadCompletionHandler(const GURL& url, bool success) {
+void URLDownloader::DownloadCompletionHandler(const GURL& url,
+                                              std::string title,
+                                              SuccessState success) {
   DCHECK(working_);
-  download_completion_.Run(url, success);
-  distiller_.reset();
-  working_ = false;
-  HandleNextTask();
+
+  auto postDelete = base::Bind(

[sdefresne, 2016/09/09 09:58:57]

style: variable should name should use name_with_underscore in c++ (note that
there are many violation of that rule in the file, maybe they can also be fixed)

nit: pass the title per const-reference

auto delete_post = base::Bind(
  [](URLDownloader* downloader, const GURL& url,
     const std::string& title, SuccessState success) {

[lody, 2016/09/09 12:17:53]

Done.

+      [](URLDownloader* _this, const GURL& url, std::string title,
+         SuccessState success) {
+        _this->download_completion_.Run(
+            url, success,
+            GURL("file://" + _this->OfflineURLPagePath(url).value()), title);

[sdefresne, 2016/09/09 09:58:57]

nit: maybe use url::kFileScheme instead of "file"

[lody, 2016/09/09 12:17:53]

Done.

+        _this->distiller_.reset();
+        _this->working_ = false;
+        _this->HandleNextTask();
+      },
+      base::Unretained(this), url, title, success);

[noyau (Ping after 24h), 2016/09/09 09:39:58]

I'm always worried when I see a base::Unretained. Is there something making sure
that the task is cancelled if this is deallocated?

[sdefresne, 2016/09/09 09:58:57]

+1

[lody, 2016/09/09 12:17:53]

That's what the task_tracker_ is for. Already had this issue for all the other
async calls in this file.

[noyau (Ping after 24h), 2016/09/09 12:45:22]

Ah, yes, I remember now. Sorry, should have checked.

+
+  // If downloading failed, clean up any partial download.
+  if (success == ERROR_RETRY || success == ERROR_PERMANENT) {
+    task_tracker_.PostTaskAndReply(
+        web::WebThread::GetTaskRunnerForThread(web::WebThread::FILE).get(),
+        FROM_HERE, base::Bind(
+                       [](base::FilePath offlineDirectoryPath) {

[sdefresne, 2016/09/09 09:58:57]

style: variable should name should use name_with_underscore in c++
nit: pass the path per const-reference

   [](const base::FilePath& offline_directory_path) {

[lody, 2016/09/09 12:17:53]

Done.

+                         base::DeleteFile(offlineDirectoryPath, true);
+                       },
+                       OfflineURLDirectoryPath(url)),
+        postDelete);
+  } else {
+    postDelete.Run();
+  }
 }
 
 void URLDownloader::DeleteCompletionHandler(const GURL& url, bool success) {
@@ -111,7 +131,7 @@ void URLDownloader::HandleNextTask() {
 
 void URLDownloader::DownloadURL(GURL url, bool offlineURLExists) {
   if (offlineURLExists) {
-    DownloadCompletionHandler(url, false);
+    DownloadCompletionHandler(url, "", DOWNLOAD_EXISTS);
     return;
   }
 
@@ -124,26 +144,34 @@ void URLDownloader::DistillerCallback(
     const GURL& pageURL,
     const std::string& html,
     const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&
-        images) {
+        images,
+    const std::string& title) {
   std::vector<dom_distiller::DistillerViewer::ImageInfo> imagesBlock = images;
   std::string blockHTML = html;
+  if (html.empty()) {
+    // TODO identify retry from permanent errors
+    DownloadCompletionHandler(pageURL, "", ERROR_PERMANENT);
+    return;
+  }
   task_tracker_.PostTaskAndReplyWithResult(
       web::WebThread::GetTaskRunnerForThread(web::WebThread::FILE).get(),
       FROM_HERE,
       base::Bind(&URLDownloader::SaveDistilledHTML, base::Unretained(this),
                  pageURL, imagesBlock, blockHTML),
       base::Bind(&URLDownloader::DownloadCompletionHandler,
-                 base::Unretained(this), pageURL));
+                 base::Unretained(this), pageURL, title));
 }
 
-bool URLDownloader::SaveDistilledHTML(
+URLDownloader::SuccessState URLDownloader::SaveDistilledHTML(
     const GURL& url,
     std::vector<dom_distiller::DistillerViewerInterface::ImageInfo> images,
     std::string html) {
   if (CreateOfflineURLDirectory(url)) {
-    return SaveHTMLForURL(SaveAndReplaceImagesInHTML(url, html, images), url);
+    return SaveHTMLForURL(SaveAndReplaceImagesInHTML(url, html, images), url)
+               ? DOWNLOAD_SUCCESS
+               : ERROR_PERMANENT;
   }
-  return false;
+  return ERROR_PERMANENT;
 }
 
 base::FilePath URLDownloader::OfflineDirectoryPath() {
@@ -167,19 +195,17 @@ bool URLDownloader::CreateOfflineURLDirectory(const GURL& url) {
   return true;
 }
 
-std::string URLDownloader::SaveImage(const GURL& url,
-                                     const GURL& imageURL,
-                                     const std::string& data) {
-  base::FilePath path =
-      OfflineURLDirectoryPath(url).Append(base::MD5String(imageURL.spec()));
+bool URLDownloader::SaveImage(const GURL& url,
+                              const GURL& imageURL,
+                              const std::string& data,
+                              base::FilePath& path) {
+  path = OfflineURLDirectoryPath(url).Append(base::MD5String(imageURL.spec()));
   if (!base::PathExists(path)) {
-    base::WriteFile(path, data.c_str(), data.length());
+    return base::WriteFile(path, data.c_str(), data.length()) > 0;
   }
-  return path.AsUTF8Unsafe();
+  return true;
 }
 
-// TODO(crbug.com/625621) DomDistiller doesn't correctly handle srcset
-// attributes in img tags.
 std::string URLDownloader::SaveAndReplaceImagesInHTML(
     const GURL& url,
     const std::string& html,
@@ -187,13 +213,15 @@ std::string URLDownloader::SaveAndReplaceImagesInHTML(
         images) {
   std::string mutableHTML = html;
   for (size_t i = 0; i < images.size(); i++) {
-    const std::string& localImagePath =
-        SaveImage(url, images[i].url, images[i].data);
+    base::FilePath localImagePath;

[sdefresne, 2016/09/09 09:58:58]

style: s/localImagePath/local_image_path/

[lody, 2016/09/09 12:17:53]

Done.

+    if (!SaveImage(url, images[i].url, images[i].data, localImagePath)) {
+      return "";

[sdefresne, 2016/09/09 09:58:58]

Please prefer to use std::string() instead of "".

[lody, 2016/09/09 12:17:53]

Done.

+    }
     const std::string& imageURL = images[i].url.spec();
     size_t imageURLSize = imageURL.size();
     size_t pos = mutableHTML.find(imageURL, 0);
     while (pos != std::string::npos) {
-      mutableHTML.replace(pos, imageURLSize, localImagePath);
+      mutableHTML.replace(pos, imageURLSize, localImagePath.AsUTF8Unsafe());
       pos = mutableHTML.find(imageURL, pos + imageURLSize);
     }
   }
@@ -201,6 +229,9 @@ std::string URLDownloader::SaveAndReplaceImagesInHTML(
 }
 
 bool URLDownloader::SaveHTMLForURL(std::string html, const GURL& url) {
+  if (html.empty()) {
+    return false;
+  }
   base::FilePath path = OfflineURLPagePath(url);
   return base::WriteFile(path, html.c_str(), html.length()) < 0;
-}
+}