Commit 39186c37 authored by cjhopman's avatar cjhopman Committed by Commit bot

Fix handling of failed distillation.

Currently, when distillation fails, the TaskTracker signals that a
source has finished. It then checks if all sources are done (and if not,
it will wait to call the callback). The check for sources done though
assumes that if distiller_ is non-null, then distillation is still in
progress. So, we should be deleting/releasing distiller_ when it
finished (and before signalling that the source is finished).

This also updates the standalone content extractor to fail if extraction
of some url fails.

Review URL: https://codereview.chromium.org/575883002

Cr-Commit-Position: refs/heads/master@{#295599}
parent dd63d249
...@@ -60,6 +60,7 @@ void TaskTracker::StartDistiller(DistillerFactory* factory, ...@@ -60,6 +60,7 @@ void TaskTracker::StartDistiller(DistillerFactory* factory,
void TaskTracker::StartBlobFetcher() { void TaskTracker::StartBlobFetcher() {
if (content_store_) { if (content_store_) {
blob_fetcher_running_ = true;
content_store_->LoadContent(entry_, content_store_->LoadContent(entry_,
base::Bind(&TaskTracker::OnBlobFetched, base::Bind(&TaskTracker::OnBlobFetched,
weak_ptr_factory_.GetWeakPtr())); weak_ptr_factory_.GetWeakPtr()));
...@@ -147,11 +148,17 @@ void TaskTracker::OnDistillerFinished( ...@@ -147,11 +148,17 @@ void TaskTracker::OnDistillerFinished(
AddDistilledContentToStore(*distilled_article_); AddDistilledContentToStore(*distilled_article_);
} }
// 'distiller_ != null' is used as a signal that distillation is in progress,
// so it needs to be released so that we know distillation is done.
base::MessageLoop::current()->DeleteSoon(FROM_HERE, distiller_.release());
ContentSourceFinished(); ContentSourceFinished();
} }
void TaskTracker::CancelPendingSources() { void TaskTracker::CancelPendingSources() {
base::MessageLoop::current()->DeleteSoon(FROM_HERE, distiller_.release()); if (distiller_) {
base::MessageLoop::current()->DeleteSoon(FROM_HERE, distiller_.release());
}
} }
void TaskTracker::OnBlobFetched( void TaskTracker::OnBlobFetched(
......
...@@ -159,6 +159,31 @@ TEST_F(DomDistillerTaskTrackerTest, TestViewerNotifiedOnDistillationComplete) { ...@@ -159,6 +159,31 @@ TEST_F(DomDistillerTaskTrackerTest, TestViewerNotifiedOnDistillationComplete) {
EXPECT_FALSE(cancel_callback.Cancelled()); EXPECT_FALSE(cancel_callback.Cancelled());
} }
TEST_F(DomDistillerTaskTrackerTest, TestDistillerFails) {
MockDistillerFactory distiller_factory;
FakeDistiller* distiller = new FakeDistiller(false);
EXPECT_CALL(distiller_factory, CreateDistillerImpl())
.WillOnce(Return(distiller));
TestCancelCallback cancel_callback;
TaskTracker task_tracker(
GetDefaultEntry(), cancel_callback.GetCallback(), NULL);
FakeViewRequestDelegate viewer_delegate;
scoped_ptr<ViewerHandle> handle(task_tracker.AddViewer(&viewer_delegate));
base::RunLoop().RunUntilIdle();
EXPECT_CALL(viewer_delegate, OnArticleReady(_));
task_tracker.StartDistiller(&distiller_factory,
scoped_ptr<DistillerPage>().Pass());
distiller->RunDistillerCallback(
scoped_ptr<DistilledArticleProto>(new DistilledArticleProto));
base::RunLoop().RunUntilIdle();
EXPECT_FALSE(cancel_callback.Cancelled());
}
TEST_F(DomDistillerTaskTrackerTest, TEST_F(DomDistillerTaskTrackerTest,
TestSaveCallbackCalledOnDistillationComplete) { TestSaveCallbackCalledOnDistillationComplete) {
MockDistillerFactory distiller_factory; MockDistillerFactory distiller_factory;
......
...@@ -205,6 +205,7 @@ class ContentExtractionRequest : public ViewRequestDelegate { ...@@ -205,6 +205,7 @@ class ContentExtractionRequest : public ViewRequestDelegate {
virtual void OnArticleReady(const DistilledArticleProto* article_proto) virtual void OnArticleReady(const DistilledArticleProto* article_proto)
OVERRIDE { OVERRIDE {
article_proto_ = article_proto; article_proto_ = article_proto;
CHECK(article_proto->pages_size()) << "Failed extracting " << url_;
base::MessageLoop::current()->PostTask( base::MessageLoop::current()->PostTask(
FROM_HERE, FROM_HERE,
finished_callback_); finished_callback_);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment