add pagination info to DistilledPageProto

components/dom_distiller/core/distiller_unittest.cc

 // Copyright 2013 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 <algorithm>
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 119 matching lines @@
     for (size_t j = 0; j < update.GetPagesSize(); ++j) {
       int actual_page_num = expected_page_nums_in_update[j];
       EXPECT_EQ(distiller_data->page_urls[actual_page_num],
                 update.GetDistilledPage(j).url());
       EXPECT_EQ(distiller_data->content[actual_page_num],
                 update.GetDistilledPage(j).html());
     }
   }
 }
 
+string GenerateNextPageUrl(size_t page_num, size_t pages_size,
+    string url_prefix) {

[nyquist, 2014/10/24 20:04:02]

How about using const reference to url_prefix here?

[kuan, 2014/10/24 20:57:32]

Done.  of course, too much java recently and forgot abt cc :(

+  return page_num + 1 < pages_size ?
+      url_prefix + base::IntToString(page_num + 1) : "";
+}
+
+string GeneratePrevPageUrl(size_t page_num, size_t pages_size,

[nyquist, 2014/10/24 20:04:02]

It seems like pages_size might not be needed for this function?

[kuan, 2014/10/24 20:57:32]

Done.

+    string url_prefix) {
+  return page_num > 0 ? url_prefix + base::IntToString(page_num - 1) : "";
+}
+
 scoped_ptr<MultipageDistillerData> CreateMultipageDistillerDataWithoutImages(
     size_t pages_size) {
   scoped_ptr<MultipageDistillerData> result(new MultipageDistillerData());
-  string url_prefix = "http://a.com/";
+  string url_prefix = kURL;
   for (size_t page_num = 0; page_num < pages_size; ++page_num) {
     result->page_urls.push_back(url_prefix + base::IntToString(page_num));
     result->content.push_back("Content for page:" +
                               base::IntToString(page_num));
     result->image_ids.push_back(vector<int>());
-    string next_page_url = (page_num + 1 < pages_size)
-                               ? url_prefix + base::IntToString(page_num + 1)
-                               : "";
+    string next_page_url =
+        GenerateNextPageUrl(page_num, pages_size, url_prefix);
     string prev_page_url =
-        (page_num > 0) ? result->page_urls[page_num - 1] : "";
+        GeneratePrevPageUrl(page_num, pages_size, url_prefix);

[nyquist, 2014/10/24 20:04:02]

If you end up removing pages_size here, maybe move url_prefix to be the first
argument here and above so the code ends up staying just as readable as it is
right now?

[kuan, 2014/10/24 20:57:32]

Done.

     scoped_ptr<base::Value> distilled_value =
         CreateDistilledValueReturnedFromJS(kTitle,
                                            result->content[page_num],
                                            result->image_ids[page_num],
                                            next_page_url,
                                            prev_page_url);
     result->distilled_values.push_back(distilled_value.release());
   }
   return result.Pass();
 }
 
 void VerifyArticleProtoMatchesMultipageData(
     const dom_distiller::DistilledArticleProto* article_proto,
     const MultipageDistillerData* distiller_data,
-    size_t pages_size) {
-  ASSERT_EQ(pages_size, static_cast<size_t>(article_proto->pages_size()));
+    size_t distilled_pages_size,
+    size_t total_pages_size) {
+  ASSERT_EQ(distilled_pages_size,
+            static_cast<size_t>(article_proto->pages_size()));
   EXPECT_EQ(kTitle, article_proto->title());
-  for (size_t page_num = 0; page_num < pages_size; ++page_num) {
+  string url_prefix = kURL;
+  for (size_t page_num = 0; page_num < distilled_pages_size; ++page_num) {
     const dom_distiller::DistilledPageProto& page =
         article_proto->pages(page_num);
     EXPECT_EQ(distiller_data->content[page_num], page.html());
     EXPECT_EQ(distiller_data->page_urls[page_num], page.url());
     EXPECT_EQ(distiller_data->image_ids[page_num].size(),
               static_cast<size_t>(page.image_size()));
     const vector<int>& image_ids_for_page = distiller_data->image_ids[page_num];
     for (size_t img_num = 0; img_num < image_ids_for_page.size(); ++img_num) {
       EXPECT_EQ(kImageData[image_ids_for_page[img_num]],
                 page.image(img_num).data());
       EXPECT_EQ(GetImageName(page_num + 1, img_num),
                 page.image(img_num).name());
     }
+    string expected_next_page_url =
+        GenerateNextPageUrl(page_num, total_pages_size, url_prefix);
+    string expected_prev_page_url =
+        GeneratePrevPageUrl(page_num, total_pages_size, url_prefix);
+    EXPECT_EQ(expected_next_page_url, page.pagination_info().next_page());
+    EXPECT_EQ(expected_prev_page_url, page.pagination_info().prev_page());
   }
 }
 
 }  // namespace
 
 namespace dom_distiller {
 
 using test::MockDistillerPage;
 using test::MockDistillerPageFactory;
 
@@ skipping 188 matching lines @@
     distiller_data->image_ids.push_back(image_indices);
   }
 
   distiller_.reset(
       new DistillerImpl(url_fetcher_factory_, DomDistillerOptions()));
   DistillPage(
       distiller_data->page_urls[0],
       CreateMockDistillerPages(distiller_data.get(), kNumPages, 0).Pass());
   base::MessageLoop::current()->RunUntilIdle();
   VerifyArticleProtoMatchesMultipageData(
-      article_proto_.get(), distiller_data.get(), kNumPages);
+      article_proto_.get(), distiller_data.get(), kNumPages, kNumPages);
 }
 
 TEST_F(DistillerTest, DistillLinkLoop) {
   base::MessageLoopForUI loop;
   // Create a loop, the next page is same as the current page. This could
   // happen if javascript misparses a next page link.
   scoped_ptr<base::Value> result =
       CreateDistilledValueReturnedFromJS(kTitle, kContent, vector<int>(), kURL);
   distiller_.reset(
       new DistillerImpl(url_fetcher_factory_, DomDistillerOptions()));
@@ skipping 87 matching lines @@
       base::Value::CreateNullValue());
   // Expect only calls till the failed page number.
   distiller_.reset(
       new DistillerImpl(url_fetcher_factory_, DomDistillerOptions()));
   DistillPage(distiller_data->page_urls[0],
               CreateMockDistillerPages(
                   distiller_data.get(), failed_page_num + 1, 0).Pass());
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kTitle, article_proto_->title());
   VerifyArticleProtoMatchesMultipageData(
-      article_proto_.get(), distiller_data.get(), failed_page_num);
+      article_proto_.get(), distiller_data.get(), failed_page_num, kNumPages);
 }
 
 TEST_F(DistillerTest, DistillPreviousPage) {
   base::MessageLoopForUI loop;
   const size_t kNumPages = 8;
 
   // The page number of the article on which distillation starts.
   int start_page_num = 3;
   scoped_ptr<MultipageDistillerData> distiller_data =
       CreateMultipageDistillerDataWithoutImages(kNumPages);
 
   distiller_.reset(
       new DistillerImpl(url_fetcher_factory_, DomDistillerOptions()));
   DistillPage(distiller_data->page_urls[start_page_num],
               CreateMockDistillerPages(
                   distiller_data.get(), kNumPages, start_page_num).Pass());
   base::MessageLoop::current()->RunUntilIdle();
   VerifyArticleProtoMatchesMultipageData(
-      article_proto_.get(), distiller_data.get(), kNumPages);
+      article_proto_.get(), distiller_data.get(), kNumPages, kNumPages);
 }
 
 TEST_F(DistillerTest, IncrementalUpdates) {
   base::MessageLoopForUI loop;
   const size_t kNumPages = 8;
 
   // The page number of the article on which distillation starts.
   int start_page_num = 3;
   scoped_ptr<MultipageDistillerData> distiller_data =
       CreateMultipageDistillerDataWithoutImages(kNumPages);
@@ skipping 24 matching lines @@
   DistillPage(distiller_data->page_urls[start_page_num],
               CreateMockDistillerPages(
                   distiller_data.get(), kNumPages, start_page_num).Pass());
   base::MessageLoop::current()->RunUntilIdle();
   EXPECT_EQ(kNumPages, in_sequence_updates_.size());
 
   in_sequence_updates_.clear();
 
   // Should still be able to access article and pages.
   VerifyArticleProtoMatchesMultipageData(
-      article_proto_.get(), distiller_data.get(), kNumPages);
+      article_proto_.get(), distiller_data.get(), kNumPages, kNumPages);
 }
 
 TEST_F(DistillerTest, DeletingArticleDoesNotInterfereWithUpdates) {
   base::MessageLoopForUI loop;
   const size_t kNumPages = 8;
   scoped_ptr<MultipageDistillerData> distiller_data =
       CreateMultipageDistillerDataWithoutImages(kNumPages);
   // The page number of the article on which distillation starts.
   int start_page_num = 3;
 
@@ skipping 50 matching lines @@
 
   ASSERT_TRUE(distiller_page);
   // Post the task to execute javascript and then delete the distiller.
   distiller_page->OnDistillationDone(GURL(kURL), distilled_value.get());
   distiller_.reset();
 
   base::MessageLoop::current()->RunUntilIdle();
 }
 
 }  // namespace dom_distiller