summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorlod <lod@chromium.org>2016-09-21 09:12:46 (GMT)
committerCommit bot <commit-bot@chromium.org>2016-09-21 09:14:56 (GMT)
commit65e74f6fd94834eac7bcb233f663383aa4e0bb5e (patch)
tree306db33c913074fcac68dbba44c8248d464d6238
parentb344fa0970355de30aae2e8d5f40103330e2c4e9 (diff)
downloadchromium-65e74f6fd94834eac7bcb233f663383aa4e0bb5e.tar.gz
chromium-65e74f6fd94834eac7bcb233f663383aa4e0bb5e.tar.xz
Update reading list entry on download
Update reading list entry with title and downloaded path; also handle errors. Left to possibly do is handling retry vs. permanent errors. BUG=616747 Review-Url: https://codereview.chromium.org/2320403002 Cr-Commit-Position: refs/heads/master@{#420014}
-rw-r--r--ios/chrome/browser/dom_distiller/distiller_viewer.cc29
-rw-r--r--ios/chrome/browser/dom_distiller/distiller_viewer.h11
-rw-r--r--ios/chrome/browser/reading_list/reading_list_download_service.cc27
-rw-r--r--ios/chrome/browser/reading_list/reading_list_download_service.h7
-rw-r--r--ios/chrome/browser/reading_list/url_downloader.cc118
-rw-r--r--ios/chrome/browser/reading_list/url_downloader.h45
-rw-r--r--ios/chrome/browser/reading_list/url_downloader_unittest.cc9
7 files changed, 164 insertions, 82 deletions
diff --git a/ios/chrome/browser/dom_distiller/distiller_viewer.cc b/ios/chrome/browser/dom_distiller/distiller_viewer.cc
index 1433246..a0bd94a 100644
--- a/ios/chrome/browser/dom_distiller/distiller_viewer.cc
+++ b/ios/chrome/browser/dom_distiller/distiller_viewer.cc
@@ -39,21 +39,24 @@ DistillerViewer::~DistillerViewer() {}
void DistillerViewer::OnArticleReady(
const dom_distiller::DistilledArticleProto* article_proto) {
DomDistillerRequestViewBase::OnArticleReady(article_proto);
+ if (article_proto->pages_size() > 0) {
+ std::vector<ImageInfo> images;
+ for (int i = 0; i < article_proto->pages(0).image_size(); i++) {
+ auto image = article_proto->pages(0).image(i);
+ images.push_back(ImageInfo{GURL(image.url()), image.data()});
+ }
- std::vector<ImageInfo> images;
- for (int i = 0; i < article_proto->pages(0).image_size(); i++) {
- auto image = article_proto->pages(0).image(i);
- images.push_back({GURL(image.url()), image.data()});
- }
-
- const std::string html = viewer::GetUnsafeArticleTemplateHtml(
- url_.spec(), distilled_page_prefs_->GetTheme(),
- distilled_page_prefs_->GetFontFamily());
+ const std::string html = viewer::GetUnsafeArticleTemplateHtml(
+ url_.spec(), distilled_page_prefs_->GetTheme(),
+ distilled_page_prefs_->GetFontFamily());
- std::string html_and_script(html);
- html_and_script +=
- "<script> distiller_on_ios = true; " + js_buffer_ + "</script>";
- callback_.Run(url_, html_and_script, images);
+ std::string html_and_script(html);
+ html_and_script +=
+ "<script> distiller_on_ios = true; " + js_buffer_ + "</script>";
+ callback_.Run(url_, html_and_script, images, article_proto->title());
+ } else {
+ callback_.Run(url_, std::string(), {}, std::string());
+ }
}
void DistillerViewer::SendJavaScript(const std::string& buffer) {
diff --git a/ios/chrome/browser/dom_distiller/distiller_viewer.h b/ios/chrome/browser/dom_distiller/distiller_viewer.h
index 4442493..639b7ea 100644
--- a/ios/chrome/browser/dom_distiller/distiller_viewer.h
+++ b/ios/chrome/browser/dom_distiller/distiller_viewer.h
@@ -23,15 +23,16 @@ class DistilledPagePrefs;
// contains.
class DistillerViewerInterface : public DomDistillerRequestViewBase {
public:
- typedef struct {
+ struct ImageInfo {
// The url of the image.
GURL url;
// The image data as a string.
std::string data;
- } ImageInfo;
- typedef base::Callback<void(const GURL&,
- const std::string&,
- const std::vector<ImageInfo>& images)>
+ };
+ typedef base::Callback<void(const GURL& url,
+ const std::string& html,
+ const std::vector<ImageInfo>& images,
+ const std::string& title)>
DistillationFinishedCallback;
DistillerViewerInterface(dom_distiller::DomDistillerService* distillerService,
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 fe510d5..61b1d01 100644
--- a/ios/chrome/browser/reading_list/reading_list_download_service.cc
+++ b/ios/chrome/browser/reading_list/reading_list_download_service.cc
@@ -10,7 +10,6 @@
#include "base/files/file_path.h"
#include "ios/chrome/browser/reading_list/reading_list_entry.h"
#include "ios/chrome/browser/reading_list/reading_list_model.h"
-#include "ios/chrome/browser/reading_list/url_downloader.h"
ReadingListDownloadService::ReadingListDownloadService(
ReadingListModel* reading_list_model,
@@ -87,7 +86,11 @@ void ReadingListDownloadService::DownloadAllEntries() {
void ReadingListDownloadService::DownloadEntry(const ReadingListEntry& entry) {
DCHECK(reading_list_model_->loaded());
- url_downloader_->DownloadOfflineURL(entry.URL());
+ if (entry.DistilledState() != ReadingListEntry::ERROR) {
+ reading_list_model_->SetEntryDistilledState(entry.URL(),
+ ReadingListEntry::PROCESSING);
+ url_downloader_->DownloadOfflineURL(entry.URL());
+ }
}
void ReadingListDownloadService::RemoveDownloadedEntry(
@@ -96,10 +99,24 @@ void ReadingListDownloadService::RemoveDownloadedEntry(
url_downloader_->RemoveOfflineURL(entry.URL());
}
-void ReadingListDownloadService::OnDownloadEnd(const GURL& url, bool success) {
- // TODO(crbug.com/616747) update entry with offline info
+void ReadingListDownloadService::OnDownloadEnd(
+ const GURL& url,
+ URLDownloader::SuccessState success,
+ const GURL& distilled_url,
+ const std::string& title) {
+ DCHECK(reading_list_model_->loaded());
+ if ((success == URLDownloader::DOWNLOAD_SUCCESS ||
+ 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);
+ } else if (success == URLDownloader::ERROR_PERMANENT) {
+ reading_list_model_->SetEntryDistilledState(url, ReadingListEntry::ERROR);
+ }
}
void ReadingListDownloadService::OnDeleteEnd(const GURL& url, bool success) {
- // TODO(crbug.com/616747) update entry with offline info
+ // Nothing to update as this is only called when deleting reading list entries
}
diff --git a/ios/chrome/browser/reading_list/reading_list_download_service.h b/ios/chrome/browser/reading_list/reading_list_download_service.h
index 9d3a285..2c23710 100644
--- a/ios/chrome/browser/reading_list/reading_list_download_service.h
+++ b/ios/chrome/browser/reading_list/reading_list_download_service.h
@@ -8,10 +8,10 @@
#include "base/macros.h"
#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"
class GURL;
class PrefService;
-class URLDownloader;
class ReadingListModel;
namespace base {
class FilePath;
@@ -63,7 +63,10 @@ class ReadingListDownloadService : public KeyedService,
// only be called after reading list model is loaded.
void RemoveDownloadedEntry(const ReadingListEntry& entry);
// Callback for entry download.
- void OnDownloadEnd(const GURL& url, bool success);
+ void OnDownloadEnd(const GURL& url,
+ URLDownloader::SuccessState success,
+ const GURL& distilled_url,
+ const std::string& title);
// Callback for entry deletion.
void OnDeleteEnd(const GURL& url, bool success);
diff --git a/ios/chrome/browser/reading_list/url_downloader.cc b/ios/chrome/browser/reading_list/url_downloader.cc
index e0cc344..8fabad8 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,38 @@ void URLDownloader::DownloadOfflineURL(const GURL& url) {
}
}
-void URLDownloader::DownloadCompletionHandler(const GURL& url, bool success) {
+void URLDownloader::DownloadCompletionHandler(const GURL& url,
+ const std::string& title,
+ SuccessState success) {
DCHECK(working_);
- download_completion_.Run(url, success);
- distiller_.reset();
- working_ = false;
- HandleNextTask();
+
+ auto post_delete = base::Bind(
+ [](URLDownloader* _this, const GURL& url, const std::string& title,
+ SuccessState success) {
+ _this->download_completion_.Run(
+ url, success,
+ GURL(std::string(url::kFileScheme) + url::kStandardSchemeSeparator +
+ _this->OfflineURLPagePath(url).value()),
+ title);
+ _this->distiller_.reset();
+ _this->working_ = false;
+ _this->HandleNextTask();
+ },
+ base::Unretained(this), url, title, success);
+
+ // 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(
+ [](const base::FilePath& offline_directory_path) {
+ base::DeleteFile(offline_directory_path, true);
+ },
+ OfflineURLDirectoryPath(url)),
+ post_delete);
+ } else {
+ post_delete.Run();
+ }
}
void URLDownloader::DeleteCompletionHandler(const GURL& url, bool success) {
@@ -109,9 +131,9 @@ void URLDownloader::HandleNextTask() {
}
}
-void URLDownloader::DownloadURL(GURL url, bool offlineURLExists) {
- if (offlineURLExists) {
- DownloadCompletionHandler(url, false);
+void URLDownloader::DownloadURL(GURL url, bool offline_url_exists) {
+ if (offline_url_exists) {
+ DownloadCompletionHandler(url, std::string(), DOWNLOAD_EXISTS);
return;
}
@@ -121,29 +143,37 @@ void URLDownloader::DownloadURL(GURL url, bool offlineURLExists) {
}
void URLDownloader::DistillerCallback(
- const GURL& pageURL,
+ const GURL& page_url,
const std::string& html,
const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&
- images) {
- std::vector<dom_distiller::DistillerViewer::ImageInfo> imagesBlock = images;
- std::string blockHTML = html;
+ images,
+ const std::string& title) {
+ std::vector<dom_distiller::DistillerViewer::ImageInfo> images_block = images;
+ std::string block_html = html;
+ if (html.empty()) {
+ // TODO identify retry from permanent errors
+ DownloadCompletionHandler(page_url, std::string(), 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),
+ page_url, images_block, block_html),
base::Bind(&URLDownloader::DownloadCompletionHandler,
- base::Unretained(this), pageURL));
+ base::Unretained(this), page_url, 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,40 +197,44 @@ 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& image_url,
+ const std::string& data,
+ base::FilePath& path) {
+ path = OfflineURLDirectoryPath(url).Append(base::MD5String(image_url.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;
+ std::string mutable_html = html;
for (size_t i = 0; i < images.size(); i++) {
- const std::string& localImagePath =
- SaveImage(url, images[i].url, images[i].data);
- const std::string& imageURL = images[i].url.spec();
- size_t imageURLSize = imageURL.size();
- size_t pos = mutableHTML.find(imageURL, 0);
+ base::FilePath local_image_path;
+ if (!SaveImage(url, images[i].url, images[i].data, local_image_path)) {
+ return std::string();
+ }
+ const std::string& image_url = images[i].url.spec();
+ size_t image_url_size = image_url.size();
+ size_t pos = mutable_html.find(image_url, 0);
while (pos != std::string::npos) {
- mutableHTML.replace(pos, imageURLSize, localImagePath);
- pos = mutableHTML.find(imageURL, pos + imageURLSize);
+ mutable_html.replace(pos, image_url_size,
+ local_image_path.AsUTF8Unsafe());
+ pos = mutable_html.find(image_url, pos + image_url_size);
}
}
- return mutableHTML;
+ return mutable_html;
}
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;
-} \ No newline at end of file
+ return base::WriteFile(path, html.c_str(), html.length()) > 0;
+}
diff --git a/ios/chrome/browser/reading_list/url_downloader.h b/ios/chrome/browser/reading_list/url_downloader.h
index 2eab241..2a839d9 100644
--- a/ios/chrome/browser/reading_list/url_downloader.h
+++ b/ios/chrome/browser/reading_list/url_downloader.h
@@ -31,16 +31,33 @@ class URLDownloader {
friend class MockURLDownloader;
public:
- // A completion callback that takes a GURL and a bool indicating
- // success and returns void.
+ // And enum indicating different download outcomes.
+ enum SuccessState {
+ DOWNLOAD_SUCCESS,
+ DOWNLOAD_EXISTS,
+ ERROR_RETRY,
+ ERROR_PERMANENT
+ };
+
+ // A completion callback that takes a GURL and a bool indicating the
+ // outcome and returns void.
using SuccessCompletion = base::Callback<void(const GURL&, bool)>;
+ // A download completion callback that takes, in order, the GURL that was
+ // downloaded, a SuccessState indicating the outcome of the download, the
+ // offline GURL for the download, and the title of the url, and returns void.
+ // The offline GURL and title should not be used in case of failure.
+ using DownloadCompletion = base::Callback<
+ void(const GURL&, SuccessState, const GURL&, const std::string&)>;
+
// Create a URL downloader with completion callbacks for downloads and
- // deletions.
+ // deletions. The completion callbacks will be called with the original url
+ // and a boolean indicating success. For downloads, if distillation was
+ // successful, it will also include the distilled url and extracted title.
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);
virtual ~URLDownloader();
@@ -60,7 +77,9 @@ class URLDownloader {
void HandleNextTask();
// Callback for completed (or failed) download, handles calling
// downloadCompletion and starting the next task.
- void DownloadCompletionHandler(const GURL& url, bool success);
+ void DownloadCompletionHandler(const GURL& url,
+ const std::string& title,
+ SuccessState success);
// Callback for completed (or failed) deletion, handles calling
// deleteCompletion and starting the next task.
void DeleteCompletionHandler(const GURL& url, bool success);
@@ -75,10 +94,11 @@ class URLDownloader {
// the directory already exists.
bool CreateOfflineURLDirectory(const GURL& url);
// Saves the |data| for image at |imageURL| to disk, for main URL |url|;
- // returns path of saved file.
- std::string SaveImage(const GURL& url,
- const GURL& imageURL,
- const std::string& data);
+ // puts path of saved file in |path| and returns whether save was successful.
+ bool SaveImage(const GURL& url,
+ const GURL& imageURL,
+ const std::string& data,
+ base::FilePath& path);
// Saves images in |images| array to disk and replaces references in |html| to
// local path. Returns updated html.
std::string SaveAndReplaceImagesInHTML(
@@ -91,7 +111,7 @@ class URLDownloader {
// Downloads |url|, depending on |offlineURLExists| state.
virtual void DownloadURL(GURL url, bool offlineURLExists);
// Saves distilled html to disk, including saving images and main file.
- bool SaveDistilledHTML(
+ SuccessState SaveDistilledHTML(
const GURL& url,
std::vector<dom_distiller::DistillerViewerInterface::ImageInfo> images,
std::string html);
@@ -100,11 +120,12 @@ class URLDownloader {
const GURL& pageURL,
const std::string& html,
const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&
- images);
+ images,
+ const std::string& title);
dom_distiller::DomDistillerService* distiller_service_;
PrefService* pref_service_;
- const SuccessCompletion download_completion_;
+ const DownloadCompletion download_completion_;
const SuccessCompletion delete_completion_;
std::deque<Task> tasks_;
bool working_;
diff --git a/ios/chrome/browser/reading_list/url_downloader_unittest.cc b/ios/chrome/browser/reading_list/url_downloader_unittest.cc
index 14ee119..8ba654b 100644
--- a/ios/chrome/browser/reading_list/url_downloader_unittest.cc
+++ b/ios/chrome/browser/reading_list/url_downloader_unittest.cc
@@ -25,7 +25,7 @@ class DistillerViewerTest : public dom_distiller::DistillerViewerInterface {
const DistillationFinishedCallback& callback)
: dom_distiller::DistillerViewerInterface(nil, nil) {
std::vector<ImageInfo> images;
- callback.Run(url, "html", images);
+ callback.Run(url, "html", images, "title");
}
void OnArticleReady(
@@ -73,7 +73,7 @@ class MockURLDownloader : public URLDownloader {
private:
void DownloadURL(GURL url, bool offlineURLExists) override {
if (offlineURLExists) {
- DownloadCompletionHandler(url, false);
+ DownloadCompletionHandler(url, std::string(), DOWNLOAD_EXISTS);
return;
}
distiller_.reset(new DistillerViewerTest(
@@ -81,7 +81,10 @@ class MockURLDownloader : public URLDownloader {
base::Bind(&URLDownloader::DistillerCallback, base::Unretained(this))));
}
- void OnEndDownload(const GURL& url, bool success) {
+ void OnEndDownload(const GURL& url,
+ SuccessState success,
+ const GURL& distilledURL,
+ const std::string& title) {
downloaded_files_.push_back(url);
}