Update reading list entry on download

ios/chrome/browser/reading_list/url_downloader.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/url_downloader.h"
 
 #include <string>
 #include <vector>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/md5.h"
 #include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "ios/chrome/browser/chrome_paths.h"
 #include "ios/chrome/browser/dom_distiller/distiller_viewer.h"
 #include "ios/web/public/web_thread.h"
 #include "url/gurl.h"
 
 namespace {
 char const kOfflineDirectory[] = "Offline";
-
-// TODO(crbug.com/629771): Handle errors & retrying of failed saves, including
-// distillation failure.
-
 }  // namespace
 
 // URLDownloader
 
 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),
       download_completion_(download_completion),
       delete_completion_(delete_completion),
       working_(false),
       base_directory_(chrome_profile_path),
       task_tracker_() {}
 
 URLDownloader::~URLDownloader() {
@@ skipping 18 matching lines @@
 }
 
 void URLDownloader::DownloadOfflineURL(const GURL& url) {
   if (std::find(tasks_.begin(), tasks_.end(), std::make_pair(DOWNLOAD, url)) ==
       tasks_.end()) {
     tasks_.push_back(std::make_pair(DOWNLOAD, url));
     HandleNextTask();
   }
 }
 
-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) {
   DCHECK(working_);
   delete_completion_.Run(url, success);
   working_ = false;
   HandleNextTask();
 }
 
 void URLDownloader::HandleNextTask() {
@@ skipping 15 matching lines @@
                    base::Unretained(this), url));
   } else if (task.first == DOWNLOAD) {
     DCHECK(!distiller_);
     OfflineURLExists(url, base::Bind(&URLDownloader::DownloadURL,
                                      base::Unretained(this), url));
   }
 }
 
 void URLDownloader::DownloadURL(GURL url, bool offlineURLExists) {
   if (offlineURLExists) {
-    DownloadCompletionHandler(url, false);
+    DownloadCompletionHandler(url, "", DOWNLOAD_EXISTS);
     return;
   }
 
   distiller_.reset(new dom_distiller::DistillerViewer(
       distiller_service_, pref_service_, url,
       base::Bind(&URLDownloader::DistillerCallback, base::Unretained(this))));
 }
 
 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() {
   return base_directory_.Append(kOfflineDirectory);
 }
 
 base::FilePath URLDownloader::OfflineURLDirectoryPath(const GURL& url) {
   std::string hash = base::MD5String(url.spec());
   return OfflineDirectoryPath().Append(hash);
 }
 
 base::FilePath URLDownloader::OfflineURLPagePath(const GURL& url) {
   return OfflineURLDirectoryPath(url).Append("page.html");
 }
 
 bool URLDownloader::CreateOfflineURLDirectory(const GURL& url) {
   base::FilePath path = OfflineURLDirectoryPath(url);
   if (!DirectoryExists(path)) {
     return CreateDirectoryAndGetError(path, nil);
   }
   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,
     const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&
         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);
     }
   }
   return mutableHTML;
 }
 
 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;
-}
+}