[pdf] Unify document layout computation methods

There are currently two public methods for layout computation, one for
each page spread. The page spread is already stored in the layout
options, which is a member of the DocumentLayout class. A unified method
should be able to deduce the page spread from its options.

Bug: 1144505
Change-Id: I0a73d0e2282f7feb55aa1896144ab17180e6ca18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2842688
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
Reviewed-by: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#875787}

pdf/document_layout.cc

@@ -101,7 +101,16 @@ void DocumentLayout::SetOptions(const Options& options) {
   options_ = options;
 }
 
-void DocumentLayout::ComputeSingleViewLayout(
+void DocumentLayout::ComputeLayout(const std::vector<gfx::Size>& page_sizes) {
+  switch (options_.page_spread()) {
+    case PageSpread::kOneUp:
+      return ComputeOneUpLayout(page_sizes);
+    case PageSpread::kTwoUpOdd:
+      return ComputeTwoUpOddLayout(page_sizes);
+  }
+}
+
+void DocumentLayout::ComputeOneUpLayout(
     const std::vector<gfx::Size>& page_sizes) {
   gfx::Size document_size(GetWidestPageWidth(page_sizes), 0);
 
@@ -133,7 +142,7 @@ void DocumentLayout::ComputeSingleViewLayout(
   }
 }
 
-void DocumentLayout::ComputeTwoUpViewLayout(
+void DocumentLayout::ComputeTwoUpOddLayout(
     const std::vector<gfx::Size>& page_sizes) {
   gfx::Size document_size(GetWidestPageWidth(page_sizes), 0);
 

pdf/document_layout.h

@@ -127,15 +127,8 @@ class DocumentLayout final {
     return page_layouts_[page_index].inner_rect;
   }
 
-  // Computes layout that represent |page_sizes| formatted for single view.
-  //
-  // TODO(kmoon): Control layout type using an option.
-  void ComputeSingleViewLayout(const std::vector<gfx::Size>& page_sizes);
-
-  // Computes layout that represent |page_sizes| formatted for two-up view.
-  //
-  // TODO(kmoon): Control layout type using an option.
-  void ComputeTwoUpViewLayout(const std::vector<gfx::Size>& page_sizes);
+  // Computes the layout for a given list of |page_sizes| based on |options_|.
+  void ComputeLayout(const std::vector<gfx::Size>& page_sizes);
 
  private:
   // Layout of a single page.
@@ -147,6 +140,10 @@ class DocumentLayout final {
     gfx::Rect inner_rect;
   };
 
+  // Helpers for ComputeLayout() handling different page spreads.
+  void ComputeOneUpLayout(const std::vector<gfx::Size>& page_sizes);
+  void ComputeTwoUpOddLayout(const std::vector<gfx::Size>& page_sizes);
+
   // Copies |source_rect| to |destination_rect|, setting |dirty_| to true if
   // |destination_rect| is modified as a result.
   void CopyRectIfModified(const gfx::Rect& source_rect,

pdf/document_layout_unittest.cc

@@ -17,11 +17,6 @@ class DocumentLayoutOptionsTest : public testing::Test {
   DocumentLayout::Options options_;
 };
 
-class DocumentLayoutTest : public testing::Test {
- protected:
-  DocumentLayout layout_;
-};
-
 TEST_F(DocumentLayoutOptionsTest, DefaultConstructor) {
   EXPECT_EQ(options_.default_page_orientation(), PageOrientation::kOriginal);
   EXPECT_EQ(options_.page_spread(), DocumentLayout::PageSpread::kOneUp);
@@ -133,6 +128,17 @@ TEST_F(DocumentLayoutOptionsTest, RotatePagesCounterclockwise) {
   EXPECT_EQ(options_.default_page_orientation(), PageOrientation::kOriginal);
 }
 
+class DocumentLayoutTest : public testing::Test {
+ protected:
+  void SetPageSpread(DocumentLayout::PageSpread page_spread) {
+    DocumentLayout::Options options;
+    options.set_page_spread(page_spread);
+    layout_.SetOptions(options);
+  }
+
+  DocumentLayout layout_;
+};
+
 TEST_F(DocumentLayoutTest, DefaultConstructor) {
   EXPECT_EQ(layout_.options().default_page_orientation(),
             PageOrientation::kOriginal);
@@ -144,7 +150,7 @@ TEST_F(DocumentLayoutTest, DefaultConstructor) {
 }
 
 TEST_F(DocumentLayoutTest, SetOptionsDoesNotRecomputeLayout) {
-  layout_.ComputeSingleViewLayout({gfx::Size(100, 200)});
+  layout_.ComputeLayout({{100, 200}});
   EXPECT_EQ(layout_.size(), gfx::Size(100, 200));
 
   DocumentLayout::Options options;
@@ -184,10 +190,12 @@ TEST_F(DocumentLayoutTest, DirtyNotSetOnSameOptions) {
   EXPECT_FALSE(layout_.dirty());
 }
 
-TEST_F(DocumentLayoutTest, ComputeSingleViewLayout) {
+TEST_F(DocumentLayoutTest, ComputeLayoutOneUp) {
+  SetPageSpread(DocumentLayout::PageSpread::kOneUp);
+
   std::vector<gfx::Size> page_sizes{
       {300, 400}, {400, 500}, {300, 400}, {200, 300}};
-  layout_.ComputeSingleViewLayout(page_sizes);
+  layout_.ComputeLayout(page_sizes);
   ASSERT_EQ(4u, layout_.page_count());
   EXPECT_EQ(gfx::Rect(50, 0, 300, 400), layout_.page_rect(0));
   EXPECT_EQ(gfx::Rect(0, 404, 400, 500), layout_.page_rect(1));
@@ -200,7 +208,7 @@ TEST_F(DocumentLayoutTest, ComputeSingleViewLayout) {
   EXPECT_EQ(gfx::Size(400, 1612), layout_.size());
 
   page_sizes = {{240, 300}, {320, 400}, {250, 360}, {300, 600}, {270, 555}};
-  layout_.ComputeSingleViewLayout(page_sizes);
+  layout_.ComputeLayout(page_sizes);
   ASSERT_EQ(5u, layout_.page_count());
   EXPECT_EQ(gfx::Rect(40, 0, 240, 300), layout_.page_rect(0));
   EXPECT_EQ(gfx::Rect(0, 304, 320, 400), layout_.page_rect(1));
@@ -215,11 +223,36 @@ TEST_F(DocumentLayoutTest, ComputeSingleViewLayout) {
   EXPECT_EQ(gfx::Size(320, 2231), layout_.size());
 }
 
-TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
+TEST_F(DocumentLayoutTest, DirtySetOnLayoutInputChangeOneUp) {
+  SetPageSpread(DocumentLayout::PageSpread::kOneUp);
+
+  layout_.ComputeLayout({{100, 200}});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+  EXPECT_FALSE(layout_.dirty());
+
+  layout_.ComputeLayout({{100, 200}});
+  EXPECT_FALSE(layout_.dirty());
+
+  layout_.ComputeLayout({{200, 100}});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+
+  layout_.ComputeLayout({{200, 100}, {300, 300}});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+
+  layout_.ComputeLayout({{200, 100}});
+  EXPECT_TRUE(layout_.dirty());
+}
+
+TEST_F(DocumentLayoutTest, ComputeLayoutTwoUpOdd) {
+  SetPageSpread(DocumentLayout::PageSpread::kTwoUpOdd);
+
   // Test case where the widest page is on the right.
   std::vector<gfx::Size> page_sizes{
       {826, 1066}, {1066, 826}, {826, 1066}, {826, 900}};
-  layout_.ComputeTwoUpViewLayout(page_sizes);
+  layout_.ComputeLayout(page_sizes);
   ASSERT_EQ(4u, layout_.page_count());
   EXPECT_EQ(gfx::Rect(240, 0, 826, 1066), layout_.page_rect(0));
   EXPECT_EQ(gfx::Rect(1066, 0, 1066, 826), layout_.page_rect(1));
@@ -233,7 +266,7 @@ TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
 
   // Test case where the widest page is on the left.
   page_sizes = {{1066, 826}, {820, 1056}, {820, 890}, {826, 1066}};
-  layout_.ComputeTwoUpViewLayout(page_sizes);
+  layout_.ComputeLayout(page_sizes);
   ASSERT_EQ(4u, layout_.page_count());
   EXPECT_EQ(gfx::Rect(0, 0, 1066, 826), layout_.page_rect(0));
   EXPECT_EQ(gfx::Rect(1066, 0, 820, 1056), layout_.page_rect(1));
@@ -247,7 +280,7 @@ TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
 
   // Test case where there's an odd # of pages.
   page_sizes = {{200, 300}, {400, 200}, {300, 600}, {250, 500}, {300, 400}};
-  layout_.ComputeTwoUpViewLayout(page_sizes);
+  layout_.ComputeLayout(page_sizes);
   ASSERT_EQ(5u, layout_.page_count());
   EXPECT_EQ(gfx::Rect(200, 0, 200, 300), layout_.page_rect(0));
   EXPECT_EQ(gfx::Rect(400, 0, 400, 200), layout_.page_rect(1));
@@ -262,46 +295,26 @@ TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
   EXPECT_EQ(gfx::Size(800, 1300), layout_.size());
 }
 
-TEST_F(DocumentLayoutTest, DirtySetOnSingleViewLayoutInputChange) {
-  layout_.ComputeSingleViewLayout({gfx::Size(100, 200)});
+TEST_F(DocumentLayoutTest, DirtySetOnLayoutInputChangeTwoUpOdd) {
+  SetPageSpread(DocumentLayout::PageSpread::kTwoUpOdd);
+
+  layout_.ComputeLayout({{100, 200}, {200, 100}});
   EXPECT_TRUE(layout_.dirty());
   layout_.clear_dirty();
   EXPECT_FALSE(layout_.dirty());
 
-  layout_.ComputeSingleViewLayout({gfx::Size(100, 200)});
+  layout_.ComputeLayout({{100, 200}, {200, 100}});
   EXPECT_FALSE(layout_.dirty());
 
-  layout_.ComputeSingleViewLayout({gfx::Size(200, 100)});
+  layout_.ComputeLayout({{200, 100}, {100, 200}});
   EXPECT_TRUE(layout_.dirty());
   layout_.clear_dirty();
 
-  layout_.ComputeSingleViewLayout({gfx::Size(200, 100), gfx::Size(300, 300)});
+  layout_.ComputeLayout({{200, 100}, {100, 200}, {300, 300}});
   EXPECT_TRUE(layout_.dirty());
   layout_.clear_dirty();
 
-  layout_.ComputeSingleViewLayout({gfx::Size(200, 100)});
-  EXPECT_TRUE(layout_.dirty());
-}
-
-TEST_F(DocumentLayoutTest, DirtySetOnTwoUpViewLayoutInputChange) {
-  layout_.ComputeTwoUpViewLayout({gfx::Size(100, 200), gfx::Size(200, 100)});
-  EXPECT_TRUE(layout_.dirty());
-  layout_.clear_dirty();
-  EXPECT_FALSE(layout_.dirty());
-
-  layout_.ComputeTwoUpViewLayout({gfx::Size(100, 200), gfx::Size(200, 100)});
-  EXPECT_FALSE(layout_.dirty());
-
-  layout_.ComputeTwoUpViewLayout({gfx::Size(200, 100), gfx::Size(100, 200)});
-  EXPECT_TRUE(layout_.dirty());
-  layout_.clear_dirty();
-
-  layout_.ComputeTwoUpViewLayout(
-      {gfx::Size(200, 100), gfx::Size(100, 200), gfx::Size(300, 300)});
-  EXPECT_TRUE(layout_.dirty());
-  layout_.clear_dirty();
-
-  layout_.ComputeTwoUpViewLayout({gfx::Size(200, 100), gfx::Size(100, 200)});
+  layout_.ComputeLayout({{200, 100}, {100, 200}});
   EXPECT_TRUE(layout_.dirty());
 }
 

pdf/pdfium/pdfium_engine.cc

@@ -2843,12 +2843,7 @@ void PDFiumEngine::UpdateDocumentLayout(DocumentLayout* layout) {
   if (page_sizes.empty())
     return;
 
-  if (layout->options().page_spread() ==
-      DocumentLayout::PageSpread::kTwoUpOdd) {
-    layout->ComputeTwoUpViewLayout(page_sizes);
-  } else {
-    layout->ComputeSingleViewLayout(page_sizes);
-  }
+  layout->ComputeLayout(page_sizes);
 }
 
 std::vector<gfx::Size> PDFiumEngine::LoadPageSizes(