Unify layout code in AppendBlankPages

Refactors PDFiumEngine::AppendBlankPages() to leverage the layout code
in LoadPageInfo(), instead of using a separate copy. AppendBlankPages()
now ensures |PDFiumEngine::pages_| is in the right state for a later
layout pass, rather than trying to compute all the layout up front when
the new PDFiumPage instances are created.

This duplication is particularly harmful because AppendBlankPages() is
only called from print preview code paths, and so may be overlooked if
not testing print previewing specifically.

Also inlined PDFiumEngine::LoadPagesInCurrentLayout(), since this method
is only used in one place (and we're trying to centralize layout), has
simple logic, and will become much shorter once the two "compute layout"
methods are merged together.

Bug: 885110
Change-Id: Ic0ae2410ca930794beeb28983b9d0cc5e89284f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769708
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: K Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691858}

pdf/BUILD.gn

@@ -203,6 +203,7 @@ if (enable_pdf) {
         "pdfium/accessibility_unittest.cc",
         "pdfium/findtext_unittest.cc",
         "pdfium/pdfium_engine_exports_unittest.cc",
+        "pdfium/pdfium_engine_unittest.cc",
         "pdfium/pdfium_page_unittest.cc",
         "pdfium/pdfium_permissions_unittest.cc",
         "pdfium/pdfium_print_unittest.cc",

pdf/document_layout.cc

@@ -59,7 +59,7 @@ void DocumentLayout::ComputeSingleViewLayout(
   for (size_t i = 0; i < page_sizes.size(); ++i) {
     if (i != 0) {
       // Add space for bottom separator.
-      EnlargeHeight(kBottomSeparator);
+      size_.Enlarge(0, kBottomSeparator);
     }
 
     const pp::Size& page_size = page_sizes[i];
@@ -89,22 +89,18 @@ void DocumentLayout::ComputeTwoUpViewLayout(
     } else {
       page_rect = draw_utils::GetRightRectForTwoUpView(
           page_size, {size_.width(), size_.height()});
-      EnlargeHeight(std::max(page_size.height(), page_sizes[i - 1].height()));
+      size_.Enlarge(0,
+                    std::max(page_size.height(), page_sizes[i - 1].height()));
     }
     page_layouts_[i].outer_rect = page_rect;
     page_layouts_[i].inner_rect = InsetRect(page_rect, page_insets);
   }
 
   if (page_sizes.size() % 2 == 1) {
-    EnlargeHeight(page_sizes.back().height());
+    size_.Enlarge(0, page_sizes.back().height());
   }
 
   size_.set_width(2 * size_.width());
 }
 
-void DocumentLayout::EnlargeHeight(int height) {
-  DCHECK_GE(height, 0);
-  size_.Enlarge(0, height);
-}
-
 }  // namespace chrome_pdf

pdf/document_layout.h

@@ -99,11 +99,6 @@ class DocumentLayout final {
   // TODO(kmoon): Control layout type using an option.
   void ComputeTwoUpViewLayout(const std::vector<pp::Size>& page_sizes);
 
-  // Increases the layout's total height by |height|.
-  //
-  // TODO(kmoon): Delete or make this private.
-  void EnlargeHeight(int height);
-
  private:
   // Layout of a single page.
   struct PageLayout {

pdf/document_layout_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "pdf/document_layout.h"
 
-#include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace chrome_pdf {
@@ -21,8 +20,6 @@ class DocumentLayoutTest : public testing::Test {
   DocumentLayout layout_;
 };
 
-using DocumentLayoutDeathTest = DocumentLayoutTest;
-
 // TODO(kmoon): Need to use this with EXPECT_PRED2 instead of just using
 // EXPECT_EQ, due to ADL issues with pp::Size's operator== (defined in global
 // namespace, instead of in "pp").
@@ -110,14 +107,6 @@ TEST_F(DocumentLayoutTest, SetOptionsDoesNotRecomputeLayout) {
   EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(1, 2));
 }
 
-TEST_F(DocumentLayoutTest, EnlargeHeight) {
-  layout_.EnlargeHeight(5);
-  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 5));
-
-  layout_.EnlargeHeight(11);
-  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 16));
-}
-
 TEST_F(DocumentLayoutTest, ComputeSingleViewLayout) {
   std::vector<pp::Size> page_sizes{
       {300, 400}, {400, 500}, {300, 400}, {200, 300}};
@@ -218,10 +207,6 @@ TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
   EXPECT_PRED2(PpSizeEq, pp::Size(800, 1300), layout_.size());
 }
 
-TEST_F(DocumentLayoutDeathTest, EnlargeHeightNegativeIncrement) {
-  EXPECT_DCHECK_DEATH(layout_.EnlargeHeight(-5));
-}
-
 }  // namespace
 
 }  // namespace chrome_pdf

pdf/pdf_engine.h

@@ -385,7 +385,7 @@ class PDFEngine {
 
   // Append blank pages to make a 1-page document to a |num_pages| document.
   // Always retain the first page data.
-  virtual void AppendBlankPages(int num_pages) = 0;
+  virtual void AppendBlankPages(size_t num_pages) = 0;
   // Append the first page of the document loaded with the |engine| to this
   // document at page |index|.
   virtual void AppendPage(PDFEngine* engine, int index) = 0;

pdf/pdfium/pdfium_engine.cc

@@ -2226,9 +2226,8 @@ bool PDFiumEngine::GetPageSizeAndUniformity(pp::Size* size) {
   return true;
 }
 
-// TODO(kmoon): Rewrite this to use DocumentLayout properly.
-void PDFiumEngine::AppendBlankPages(int num_pages) {
-  DCHECK_NE(num_pages, 0);
+void PDFiumEngine::AppendBlankPages(size_t num_pages) {
+  DCHECK_GT(num_pages, 0U);
 
   if (!doc())
     return;
@@ -2242,49 +2241,26 @@ void PDFiumEngine::AppendBlankPages(int num_pages) {
     FPDFPage_Delete(doc(), pages_.size());
   }
 
-  // Calculate document size and all page sizes.
-  std::vector<pp::Rect> page_rects;
-  pp::Size page_size = GetPageSize(0);
-  page_size.Enlarge(DocumentLayout::kSingleViewInsets.left +
-                        DocumentLayout::kSingleViewInsets.right,
-                    DocumentLayout::kSingleViewInsets.top +
-                        DocumentLayout::kSingleViewInsets.bottom);
-  pp::Size old_document_size = layout_.size();
-  layout_.set_size(pp::Size(page_size.width(), 0));
-  for (int i = 0; i < num_pages; ++i) {
-    if (i != 0) {
-      // Add space for bottom separator.
-      layout_.EnlargeHeight(DocumentLayout::kBottomSeparator);
-    }
+  // Create blank pages with the same size as the first page.
+  pp::Size page_0_size = GetPageSize(0);
+  double page_0_width_in_points =
+      ConvertUnitDouble(page_0_size.width(), kPixelsPerInch, kPointsPerInch);
+  double page_0_height_in_points =
+      ConvertUnitDouble(page_0_size.height(), kPixelsPerInch, kPointsPerInch);
 
-    pp::Rect rect(pp::Point(0, layout_.size().height()), page_size);
-    page_rects.push_back(rect);
-
-    layout_.EnlargeHeight(page_size.height());
-  }
-
-  // Create blank pages.
-  for (int i = 1; i < num_pages; ++i) {
-    pp::Rect page_rect(page_rects[i]);
-    page_rect.Inset(DocumentLayout::kSingleViewInsets.left,
-                    DocumentLayout::kSingleViewInsets.top,
-                    DocumentLayout::kSingleViewInsets.right,
-                    DocumentLayout::kSingleViewInsets.bottom);
-    double width_in_points =
-        ConvertUnitDouble(page_rect.width(), kPixelsPerInch, kPointsPerInch);
-    double height_in_points =
-        ConvertUnitDouble(page_rect.height(), kPixelsPerInch, kPointsPerInch);
+  for (size_t i = 1; i < num_pages; ++i) {
     {
       // Add a new page to the document, but delete the FPDF_PAGE object.
-      ScopedFPDFPage temp_page(
-          FPDFPage_New(doc(), i, width_in_points, height_in_points));
+      ScopedFPDFPage temp_page(FPDFPage_New(doc(), i, page_0_width_in_points,
+                                            page_0_height_in_points));
     }
-    pages_.push_back(std::make_unique<PDFiumPage>(this, i, page_rect, true));
+
+    auto page =
+        std::make_unique<PDFiumPage>(this, i, pp::Rect(), /*available=*/true);
+    pages_.push_back(std::move(page));
   }
 
-  CalculateVisiblePages();
-  if (layout_.size() != old_document_size)
-    client_->DocumentSizeUpdated(layout_.size());
+  LoadPageInfo(true);
 }
 
 void PDFiumEngine::LoadDocument() {
@@ -2386,17 +2362,6 @@ void PDFiumEngine::ContinueLoadingDocument(const std::string& password) {
     FinishLoadingDocument();
 }
 
-void PDFiumEngine::LoadPagesInCurrentLayout(std::vector<pp::Size> page_sizes,
-                                            bool reload) {
-  if (two_up_view_) {
-    layout_.ComputeTwoUpViewLayout(page_sizes);
-  } else {
-    layout_.ComputeSingleViewLayout(page_sizes);
-  }
-
-  ApplyCurrentLayoutToPages(reload);
-}
-
 // TODO(kmoon): This should be the only method that sets |PDFiumPage::rect_|.
 void PDFiumEngine::ApplyCurrentLayoutToPages(bool reload) {
   for (size_t i = 0; i < layout_.page_count(); ++i) {
@@ -2452,7 +2417,13 @@ void PDFiumEngine::LoadPageInfo(bool reload) {
     page_sizes.push_back(size);
   }
 
-  LoadPagesInCurrentLayout(page_sizes, reload);
+  if (two_up_view_) {
+    layout_.ComputeTwoUpViewLayout(page_sizes);
+  } else {
+    layout_.ComputeSingleViewLayout(page_sizes);
+  }
+
+  ApplyCurrentLayoutToPages(reload);
 
   // Remove pages that do not exist anymore.
   if (pages_.size() > new_page_count) {

pdf/pdfium/pdfium_engine.h

@@ -120,7 +120,7 @@ class PDFiumEngine : public PDFEngine,
   int GetCopiesToPrint() override;
   int GetDuplexType() override;
   bool GetPageSizeAndUniformity(pp::Size* size) override;
-  void AppendBlankPages(int num_pages) override;
+  void AppendBlankPages(size_t num_pages) override;
   void AppendPage(PDFEngine* engine, int index) override;
   std::string GetMetadata(const std::string& key) override;
   std::vector<uint8_t> GetSaveData() override;
@@ -233,10 +233,6 @@ class PDFiumEngine : public PDFEngine,
   // If this has been run once, it will not notify the client again.
   void FinishLoadingDocument();
 
-  // Formats the pages of |page_sizes| and appends them to |pages_|. Formats to
-  // two-up view if |two_up_view_| is true, else formats to single-view.
-  void LoadPagesInCurrentLayout(std::vector<pp::Size> page_sizes, bool reload);
-
   // Applies the current layout to the PDFiumPage objects. This primarily
   // involves updating the PDFiumPage rectangles from the corresponding layout
   // page rectangles.

pdf/pdfium/pdfium_engine_unittest.cc

@@ -0,0 +1,99 @@
+// Copyright 2019 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 "pdf/pdfium/pdfium_engine.h"
+
+#include "pdf/pdfium/pdfium_page.h"
+#include "pdf/pdfium/pdfium_test_base.h"
+#include "pdf/test/test_client.h"
+#include "pdf/test/test_utils.h"
+#include "ppapi/cpp/size.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chrome_pdf {
+namespace {
+
+using ::testing::InSequence;
+using ::testing::NiceMock;
+
+class MockTestClient : public TestClient {
+ public:
+  // TODO(crbug.com/989095): MOCK_METHOD() triggers static_assert on Windows.
+  MOCK_METHOD1(DocumentSizeUpdated, void(const pp::Size& size));
+};
+
+class PDFiumEngineTest : public PDFiumTestBase {
+ protected:
+  void ExpectPageRect(PDFiumEngine* engine,
+                      size_t page_index,
+                      const pp::Rect& expected_rect) {
+    PDFiumPage* page = GetPDFiumPageForTest(engine, page_index);
+    ASSERT_TRUE(page);
+    CompareRect(expected_rect, page->rect());
+  }
+};
+
+TEST_F(PDFiumEngineTest, InitializeWithRectanglesMultiPagesPdf) {
+  NiceMock<MockTestClient> client;
+  EXPECT_CALL(client, DocumentSizeUpdated(pp::Size(343, 1664)));
+
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
+  ASSERT_TRUE(engine);
+  ASSERT_EQ(5, engine->GetNumberOfPages());
+
+  ExpectPageRect(engine.get(), 0, {38, 3, 266, 333});
+  ExpectPageRect(engine.get(), 1, {5, 350, 333, 266});
+  ExpectPageRect(engine.get(), 2, {38, 630, 266, 333});
+  ExpectPageRect(engine.get(), 3, {38, 977, 266, 333});
+  ExpectPageRect(engine.get(), 4, {38, 1324, 266, 333});
+}
+
+TEST_F(PDFiumEngineTest, AppendBlankPagesWithFewerPages) {
+  NiceMock<MockTestClient> client;
+  {
+    InSequence normal_then_append;
+    EXPECT_CALL(client, DocumentSizeUpdated(pp::Size(343, 1664)));
+    EXPECT_CALL(client, DocumentSizeUpdated(pp::Size(276, 1037)));
+  }
+
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
+  ASSERT_TRUE(engine);
+
+  engine->AppendBlankPages(3);
+  ASSERT_EQ(3, engine->GetNumberOfPages());
+
+  ExpectPageRect(engine.get(), 0, {5, 3, 266, 333});
+  ExpectPageRect(engine.get(), 1, {5, 350, 266, 333});
+  ExpectPageRect(engine.get(), 2, {5, 697, 266, 333});
+}
+
+TEST_F(PDFiumEngineTest, AppendBlankPagesWithMorePages) {
+  NiceMock<MockTestClient> client;
+  {
+    InSequence normal_then_append;
+    EXPECT_CALL(client, DocumentSizeUpdated(pp::Size(343, 1664)));
+    EXPECT_CALL(client, DocumentSizeUpdated(pp::Size(276, 2425)));
+  }
+
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
+  ASSERT_TRUE(engine);
+
+  engine->AppendBlankPages(7);
+  ASSERT_EQ(7, engine->GetNumberOfPages());
+
+  ExpectPageRect(engine.get(), 0, {5, 3, 266, 333});
+  ExpectPageRect(engine.get(), 1, {5, 350, 266, 333});
+  ExpectPageRect(engine.get(), 2, {5, 697, 266, 333});
+  ExpectPageRect(engine.get(), 3, {5, 1044, 266, 333});
+  ExpectPageRect(engine.get(), 4, {5, 1391, 266, 333});
+  ExpectPageRect(engine.get(), 5, {5, 1738, 266, 333});
+  ExpectPageRect(engine.get(), 6, {5, 2085, 266, 333});
+}
+
+}  // namespace
+}  // namespace chrome_pdf