Split options from chrome_pdf::DocumentLayout

Due to the loose, asynchronous coupling between the C++ and JavaScript
portions of the layout code, we need to pass certain layout "options"
(such as the default page orientation) back and forth by value.

As a result, it makes sense to have a separate Options class to hold
these settings, which then can be used to recalculate the layout.

The expected layout lifecycle eventually should look like this:
1. Update desired layout options one or more times.
2. Estimate layout size with new layout options.
3. Send options and estimated size to JavaScript.
4. Receive updated options from JavaScript.
5. Apply final options to current layout.
6. Repaint using updated layout.

As a side effect of this change, I added the desired_layout_options_
field to PDFiumEngine. This new field holds the mutable state about
page orientation that was split off into DocumentLayout::Options.

Miscellaneous changes:
* Revised some of the API comments for DocumentLayout
* Eliminated need for unsigned math by replacing -1 with +3 (mod 4)
* DocumentLayout no longer needs to be copyable, so it isn't

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

pdf/document_layout.cc

@@ -11,22 +11,27 @@ namespace chrome_pdf {
 const draw_utils::PageInsetSizes DocumentLayout::kSingleViewInsets{
     /*left=*/5, /*top=*/3, /*right=*/5, /*bottom=*/7};
 
-DocumentLayout::DocumentLayout() = default;
+DocumentLayout::Options::Options() = default;
 
-DocumentLayout::DocumentLayout(const DocumentLayout& other) = default;
-DocumentLayout& DocumentLayout::operator=(const DocumentLayout& other) =
-    default;
+DocumentLayout::Options::Options(const Options& other) = default;
+DocumentLayout::Options& DocumentLayout::Options::operator=(
+    const Options& other) = default;
 
-DocumentLayout::~DocumentLayout() = default;
+DocumentLayout::Options::~Options() = default;
 
-void DocumentLayout::RotatePagesClockwise() {
+void DocumentLayout::Options::RotatePagesClockwise() {
   default_page_orientation_ = (default_page_orientation_ + 1) % 4;
 }
 
-void DocumentLayout::RotatePagesCounterclockwise() {
-  default_page_orientation_ = (default_page_orientation_ - 1) % 4;
+void DocumentLayout::Options::RotatePagesCounterclockwise() {
+  // Use the modular additive inverse of -1 to avoid negative values.
+  default_page_orientation_ = (default_page_orientation_ + 3) % 4;
 }
 
+DocumentLayout::DocumentLayout() = default;
+
+DocumentLayout::~DocumentLayout() = default;
+
 std::vector<pp::Rect> DocumentLayout::GetTwoUpViewLayout(
     const std::vector<pp::Rect>& page_rects) {
   std::vector<pp::Rect> formatted_rects(page_rects.size());

pdf/document_layout.h

@@ -17,33 +17,57 @@ namespace chrome_pdf {
 // (possibly rotated) in a non-overlapping vertical sequence.
 //
 // All layout units are pixels.
+//
+// The |Options| class controls the behavior of the layout, such as the default
+// orientation of pages.
 class DocumentLayout final {
  public:
+  // Options controlling layout behavior.
+  class Options final {
+   public:
+    Options();
+
+    Options(const Options& other);
+    Options& operator=(const Options& other);
+
+    ~Options();
+
+    // Returns the default page orientation, encoded as an integer from 0 to 3
+    // (inclusive).
+    //
+    // A return value of 0 indicates the original page orientation, with each
+    // increment indicating clockwise rotation by an additional 90 degrees.
+    //
+    // TODO(kmoon): Return an enum (class) instead of an integer.
+    int default_page_orientation() const { return default_page_orientation_; }
+
+    // Rotates default page orientation 90 degrees clockwise.
+    void RotatePagesClockwise();
+
+    // Rotates default page orientation 90 degrees counterclockwise.
+    void RotatePagesCounterclockwise();
+
+   private:
+    // Orientations are non-negative integers modulo 4.
+    int default_page_orientation_ = 0;
+  };
+
   static const draw_utils::PageInsetSizes kSingleViewInsets;
   static constexpr int32_t kBottomSeparator = 4;
   static constexpr int32_t kHorizontalSeparator = 1;
 
   DocumentLayout();
 
-  DocumentLayout(const DocumentLayout& other);
-  DocumentLayout& operator=(const DocumentLayout& other);
+  DocumentLayout(const DocumentLayout& other) = delete;
+  DocumentLayout& operator=(const DocumentLayout& other) = delete;
 
   ~DocumentLayout();
 
-  // Returns an integer from 0 to 3 (inclusive), encoding the default
-  // orientation of the document's pages.
-  //
-  // A return value of 0 indicates the original page orientation, with each
-  // increment indicating clockwise rotation by an additional 90 degrees.
-  //
-  // TODO(kmoon): Return an enum (class) instead of an integer.
-  int default_page_orientation() const { return default_page_orientation_; }
-
-  // Rotates all pages 90 degrees clockwise. Does not recompute layout.
-  void RotatePagesClockwise();
+  // Returns the layout options.
+  const Options& options() const { return options_; }
 
-  // Rotates all pages 90 degrees counterclockwise. Does not recompute layout.
-  void RotatePagesCounterclockwise();
+  // Sets the layout options.
+  void set_options(const Options& options) { options_ = options; }
 
   // Returns the layout's total size.
   const pp::Size& size() const { return size_; }
@@ -66,12 +90,7 @@ class DocumentLayout final {
   void AppendPageRect(const pp::Size& page_rect);
 
  private:
-  // Orientations are non-negative integers modulo 4.
-  //
-  // TODO(kmoon): Doesn't match return type of default_page_orientation().
-  // Callers expect int, but internally, we want unsigned semantics. This will
-  // be cleaned up when we switch to an enum.
-  unsigned int default_page_orientation_ = 0;
+  Options options_;
 
   // Layout's total size.
   pp::Size size_;

pdf/document_layout_unittest.cc

@@ -11,6 +11,11 @@ namespace chrome_pdf {
 
 namespace {
 
+class DocumentLayoutOptionsTest : public testing::Test {
+ protected:
+  DocumentLayout::Options options_;
+};
+
 class DocumentLayoutTest : public testing::Test {
  protected:
   DocumentLayout layout_;
@@ -29,79 +34,73 @@ inline bool PpRectEq(const pp::Rect& lhs, const pp::Rect& rhs) {
   return lhs == rhs;
 }
 
-TEST_F(DocumentLayoutTest, DefaultConstructor) {
-  EXPECT_EQ(layout_.default_page_orientation(), 0);
-  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 0));
+TEST_F(DocumentLayoutOptionsTest, DefaultConstructor) {
+  EXPECT_EQ(options_.default_page_orientation(), 0);
 }
 
-TEST_F(DocumentLayoutTest, CopyConstructor) {
-  layout_.RotatePagesClockwise();
-  layout_.EnlargeHeight(2);
+TEST_F(DocumentLayoutOptionsTest, CopyConstructor) {
+  options_.RotatePagesClockwise();
 
-  DocumentLayout copy(layout_);
+  DocumentLayout::Options copy(options_);
   EXPECT_EQ(copy.default_page_orientation(), 1);
-  EXPECT_PRED2(PpSizeEq, copy.size(), pp::Size(0, 2));
 
-  layout_.RotatePagesClockwise();
-  layout_.EnlargeHeight(5);
+  options_.RotatePagesClockwise();
   EXPECT_EQ(copy.default_page_orientation(), 1);
-  EXPECT_PRED2(PpSizeEq, copy.size(), pp::Size(0, 2));
 }
 
-TEST_F(DocumentLayoutTest, CopyAssignment) {
-  layout_.RotatePagesClockwise();
-  layout_.EnlargeHeight(2);
+TEST_F(DocumentLayoutOptionsTest, CopyAssignment) {
+  options_.RotatePagesClockwise();
 
-  DocumentLayout copy;
+  DocumentLayout::Options copy;
   EXPECT_EQ(copy.default_page_orientation(), 0);
-  EXPECT_PRED2(PpSizeEq, copy.size(), pp::Size(0, 0));
-
-  copy = layout_;
+  copy = options_;
   EXPECT_EQ(copy.default_page_orientation(), 1);
-  EXPECT_PRED2(PpSizeEq, copy.size(), pp::Size(0, 2));
 
-  layout_.RotatePagesClockwise();
-  layout_.EnlargeHeight(5);
+  options_.RotatePagesClockwise();
   EXPECT_EQ(copy.default_page_orientation(), 1);
-  EXPECT_PRED2(PpSizeEq, copy.size(), pp::Size(0, 2));
 }
 
-TEST_F(DocumentLayoutTest, RotatePagesClockwise) {
-  layout_.RotatePagesClockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 1);
+TEST_F(DocumentLayoutOptionsTest, RotatePagesClockwise) {
+  options_.RotatePagesClockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 1);
 
-  layout_.RotatePagesClockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 2);
+  options_.RotatePagesClockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 2);
 
-  layout_.RotatePagesClockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 3);
+  options_.RotatePagesClockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 3);
 
-  layout_.RotatePagesClockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 0);
+  options_.RotatePagesClockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 0);
 }
 
-TEST_F(DocumentLayoutTest, RotatePagesCounterclockwise) {
-  layout_.RotatePagesCounterclockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 3);
+TEST_F(DocumentLayoutOptionsTest, RotatePagesCounterclockwise) {
+  options_.RotatePagesCounterclockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 3);
 
-  layout_.RotatePagesCounterclockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 2);
+  options_.RotatePagesCounterclockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 2);
 
-  layout_.RotatePagesCounterclockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 1);
+  options_.RotatePagesCounterclockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 1);
 
-  layout_.RotatePagesCounterclockwise();
-  EXPECT_EQ(layout_.default_page_orientation(), 0);
+  options_.RotatePagesCounterclockwise();
+  EXPECT_EQ(options_.default_page_orientation(), 0);
 }
 
-TEST_F(DocumentLayoutTest, RotatePagesDoesNotRecomputeLayout) {
-  layout_.EnlargeHeight(2);
+TEST_F(DocumentLayoutTest, DefaultConstructor) {
+  EXPECT_EQ(layout_.options().default_page_orientation(), 0);
+  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 0));
+}
 
-  layout_.RotatePagesClockwise();
-  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 2));
+TEST_F(DocumentLayoutTest, SetOptionsDoesNotRecomputeLayout) {
+  layout_.set_size(pp::Size(1, 2));
 
-  layout_.RotatePagesCounterclockwise();
-  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 2));
+  DocumentLayout::Options options;
+  options.RotatePagesClockwise();
+  layout_.set_options(options);
+  EXPECT_EQ(layout_.options().default_page_orientation(), 1);
+  EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(1, 2));
 }
 
 TEST_F(DocumentLayoutTest, EnlargeHeight) {

pdf/pdfium/pdfium_engine.cc

@@ -973,8 +973,8 @@ PDFiumPage::Area PDFiumEngine::GetCharIndex(const pp::Point& point,
 
   *page_index = page;
   PDFiumPage::Area result = pages_[page]->GetCharIndex(
-      point_in_page, layout_.default_page_orientation(), char_index, form_type,
-      target);
+      point_in_page, layout_.options().default_page_orientation(), char_index,
+      form_type, target);
   return (client_->IsPrintPreview() && result == PDFiumPage::WEBLINK_AREA)
              ? PDFiumPage::NONSELECTABLE_AREA
              : result;
@@ -1818,7 +1818,7 @@ bool PDFiumEngine::SelectFindResult(bool forward) {
   // Use zoom of 1.0 since |visible_rect| is without zoom.
   const std::vector<pp::Rect>& rects =
       find_results_[current_find_index_.value()].GetScreenRects(
-          pp::Point(), 1.0, layout_.default_page_orientation());
+          pp::Point(), 1.0, layout_.options().default_page_orientation());
   for (const auto& rect : rects)
     bounding_rect = bounding_rect.Union(rect);
   if (!visible_rect.Contains(bounding_rect)) {
@@ -1865,8 +1865,9 @@ void PDFiumEngine::GetAllScreenRectsUnion(
     std::vector<pp::Rect>* rect_vector) const {
   for (const auto& range : rect_range) {
     pp::Rect result_rect;
-    const std::vector<pp::Rect>& rects = range.GetScreenRects(
-        offset_point, current_zoom_, layout_.default_page_orientation());
+    const std::vector<pp::Rect>& rects =
+        range.GetScreenRects(offset_point, current_zoom_,
+                             layout_.options().default_page_orientation());
     for (const auto& rect : rects)
       result_rect = result_rect.Union(rect);
     rect_vector->push_back(result_rect);
@@ -1889,12 +1890,12 @@ void PDFiumEngine::ZoomUpdated(double new_zoom_level) {
 }
 
 void PDFiumEngine::RotateClockwise() {
-  layout_.RotatePagesClockwise();
+  desired_layout_options_.RotatePagesClockwise();
   RotateInternal();
 }
 
 void PDFiumEngine::RotateCounterclockwise() {
-  layout_.RotatePagesCounterclockwise();
+  desired_layout_options_.RotatePagesCounterclockwise();
   RotateInternal();
 }
 
@@ -2635,7 +2636,7 @@ pp::Size PDFiumEngine::GetPageSize(int index) {
         ConvertUnitDouble(width_in_points, kPointsPerInch, kPixelsPerInch));
     int height_in_pixels = static_cast<int>(
         ConvertUnitDouble(height_in_points, kPointsPerInch, kPixelsPerInch));
-    if (layout_.default_page_orientation() % 2 == 1)
+    if (layout_.options().default_page_orientation() % 2 == 1)
       std::swap(width_in_pixels, height_in_pixels);
     size = pp::Size(width_in_pixels, height_in_pixels);
   }
@@ -2732,7 +2733,8 @@ bool PDFiumEngine::ContinuePaint(int progressive_index,
                         0xFFFFFFFF);
     rv = FPDF_RenderPageBitmap_Start(
         new_bitmap.get(), page, start_x, start_y, size_x, size_y,
-        layout_.default_page_orientation(), GetRenderingFlags(), this);
+        layout_.options().default_page_orientation(), GetRenderingFlags(),
+        this);
     progressive_paints_[progressive_index].SetBitmapAndImageData(
         std::move(new_bitmap), *image_data);
   }
@@ -2759,7 +2761,7 @@ void PDFiumEngine::FinishPaint(int progressive_index,
 
   // Draw the forms.
   FPDF_FFLDraw(form(), bitmap, pages_[page_index]->GetPage(), start_x, start_y,
-               size_x, size_y, layout_.default_page_orientation(),
+               size_x, size_y, layout_.options().default_page_orientation(),
                GetRenderingFlags());
 
   FillPageSides(progressive_index);
@@ -2890,7 +2892,7 @@ void PDFiumEngine::DrawSelections(int progressive_index,
 
     const std::vector<pp::Rect>& rects =
         range.GetScreenRects(visible_rect.point(), current_zoom_,
-                             layout_.default_page_orientation());
+                             layout_.options().default_page_orientation());
     for (const auto& rect : rects) {
       pp::Rect visible_selection = rect.Intersect(dirty_in_screen);
       if (visible_selection.IsEmpty())
@@ -3111,9 +3113,9 @@ PDFiumEngine::SelectionChangeInvalidator::GetVisibleSelections() const {
     if (!engine_->IsPageVisible(range.page_index()))
       continue;
 
-    const std::vector<pp::Rect>& selection_rects =
-        range.GetScreenRects(visible_point, engine_->current_zoom_,
-                             engine_->layout_.default_page_orientation());
+    const std::vector<pp::Rect>& selection_rects = range.GetScreenRects(
+        visible_point, engine_->current_zoom_,
+        engine_->layout_.options().default_page_orientation());
     rects.insert(rects.end(), selection_rects.begin(), selection_rects.end());
   }
   return rects;
@@ -3172,8 +3174,9 @@ void PDFiumEngine::DeviceToPage(int page_index,
                                 pages_[page_index]->rect().y());
   FPDF_BOOL ret = FPDF_DeviceToPage(
       pages_[page_index]->GetPage(), 0, 0, pages_[page_index]->rect().width(),
-      pages_[page_index]->rect().height(), layout_.default_page_orientation(),
-      temp_x, temp_y, page_x, page_y);
+      pages_[page_index]->rect().height(),
+      layout_.options().default_page_orientation(), temp_x, temp_y, page_x,
+      page_y);
   DCHECK(ret);
 }
 
@@ -3281,7 +3284,7 @@ void PDFiumEngine::OnSelectionPositionChanged() {
   for (const auto& sel : selection_) {
     const std::vector<pp::Rect>& screen_rects =
         sel.GetScreenRects(GetVisibleRect().point(), current_zoom_,
-                           layout_.default_page_orientation());
+                           layout_.options().default_page_orientation());
     for (const auto& rect : screen_rects) {
       if (IsAboveOrDirectlyLeftOf(rect, left))
         left = rect;
@@ -3306,6 +3309,7 @@ void PDFiumEngine::RotateInternal() {
   // Save the current page.
   int most_visible_page = most_visible_page_;
 
+  layout_.set_options(desired_layout_options_);
   InvalidateAllPages();
 
   // Restore find results.

pdf/pdfium/pdfium_engine.h

@@ -529,6 +529,9 @@ class PDFiumEngine : public PDFEngine,
   // The current document layout.
   DocumentLayout layout_;
 
+  // The options for the desired document layout.
+  DocumentLayout::Options desired_layout_options_;
+
   // The scroll position in screen coordinates.
   pp::Point position_;
   // The offset of the page into the viewport.

pdf/pdfium/pdfium_form_filler.cc

@@ -100,7 +100,7 @@ void PDFiumFormFiller::Form_Invalidate(FPDF_FORMFILLINFO* param,
 
   pp::Rect rect = engine->pages_[page_index]->PageToScreen(
       engine->GetVisibleRect().point(), engine->current_zoom_, left, top, right,
-      bottom, engine->layout_.default_page_orientation());
+      bottom, engine->layout_.options().default_page_orientation());
   engine->client_->Invalidate(rect);
 }
 
@@ -119,7 +119,7 @@ void PDFiumFormFiller::Form_OutputSelectedRect(FPDF_FORMFILLINFO* param,
   }
   pp::Rect rect = engine->pages_[page_index]->PageToScreen(
       engine->GetVisibleRect().point(), engine->current_zoom_, left, top, right,
-      bottom, engine->layout_.default_page_orientation());
+      bottom, engine->layout_.options().default_page_orientation());
   if (rect.IsEmpty())
     return;