Add incremental updates for multipage distillation.

components/dom_distiller/core/distiller_unittest.cc

Index: components/dom_distiller/core/distiller_unittest.cc
diff --git a/components/dom_distiller/core/distiller_unittest.cc b/components/dom_distiller/core/distiller_unittest.cc
index d3467c72aec481f1117c2b21b06cab57a27898c2..032da936a334854b77d3e883385d33908a164e04 100644
--- a/components/dom_distiller/core/distiller_unittest.cc
+++ b/components/dom_distiller/core/distiller_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
 #include <map>
 #include <string>
 #include <vector>
@@ -13,6 +14,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
+#include "components/dom_distiller/core/article_distillation_update.h"
 #include "components/dom_distiller/core/distiller.h"
 #include "components/dom_distiller/core/distiller_page.h"
 #include "components/dom_distiller/core/proto/distilled_article.pb.h"
@@ -58,6 +60,19 @@ namespace {
     return list.Pass();
   }
 
+  // Return the sequence in which Distiller will distill pages.
+  // Note: ignores any delays due to fetching images etc.
+  vector<int> get_pages_in_sequence(int start_page_num, int num_pages) {
+    // Distiller prefers distilling past pages first. E.g. when distillation
+    // starts on page 2 then pages are distilled in the order: 2, 1, 0, 3, 4.
+    vector<int> page_nums;
+    for (int page = start_page_num; page >= 0; --page)
+      page_nums.push_back(page);
+    for (int page = start_page_num + 1; page < num_pages; ++page)
+      page_nums.push_back(page);
+    return page_nums;
+  }
+
 }  // namespace
 
 namespace dom_distiller {
@@ -122,13 +137,26 @@ class MockDistillerPageFactory : public DistillerPageFactory {
 class DistillerTest : public testing::Test {
  public:
   virtual ~DistillerTest() {}
-  void OnDistillPageDone(scoped_ptr<DistilledArticleProto> proto) {
+  void OnDistillArticleDone(scoped_ptr<DistilledArticleProto> proto) {
     article_proto_ = proto.Pass();
   }
 
+  void OnDistillArticleUpdate(const ArticleDistillationUpdate& article_update) {
+    in_sequence_updates_.push_back(article_update);
+  }
+
+  void DistillPage(const std::string& url) {
+    distiller_->DistillPage(GURL(url),
+                            base::Bind(&DistillerTest::OnDistillArticleDone,
+                                       base::Unretained(this)),
+                            base::Bind(&DistillerTest::OnDistillArticleUpdate,
+                                       base::Unretained(this)));
+  }
+
  protected:
   scoped_ptr<DistillerImpl> distiller_;
   scoped_ptr<DistilledArticleProto> article_proto_;
+  std::vector<ArticleDistillationUpdate> in_sequence_updates_;
   MockDistillerPageFactory page_factory_;
   TestDistillerURLFetcherFactory url_fetcher_factory_;
 };
@@ -155,14 +183,7 @@ ACTION_P4(CreateMockDistillerPages, lists, kurls, num_pages, start_page_num) {
   EXPECT_CALL(*distiller_page, InitImpl());
   {
     testing::InSequence s;
-    // Distiller prefers distilling past pages first. E.g. when distillation
-    // starts on page 2 then pages are distilled in the order: 2, 1, 0, 3, 4.
-    vector<int> page_nums;
-    for (int page = start_page_num; page >= 0; --page)
-      page_nums.push_back(page);
-    for (int page = start_page_num + 1; page < num_pages; ++page)
-      page_nums.push_back(page);
-
+    vector<int> page_nums = get_pages_in_sequence(start_page_num, num_pages);
     for (size_t page_num = 0; page_num < page_nums.size(); ++page_num) {
       int page = page_nums[page_num];
       GURL url = GURL(kurls[page]);
@@ -177,6 +198,43 @@ ACTION_P4(CreateMockDistillerPages, lists, kurls, num_pages, start_page_num) {
   return distiller_page;
 }
 
+ACTION_P5(VerifyIncrementalUpdatesMatch,

[cjhopman, 2014/02/28 02:38:14]

Why is this an action? It isn't ever used as an action.

[shashi, 2014/03/02 02:53:40]

Changed to function.

+          start_page_num,
+          kNumPages,

[cjhopman, 2014/02/28 02:38:14]

nit: the capitalization of this is wrong

[shashi, 2014/03/02 02:53:40]

Done.

+          expected_page_urls,
+          expected_content,
+          incremental_updates) {
+  vector<int> page_nums = get_pages_in_sequence(start_page_num, kNumPages);

[cjhopman, 2014/02/28 02:38:14]

This function contains the following names:
start_page_num
page_num
page_nums
num_pages
kNumPages
update_pages
actual_page_num

This makes it really hard to follow. Please rename some things and maybe use
more descriptive names.

[shashi, 2014/03/02 02:53:40]

Done.

+  // Updates should contain a list of pages. Pages in a update should be in
+  // the correct ascending page order regardless of start_page_num.
+  // E.g. if distillation starts at page 2 of a 3 page article, the updates
+  // will be [ [2], [1, 2], [1, 2, 3]]. This example assumes that image fetches
+  // do not delay distillation of a page. There can be scenarios when image
+  // fetch delays distillation of a page (E.g. 1 is delayed due to image fetches
+  // so updates can be in this order [ [2], [2,3], [1,2,3]].
+  for (int page_num = 0; page_num < incremental_updates.size(); ++page_num) {

[cjhopman, 2014/02/28 02:38:14]

This seems like the index of an incremental update, why is it called page_num?

[cjhopman, 2014/02/28 02:38:14]

incremental_updates is a vector, why not use an iterator? If not that, than at
least store a reference into the vector here so you don't keep repeating the
lookup.

[shashi, 2014/03/02 02:53:40]

Done.

[shashi, 2014/03/02 02:53:40]

Done.

[shashi, 2014/03/02 02:53:40]

Done.

+    size_t num_pages = incremental_updates[page_num].getPagesSize();
+    EXPECT_EQ(static_cast<size_t>(page_num + 1), num_pages);
+    vector<int> update_pages(page_nums.begin(), page_nums.begin() + num_pages);

[cjhopman, 2014/02/28 02:38:14]

I'd rename this: expected_pages_in_update or some such

[shashi, 2014/03/02 02:53:40]

Done.

+    std::sort(update_pages.begin(), update_pages.end());
+
+    // If we already got the first page then there is no previous page.
+    EXPECT_EQ((update_pages[0] != 0),
+              incremental_updates[page_num].hasPrevPage());
+
+    // if we already got the last page then there is no next page.
+    EXPECT_EQ((*update_pages.rbegin()) != kNumPages - 1,
+              incremental_updates[page_num].hasNextPage());
+    for (size_t j = 0; j < num_pages; ++j) {
+      int actual_page_num = update_pages[j];
+      EXPECT_EQ(expected_page_urls[actual_page_num],
+                incremental_updates[page_num].getDistilledPage(j)->data.url());
+      EXPECT_EQ(expected_content[actual_page_num],
+                incremental_updates[page_num].getDistilledPage(j)->data.html());
+    }
+  }
+}
+
 TEST_F(DistillerTest, DistillPage) {
   base::MessageLoopForUI loop;
   scoped_ptr<base::ListValue> list =
@@ -185,9 +243,7 @@ TEST_F(DistillerTest, DistillPage) {
       .WillOnce(CreateMockDistillerPage(list.get(), GURL(kURL)));
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(kURL),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(kURL);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(article_proto_->pages_size(), 1);
@@ -208,9 +264,7 @@ TEST_F(DistillerTest, DistillPageWithImages) {
                   CreateMockDistillerPage(list.get(), GURL(kURL)));
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(kURL),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(kURL);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(article_proto_->pages_size(), 1);
@@ -259,9 +313,7 @@ TEST_F(DistillerTest, DistillMultiplePages) {
 
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(page_urls[0]),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(page_urls[0]);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(article_proto_->pages_size(), kNumPages);
@@ -291,9 +343,7 @@ TEST_F(DistillerTest, DistillLinkLoop) {
       .WillOnce(CreateMockDistillerPage(list.get(), GURL(kURL)));
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(kURL),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(kURL);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(article_proto_->pages_size(), 1);
@@ -326,9 +376,7 @@ TEST_F(DistillerTest, CheckMaxPageLimit) {
   distiller_->SetMaxNumPagesInArticle(kMaxPagesInArticle);
 
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(page_urls[0]),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(page_urls[0]);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(kMaxPagesInArticle,
@@ -346,9 +394,7 @@ TEST_F(DistillerTest, CheckMaxPageLimit) {
   distiller_->SetMaxNumPagesInArticle(kMaxPagesInArticle);
 
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(page_urls[0]),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(page_urls[0]);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(kMaxPagesInArticle,
@@ -366,9 +412,7 @@ TEST_F(DistillerTest, CheckMaxPageLimit) {
   distiller_->SetMaxNumPagesInArticle(kMaxPagesInArticle);
 
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(page_urls[0]),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(page_urls[0]);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(kMaxPagesInArticle,
@@ -383,9 +427,7 @@ TEST_F(DistillerTest, SinglePageDistillationFailure) {
       .WillOnce(CreateMockDistillerPage(nullValue.get(), GURL(kURL)));
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(kURL),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(kURL);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ("", article_proto_->title());
   EXPECT_EQ(0, article_proto_->pages_size());
@@ -419,9 +461,7 @@ TEST_F(DistillerTest, MultiplePagesDistillationFailure) {
 
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(page_urls[0]),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(page_urls[0]);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(article_proto_->pages_size(), failed_page_num);
@@ -440,36 +480,153 @@ TEST_F(DistillerTest, DistillPreviousPage) {
   scoped_ptr<base::Value> distilled_values[kNumPages];
 
   // The page number of the article on which distillation starts.
-  int start_page_number = 3;
+  int start_page_num = 3;
   string url_prefix = "http://a.com/";
-  for (int page_no = 0; page_no < kNumPages; ++page_no) {
-    page_urls[page_no] = url_prefix + base::IntToString(page_no);
-    content[page_no] = "Content for page:" + base::IntToString(page_no);
-    string next_page_url = (page_no + 1 < kNumPages)
-                               ? url_prefix + base::IntToString(page_no + 1)
+  for (int page_num = 0; page_num < kNumPages; ++page_num) {
+    page_urls[page_num] = url_prefix + base::IntToString(page_num);
+    content[page_num] = "Content for page:" + base::IntToString(page_num);
+    string next_page_url = (page_num + 1 < kNumPages)
+                               ? url_prefix + base::IntToString(page_num + 1)
                                : "";
-    string prev_page_url = (page_no > 0) ? page_urls[page_no - 1] : "";
-    distilled_values[page_no] = CreateDistilledValueReturnedFromJS(
-        kTitle, content[page_no], vector<int>(), next_page_url, prev_page_url);
+    string prev_page_url = (page_num > 0) ? page_urls[page_num - 1] : "";
+    distilled_values[page_num] = CreateDistilledValueReturnedFromJS(
+        kTitle, content[page_num], vector<int>(), next_page_url, prev_page_url);
   }
 
   EXPECT_CALL(page_factory_, CreateDistillerPageMock(_))
       .WillOnce(CreateMockDistillerPages(
-          distilled_values, page_urls, kNumPages, start_page_number));
+          distilled_values, page_urls, kNumPages, start_page_num));
 
   distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
   distiller_->Init();
-  distiller_->DistillPage(
-      GURL(page_urls[start_page_number]),
-      base::Bind(&DistillerTest::OnDistillPageDone, base::Unretained(this)));
+  DistillPage(page_urls[start_page_num]);
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   EXPECT_EQ(kNumPages, article_proto_->pages_size());
-  for (int page_no = 0; page_no < kNumPages; ++page_no) {
-    const DistilledPageProto& page = article_proto_->pages(page_no);
-    EXPECT_EQ(content[page_no], page.html());
-    EXPECT_EQ(page_urls[page_no], page.url());
+  for (int page_num = 0; page_num < kNumPages; ++page_num) {
+    const DistilledPageProto& page = article_proto_->pages(page_num);
+    EXPECT_EQ(content[page_num], page.html());
+    EXPECT_EQ(page_urls[page_num], page.url());
+  }
+}
+
+TEST_F(DistillerTest, IncrementalUpdates) {
+  base::MessageLoopForUI loop;
+  const int kNumPages = 8;
+  string content[kNumPages];
+  string page_urls[kNumPages];
+  scoped_ptr<base::Value> distilled_values[kNumPages];
+
+  // The page number of the article on which distillation starts.
+  int start_page_num = 3;
+  string url_prefix = "http://a.com/";
+  for (int page_num = 0; page_num < kNumPages; ++page_num) {
+    page_urls[page_num] = url_prefix + base::IntToString(page_num);
+    content[page_num] = "Content for page:" + base::IntToString(page_num);
+    string next_page_url = (page_num + 1 < kNumPages)
+                               ? url_prefix + base::IntToString(page_num + 1)
+                               : "";
+    string prev_page_url = (page_num > 0) ? page_urls[page_num - 1] : "";
+    distilled_values[page_num] = CreateDistilledValueReturnedFromJS(
+        kTitle, content[page_num], vector<int>(), next_page_url, prev_page_url);
   }
+
+  EXPECT_CALL(page_factory_, CreateDistillerPageMock(_))
+      .WillOnce(CreateMockDistillerPages(
+          distilled_values, page_urls, kNumPages, start_page_num));
+
+  distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
+  distiller_->Init();
+  DistillPage(page_urls[start_page_num]);
+  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(kTitle, article_proto_->title());
+  EXPECT_EQ(kNumPages, article_proto_->pages_size());
+  EXPECT_EQ(static_cast<size_t>(kNumPages), in_sequence_updates_.size());
+
+  VerifyIncrementalUpdatesMatch(
+      start_page_num, kNumPages, page_urls, content, in_sequence_updates_);
+}
+
+TEST_F(DistillerTest, IncrementalUpdatesDoNotDeleteFinalArticle) {
+  base::MessageLoopForUI loop;
+  const int kNumPages = 8;
+  string content[kNumPages];
+  string page_urls[kNumPages];
+  scoped_ptr<base::Value> distilled_values[kNumPages];
+
+  // The page number of the article on which distillation starts.
+  int start_page_num = 3;
+  string url_prefix = "http://a.com/";
+  for (int page_num = 0; page_num < kNumPages; ++page_num) {
+    page_urls[page_num] = url_prefix + base::IntToString(page_num);
+    content[page_num] = "Content for page:" + base::IntToString(page_num);
+    string next_page_url = (page_num + 1 < kNumPages)
+                               ? url_prefix + base::IntToString(page_num + 1)
+                               : "";
+    string prev_page_url = (page_num > 0) ? page_urls[page_num - 1] : "";
+    distilled_values[page_num] = CreateDistilledValueReturnedFromJS(
+        kTitle, content[page_num], vector<int>(), next_page_url, prev_page_url);
+  }
+
+  EXPECT_CALL(page_factory_, CreateDistillerPageMock(_))
+      .WillOnce(CreateMockDistillerPages(
+          distilled_values, page_urls, kNumPages, start_page_num));
+
+  distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
+  distiller_->Init();
+  DistillPage(page_urls[start_page_num]);
+  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(static_cast<size_t>(kNumPages), in_sequence_updates_.size());
+
+  in_sequence_updates_.clear();
+
+  // Should still be able to access article and pages.
+  EXPECT_EQ(kTitle, article_proto_->title());
+  EXPECT_EQ(kNumPages, article_proto_->pages_size());
+  for (int page_num = 0; page_num < kNumPages; ++page_num) {
+    const DistilledPageProto& page = article_proto_->pages(page_num);
+    EXPECT_EQ(content[page_num], page.html());
+    EXPECT_EQ(page_urls[page_num], page.url());
+  }
+}
+
+TEST_F(DistillerTest, DeletingArticleDoesNotInterfereWithUpdates) {
+  base::MessageLoopForUI loop;
+  const int kNumPages = 8;
+  string content[kNumPages];
+  string page_urls[kNumPages];
+  scoped_ptr<base::Value> distilled_values[kNumPages];
+
+  // The page number of the article on which distillation starts.
+  int start_page_num = 3;
+  string url_prefix = "http://a.com/";
+  for (int page_num = 0; page_num < kNumPages; ++page_num) {
+    page_urls[page_num] = url_prefix + base::IntToString(page_num);
+    content[page_num] = "Content for page:" + base::IntToString(page_num);
+    string next_page_url = (page_num + 1 < kNumPages)
+                               ? url_prefix + base::IntToString(page_num + 1)
+                               : "";
+    string prev_page_url = (page_num > 0) ? page_urls[page_num - 1] : "";
+    distilled_values[page_num] = CreateDistilledValueReturnedFromJS(
+        kTitle, content[page_num], vector<int>(), next_page_url, prev_page_url);
+  }

[cjhopman, 2014/02/28 02:38:14]

This setup code seems to be repeated three times. Should probably just extract
that out.

[shashi, 2014/03/02 02:53:40]

Good point, done, though it might make it slightly hard to review: I have
extracted it to CreateMultipageDistillerDataWithoutImages;

+
+  EXPECT_CALL(page_factory_, CreateDistillerPageMock(_))
+      .WillOnce(CreateMockDistillerPages(
+          distilled_values, page_urls, kNumPages, start_page_num));
+
+  distiller_.reset(new DistillerImpl(page_factory_, url_fetcher_factory_));
+  distiller_->Init();
+  DistillPage(page_urls[start_page_num]);
+  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_EQ(static_cast<size_t>(kNumPages), in_sequence_updates_.size());
+  EXPECT_EQ(kTitle, article_proto_->title());
+  EXPECT_EQ(kNumPages, article_proto_->pages_size());
+
+  // Delete the article.
+  article_proto_.reset();
+  VerifyIncrementalUpdatesMatch(
+      start_page_num, kNumPages, page_urls, content, in_sequence_updates_);
 }
 
 }  // namespace dom_distiller