Clarify page rectangles in DocumentLayout

This change splits DocumentLayout::page_rect() into two methods,
page_rect() and page_bounds_rect(), to better mirror the corresponding
APIs in PDFEngine, GetPageRect() (page rectangle before insets) and
GetPageBoundsRect() (page rectangle after insets).

The existing page_rect() method is equivalent to PDFEngine's
GetPageBoundsRect(), which is confusing, so it has been renamed to
page_bounds_rect(). The new page_rect() method is equivalent to
PDFEngine's GetPageRect(). (This will be useful in a future change.)

DocumentLayout is now responsible for adding insets to the page
rectangles returned by draw_utils::GetRectForSingleView(), etc.,
eliminating the need to pass the insets to these functions.

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

pdf/document_layout.cc

@@ -19,6 +19,13 @@ int GetWidestPageWidth(const std::vector<pp::Size>& page_sizes) {
   return widest_page_width;
 }
 
+pp::Rect InsetRect(pp::Rect rect,
+                   const draw_utils::PageInsetSizes& inset_sizes) {
+  rect.Inset(inset_sizes.left, inset_sizes.top, inset_sizes.right,
+             inset_sizes.bottom);
+  return rect;
+}
+
 }  // namespace
 
 const draw_utils::PageInsetSizes DocumentLayout::kSingleViewInsets{
@@ -48,7 +55,7 @@ void DocumentLayout::ComputeSingleViewLayout(
     const std::vector<pp::Size>& page_sizes) {
   set_size({GetWidestPageWidth(page_sizes), 0});
 
-  page_rects_.resize(page_sizes.size());
+  page_layouts_.resize(page_sizes.size());
   for (size_t i = 0; i < page_sizes.size(); ++i) {
     if (i != 0) {
       // Add space for bottom separator.
@@ -56,8 +63,10 @@ void DocumentLayout::ComputeSingleViewLayout(
     }
 
     const pp::Size& page_size = page_sizes[i];
-    page_rects_[i] =
-        draw_utils::GetRectForSingleView(page_size, size_, kSingleViewInsets);
+    pp::Rect page_rect = draw_utils::GetRectForSingleView(page_size, size_);
+    page_layouts_[i].outer_rect = page_rect;
+    page_layouts_[i].inner_rect = InsetRect(page_rect, kSingleViewInsets);
+
     draw_utils::ExpandDocumentSize(page_size, &size_);
   }
 }
@@ -66,21 +75,24 @@ void DocumentLayout::ComputeTwoUpViewLayout(
     const std::vector<pp::Size>& page_sizes) {
   set_size({GetWidestPageWidth(page_sizes), 0});
 
-  page_rects_.resize(page_sizes.size());
+  page_layouts_.resize(page_sizes.size());
   for (size_t i = 0; i < page_sizes.size(); ++i) {
     draw_utils::PageInsetSizes page_insets =
         draw_utils::GetPageInsetsForTwoUpView(
             i, page_sizes.size(), kSingleViewInsets, kHorizontalSeparator);
     const pp::Size& page_size = page_sizes[i];
 
+    pp::Rect page_rect;
     if (i % 2 == 0) {
-      page_rects_[i] = draw_utils::GetLeftRectForTwoUpView(
-          page_size, {size_.width(), size_.height()}, page_insets);
+      page_rect = draw_utils::GetLeftRectForTwoUpView(
+          page_size, {size_.width(), size_.height()});
     } else {
-      page_rects_[i] = draw_utils::GetRightRectForTwoUpView(
-          page_size, {size_.width(), size_.height()}, page_insets);
+      page_rect = draw_utils::GetRightRectForTwoUpView(
+          page_size, {size_.width(), size_.height()});
       EnlargeHeight(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) {

pdf/document_layout.h

@@ -74,12 +74,19 @@ class DocumentLayout final {
   // TODO(kmoon): Get rid of this method.
   void set_size(const pp::Size& size) { size_ = size; }
 
-  size_t page_count() const { return page_rects_.size(); }
+  size_t page_count() const { return page_layouts_.size(); }
 
   // Gets the layout rectangle for a page. Only valid after computing a layout.
   const pp::Rect& page_rect(size_t page_index) const {
     DCHECK_LT(page_index, page_count());
-    return page_rects_[page_index];
+    return page_layouts_[page_index].outer_rect;
+  }
+
+  // Gets the layout rectangle for a page's bounds (which excludes additional
+  // regions like page shadows). Only valid after computing a layout.
+  const pp::Rect& page_bounds_rect(size_t page_index) const {
+    DCHECK_LT(page_index, page_count());
+    return page_layouts_[page_index].inner_rect;
   }
 
   // Computes layout that represent |page_sizes| formatted for single view.
@@ -98,13 +105,21 @@ class DocumentLayout final {
   void EnlargeHeight(int height);
 
  private:
+  // Layout of a single page.
+  struct PageLayout {
+    // Bounding rectangle for the page with decorations.
+    pp::Rect outer_rect;
+
+    // Bounding rectangle for the page without decorations.
+    pp::Rect inner_rect;
+  };
+
   Options options_;
 
   // Layout's total size.
   pp::Size size_;
 
-  // Page layout rectangles.
-  std::vector<pp::Rect> page_rects_;
+  std::vector<PageLayout> page_layouts_;
 };
 
 }  // namespace chrome_pdf

pdf/document_layout_unittest.cc

@@ -123,20 +123,38 @@ TEST_F(DocumentLayoutTest, ComputeSingleViewLayout) {
       {300, 400}, {400, 500}, {300, 400}, {200, 300}};
   layout_.ComputeSingleViewLayout(page_sizes);
   ASSERT_EQ(4u, layout_.page_count());
-  EXPECT_PRED2(PpRectEq, pp::Rect(55, 3, 290, 390), layout_.page_rect(0));
-  EXPECT_PRED2(PpRectEq, pp::Rect(5, 407, 390, 490), layout_.page_rect(1));
-  EXPECT_PRED2(PpRectEq, pp::Rect(55, 911, 290, 390), layout_.page_rect(2));
-  EXPECT_PRED2(PpRectEq, pp::Rect(105, 1315, 190, 290), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(50, 0, 300, 400), layout_.page_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(0, 404, 400, 500), layout_.page_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(50, 908, 300, 400), layout_.page_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(100, 1312, 200, 300), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(55, 3, 290, 390),
+               layout_.page_bounds_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(5, 407, 390, 490),
+               layout_.page_bounds_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(55, 911, 290, 390),
+               layout_.page_bounds_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(105, 1315, 190, 290),
+               layout_.page_bounds_rect(3));
   EXPECT_PRED2(PpSizeEq, pp::Size(400, 1612), layout_.size());
 
   page_sizes = {{240, 300}, {320, 400}, {250, 360}, {300, 600}, {270, 555}};
   layout_.ComputeSingleViewLayout(page_sizes);
   ASSERT_EQ(5u, layout_.page_count());
-  EXPECT_PRED2(PpRectEq, pp::Rect(45, 3, 230, 290), layout_.page_rect(0));
-  EXPECT_PRED2(PpRectEq, pp::Rect(5, 307, 310, 390), layout_.page_rect(1));
-  EXPECT_PRED2(PpRectEq, pp::Rect(40, 711, 240, 350), layout_.page_rect(2));
-  EXPECT_PRED2(PpRectEq, pp::Rect(15, 1075, 290, 590), layout_.page_rect(3));
-  EXPECT_PRED2(PpRectEq, pp::Rect(30, 1679, 260, 545), layout_.page_rect(4));
+  EXPECT_PRED2(PpRectEq, pp::Rect(40, 0, 240, 300), layout_.page_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(0, 304, 320, 400), layout_.page_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(35, 708, 250, 360), layout_.page_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(10, 1072, 300, 600), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(25, 1676, 270, 555), layout_.page_rect(4));
+  EXPECT_PRED2(PpRectEq, pp::Rect(45, 3, 230, 290),
+               layout_.page_bounds_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(5, 307, 310, 390),
+               layout_.page_bounds_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(40, 711, 240, 350),
+               layout_.page_bounds_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(15, 1075, 290, 590),
+               layout_.page_bounds_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(30, 1679, 260, 545),
+               layout_.page_bounds_rect(4));
   EXPECT_PRED2(PpSizeEq, pp::Size(320, 2231), layout_.size());
 }
 
@@ -146,31 +164,57 @@ TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
       {826, 1066}, {1066, 826}, {826, 1066}, {826, 900}};
   layout_.ComputeTwoUpViewLayout(page_sizes);
   ASSERT_EQ(4u, layout_.page_count());
-  EXPECT_PRED2(PpRectEq, pp::Rect(245, 3, 820, 1056), layout_.page_rect(0));
-  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 3, 1060, 816), layout_.page_rect(1));
-  EXPECT_PRED2(PpRectEq, pp::Rect(245, 1069, 820, 1056), layout_.page_rect(2));
-  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 1069, 820, 890), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(240, 0, 826, 1066), layout_.page_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1066, 0, 1066, 826), layout_.page_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(240, 1066, 826, 1066), layout_.page_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1066, 1066, 826, 900), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(245, 3, 820, 1056),
+               layout_.page_bounds_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 3, 1060, 816),
+               layout_.page_bounds_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(245, 1069, 820, 1056),
+               layout_.page_bounds_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 1069, 820, 890),
+               layout_.page_bounds_rect(3));
   EXPECT_PRED2(PpSizeEq, pp::Size(2132, 2132), layout_.size());
 
   // Test case where the widest page is on the left.
   page_sizes = {{1066, 826}, {820, 1056}, {820, 890}, {826, 1066}};
   layout_.ComputeTwoUpViewLayout(page_sizes);
   ASSERT_EQ(4u, layout_.page_count());
-  EXPECT_PRED2(PpRectEq, pp::Rect(5, 3, 1060, 816), layout_.page_rect(0));
-  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 3, 814, 1046), layout_.page_rect(1));
-  EXPECT_PRED2(PpRectEq, pp::Rect(251, 1059, 814, 880), layout_.page_rect(2));
-  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 1059, 820, 1056), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(0, 0, 1066, 826), layout_.page_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1066, 0, 820, 1056), layout_.page_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(246, 1056, 820, 890), layout_.page_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1066, 1056, 826, 1066), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(5, 3, 1060, 816),
+               layout_.page_bounds_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 3, 814, 1046),
+               layout_.page_bounds_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(251, 1059, 814, 880),
+               layout_.page_bounds_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(1067, 1059, 820, 1056),
+               layout_.page_bounds_rect(3));
   EXPECT_PRED2(PpSizeEq, pp::Size(2132, 2122), layout_.size());
 
   // 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);
   ASSERT_EQ(5u, layout_.page_count());
-  EXPECT_PRED2(PpRectEq, pp::Rect(205, 3, 194, 290), layout_.page_rect(0));
-  EXPECT_PRED2(PpRectEq, pp::Rect(401, 3, 394, 190), layout_.page_rect(1));
-  EXPECT_PRED2(PpRectEq, pp::Rect(105, 303, 294, 590), layout_.page_rect(2));
-  EXPECT_PRED2(PpRectEq, pp::Rect(401, 303, 244, 490), layout_.page_rect(3));
-  EXPECT_PRED2(PpRectEq, pp::Rect(105, 903, 290, 390), layout_.page_rect(4));
+  EXPECT_PRED2(PpRectEq, pp::Rect(200, 0, 200, 300), layout_.page_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(400, 0, 400, 200), layout_.page_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(100, 300, 300, 600), layout_.page_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(400, 300, 250, 500), layout_.page_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(100, 900, 300, 400), layout_.page_rect(4));
+  EXPECT_PRED2(PpRectEq, pp::Rect(205, 3, 194, 290),
+               layout_.page_bounds_rect(0));
+  EXPECT_PRED2(PpRectEq, pp::Rect(401, 3, 394, 190),
+               layout_.page_bounds_rect(1));
+  EXPECT_PRED2(PpRectEq, pp::Rect(105, 303, 294, 590),
+               layout_.page_bounds_rect(2));
+  EXPECT_PRED2(PpRectEq, pp::Rect(401, 303, 244, 490),
+               layout_.page_bounds_rect(3));
+  EXPECT_PRED2(PpRectEq, pp::Rect(105, 903, 290, 390),
+               layout_.page_bounds_rect(4));
   EXPECT_PRED2(PpSizeEq, pp::Size(800, 1300), layout_.size());
 }
 

pdf/draw_utils/coordinates.cc

@@ -81,13 +81,9 @@ PageInsetSizes GetPageInsetsForTwoUpView(
 }
 
 pp::Rect GetRectForSingleView(const pp::Size& rect_size,
-                              const pp::Size& document_size,
-                              const PageInsetSizes& page_insets) {
+                              const pp::Size& document_size) {
   pp::Rect page_rect({0, document_size.height()}, rect_size);
   CenterRectHorizontally(document_size.width(), &page_rect);
-  page_rect.Inset(page_insets.left, page_insets.top, page_insets.right,
-                  page_insets.bottom);
-
   return page_rect;
 }
 
@@ -147,25 +143,17 @@ pp::Rect GetBottomFillRect(const pp::Rect& page_rect,
 }
 
 pp::Rect GetLeftRectForTwoUpView(const pp::Size& rect_size,
-                                 const pp::Point& position,
-                                 const PageInsetSizes& page_insets) {
+                                 const pp::Point& position) {
   DCHECK_LE(rect_size.width(), position.x());
 
-  pp::Rect left_rect(position.x() - rect_size.width(), position.y(),
-                     rect_size.width(), rect_size.height());
-  left_rect.Inset(page_insets.left, page_insets.top, page_insets.right,
-                  page_insets.bottom);
-  return left_rect;
+  return pp::Rect(position.x() - rect_size.width(), position.y(),
+                  rect_size.width(), rect_size.height());
 }
 
 pp::Rect GetRightRectForTwoUpView(const pp::Size& rect_size,
-                                  const pp::Point& position,
-                                  const PageInsetSizes& page_insets) {
-  pp::Rect right_rect(position.x(), position.y(), rect_size.width(),
-                      rect_size.height());
-  right_rect.Inset(page_insets.left, page_insets.top, page_insets.right,
-                   page_insets.bottom);
-  return right_rect;
+                                  const pp::Point& position) {
+  return pp::Rect(position.x(), position.y(), rect_size.width(),
+                  rect_size.height());
 }
 
 }  // namespace draw_utils

pdf/draw_utils/coordinates.h

@@ -75,11 +75,9 @@ PageInsetSizes GetPageInsetsForTwoUpView(
     int horizontal_separator);
 
 // Given |rect_size| and |document_size| create a horizontally centered
-// pp::Rect placed at the bottom of the current document, and then inset it with
-// |page_insets|.
+// pp::Rect placed at the bottom of the current document.
 pp::Rect GetRectForSingleView(const pp::Size& rect_size,
-                              const pp::Size& document_size,
-                              const PageInsetSizes& page_insets);
+                              const pp::Size& document_size);
 
 // Given |rect| in document coordinates, a |position| in screen coordinates,
 // and a |zoom| factor, returns the rectangle in screen coordinates (i.e.
@@ -125,20 +123,15 @@ pp::Rect GetBottomFillRect(const pp::Rect& page_rect,
                            int bottom_separator);
 
 // Given |rect_size|, create a pp::Rect where the top-right corner lies at
-// |position|, and then inset it with the corresponding values of |page_insets|,
-// i.e. inset on left side with |page_insets.left|.
-// The width of |rect_size| must be less than or equal to the x value for
-// |position|.
+// |position|. The width of |rect_size| must be less than or equal to the x
+// value for |position|.
 pp::Rect GetLeftRectForTwoUpView(const pp::Size& rect_size,
-                                 const pp::Point& position,
-                                 const PageInsetSizes& page_insets);
+                                 const pp::Point& position);
 
 // Given |rect_size|, create a pp::Rect where the top-left corner lies at
-// |position|, and then inset it with the corresponding values of |page_insets|,
-// i.e. inset on left side with |page_insets.left|.
+// |position|.
 pp::Rect GetRightRectForTwoUpView(const pp::Size& rect_size,
-                                  const pp::Point& position,
-                                  const PageInsetSizes& page_insets);
+                                  const pp::Point& position);
 
 }  // namespace draw_utils
 }  // namespace chrome_pdf

pdf/draw_utils/coordinates_unittest.cc

@@ -151,16 +151,16 @@ TEST(CoordinateTest, GetPageInsetsForTwoUpView) {
 
 TEST(CoordinateTest, GetRectForSingleView) {
   // Test portrait pages.
-  CompareRect({55, 503, 190, 390},
-              GetRectForSingleView({200, 400}, {300, 500}, kSingleViewInsets));
-  CompareRect({55, 603, 90, 330},
-              GetRectForSingleView({100, 340}, {200, 600}, kSingleViewInsets));
+  CompareRect({50, 500, 200, 400},
+              GetRectForSingleView({200, 400}, {300, 500}));
+  CompareRect({50, 600, 100, 340},
+              GetRectForSingleView({100, 340}, {200, 600}));
 
   // Test landscape pages.
-  CompareRect({5, 1003, 490, 440},
-              GetRectForSingleView({500, 450}, {500, 1000}, kSingleViewInsets));
-  CompareRect({30, 1503, 640, 190},
-              GetRectForSingleView({650, 200}, {700, 1500}, kSingleViewInsets));
+  CompareRect({0, 1000, 500, 450},
+              GetRectForSingleView({500, 450}, {500, 1000}));
+  CompareRect({25, 1500, 650, 200},
+              GetRectForSingleView({650, 200}, {700, 1500}));
 }
 
 TEST(CoordinateTest, GetScreenRect) {
@@ -275,33 +275,22 @@ TEST(CoordinateTest, GetBottomFillRect) {
 }
 
 TEST(CoordinateTest, GetLeftRectForTwoUpView) {
-  CompareRect({105, 103, 194, 390},
-              GetLeftRectForTwoUpView({200, 400}, {300, 100}, kLeftInsets));
-  CompareRect({5, 3, 294, 390},
-              GetLeftRectForTwoUpView({300, 400}, {300, 0}, kLeftInsets));
-
-  // Test rect smaller than shadow insets returns empty rect.
-  CompareRect({10, 3, 0, 0},
-              GetLeftRectForTwoUpView({5, 5}, {10, 0}, kLeftInsets));
+  CompareRect({100, 100, 200, 400},
+              GetLeftRectForTwoUpView({200, 400}, {300, 100}));
+  CompareRect({0, 0, 300, 400}, GetLeftRectForTwoUpView({300, 400}, {300, 0}));
 
   // Test empty rect gets positioned.
-  CompareRect({105, 3, 0, 0},
-              GetLeftRectForTwoUpView({0, 0}, {100, 0}, kLeftInsets));
+  CompareRect({100, 0, 0, 0}, GetLeftRectForTwoUpView({0, 0}, {100, 0}));
 }
 
 TEST(CoordinateTest, GetRightRectForTwoUpView) {
-  CompareRect({301, 103, 194, 390},
-              GetRightRectForTwoUpView({200, 400}, {300, 100}, kRightInsets));
-  CompareRect({301, 3, 294, 390},
-              GetRightRectForTwoUpView({300, 400}, {300, 0}, kRightInsets));
-
-  // Test rect smaller than shadow insets returns empty rect.
-  CompareRect({11, 3, 0, 0},
-              GetRightRectForTwoUpView({5, 5}, {10, 0}, kRightInsets));
+  CompareRect({300, 100, 200, 400},
+              GetRightRectForTwoUpView({200, 400}, {300, 100}));
+  CompareRect({300, 0, 300, 400},
+              GetRightRectForTwoUpView({300, 400}, {300, 0}));
 
   // Test empty rect gets positioned.
-  CompareRect({101, 3, 0, 0},
-              GetRightRectForTwoUpView({0, 0}, {100, 0}, kRightInsets));
+  CompareRect({100, 0, 0, 0}, GetRightRectForTwoUpView({0, 0}, {100, 0}));
 }
 
 }  // namespace draw_utils

pdf/pdfium/pdfium_engine.cc

@@ -2403,7 +2403,7 @@ void PDFiumEngine::LoadPagesInCurrentLayout(std::vector<pp::Size> page_sizes,
 // 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) {
-    const pp::Rect& page_rect = layout_.page_rect(i);
+    const pp::Rect& page_rect = layout_.page_bounds_rect(i);
     if (!reload) {
       // The page is marked as not being available even if |doc_complete| is
       // true because FPDFAvail_IsPageAvail() still has to be called for this