Increase precision of PDF layout dirty tracking

DocumentLayout now sets a "dirty" bit if (and only if) the layout
changed. The current behavior only looks at changes to the overall
document size, which is inaccurate in cases such as orientation changes
that may change the layout of individual pages, but not the overall
document size (for certain combinations of page sizes).

This doesn't matter in the current code (the DocumentSizeUpdated event
might not fire, which is mostly harmless), but the fix for bug 885110
requires accurately detecting and reporting layout changes to the
viewer's JavaScript code, so DocumentLayout has been instrumented to
detect all current triggers for layout changes:

* Default page orientation changes
* Overall page count changes
* Overall document size changes (possibly redundant)
* Individual page layout rectangle changes

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

pdf/document_layout.cc

@@ -5,6 +5,8 @@
 #include "pdf/document_layout.h"
 
 #include "base/logging.h"
+#include "ppapi/cpp/rect.h"
+#include "ppapi/cpp/size.h"
 
 namespace chrome_pdf {
 
@@ -51,31 +53,62 @@ DocumentLayout::DocumentLayout() = default;
 
 DocumentLayout::~DocumentLayout() = default;
 
+void DocumentLayout::SetOptions(const Options& options) {
+  // TODO(kmoon): This is pessimistic, but we still want to consider the layout
+  // dirty for orientation changes, even if the page rects don't change.
+  //
+  // We also probably don't want orientation changes to actually kick in until
+  // the next call to ComputeLayout(). (In practice, we'll call ComputeLayout()
+  // shortly after calling SetOptions().)
+  if (options_.default_page_orientation() !=
+      options.default_page_orientation()) {
+    dirty_ = true;
+  }
+  options_ = options;
+}
+
 void DocumentLayout::ComputeSingleViewLayout(
     const std::vector<pp::Size>& page_sizes) {
-  size_ = {GetWidestPageWidth(page_sizes), 0};
+  pp::Size document_size(GetWidestPageWidth(page_sizes), 0);
+
+  if (page_layouts_.size() != page_sizes.size()) {
+    // TODO(kmoon): May want to do less work when shrinking a layout.
+    page_layouts_.resize(page_sizes.size());
+    dirty_ = true;
+  }
 
-  page_layouts_.resize(page_sizes.size());
   for (size_t i = 0; i < page_sizes.size(); ++i) {
     if (i != 0) {
       // Add space for bottom separator.
-      size_.Enlarge(0, kBottomSeparator);
+      document_size.Enlarge(0, kBottomSeparator);
     }
 
     const pp::Size& page_size = page_sizes[i];
-    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);
+    pp::Rect page_rect =
+        draw_utils::GetRectForSingleView(page_size, document_size);
+    CopyRectIfModified(page_rect, &page_layouts_[i].outer_rect);
+    CopyRectIfModified(InsetRect(page_rect, kSingleViewInsets),
+                       &page_layouts_[i].inner_rect);
 
-    draw_utils::ExpandDocumentSize(page_size, &size_);
+    draw_utils::ExpandDocumentSize(page_size, &document_size);
+  }
+
+  if (size_ != document_size) {
+    size_ = document_size;
+    dirty_ = true;
   }
 }
 
 void DocumentLayout::ComputeTwoUpViewLayout(
     const std::vector<pp::Size>& page_sizes) {
-  size_ = {GetWidestPageWidth(page_sizes), 0};
+  pp::Size document_size(GetWidestPageWidth(page_sizes), 0);
+
+  if (page_layouts_.size() != page_sizes.size()) {
+    // TODO(kmoon): May want to do less work when shrinking a layout.
+    page_layouts_.resize(page_sizes.size());
+    dirty_ = true;
+  }
 
-  page_layouts_.resize(page_sizes.size());
   for (size_t i = 0; i < page_sizes.size(); ++i) {
     draw_utils::PageInsetSizes page_insets =
         draw_utils::GetPageInsetsForTwoUpView(
@@ -85,22 +118,36 @@ void DocumentLayout::ComputeTwoUpViewLayout(
     pp::Rect page_rect;
     if (i % 2 == 0) {
       page_rect = draw_utils::GetLeftRectForTwoUpView(
-          page_size, {size_.width(), size_.height()});
+          page_size, {document_size.width(), document_size.height()});
     } else {
       page_rect = draw_utils::GetRightRectForTwoUpView(
-          page_size, {size_.width(), size_.height()});
-      size_.Enlarge(0,
-                    std::max(page_size.height(), page_sizes[i - 1].height()));
+          page_size, {document_size.width(), document_size.height()});
+      document_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);
+    CopyRectIfModified(page_rect, &page_layouts_[i].outer_rect);
+    CopyRectIfModified(InsetRect(page_rect, page_insets),
+                       &page_layouts_[i].inner_rect);
   }
 
   if (page_sizes.size() % 2 == 1) {
-    size_.Enlarge(0, page_sizes.back().height());
+    document_size.Enlarge(0, page_sizes.back().height());
   }
 
-  size_.set_width(2 * size_.width());
+  document_size.set_width(2 * document_size.width());
+
+  if (size_ != document_size) {
+    size_ = document_size;
+    dirty_ = true;
+  }
+}
+
+void DocumentLayout::CopyRectIfModified(const pp::Rect& source_rect,
+                                        pp::Rect* destination_rect) {
+  if (*destination_rect != source_rect) {
+    *destination_rect = source_rect;
+    dirty_ = true;
+  }
 }
 
 }  // namespace chrome_pdf

pdf/document_layout.h

@@ -63,8 +63,20 @@ class DocumentLayout final {
   // Returns the layout options.
   const Options& options() const { return options_; }
 
-  // Sets the layout options.
-  void set_options(const Options& options) { options_ = options; }
+  // Sets the layout options. If certain options with immediate effect change
+  // (such as the default page orientation), the layout will be marked dirty.
+  //
+  // TODO(kmoon): We shouldn't have layout options that take effect immediately.
+  void SetOptions(const Options& options);
+
+  // Returns true if the layout has been modified since the last call to
+  // clear_dirty(). The initial state is false (clean), which assumes
+  // appropriate default behavior for an initially empty layout.
+  bool dirty() const { return dirty_; }
+
+  // Clears the dirty() state of the layout. This should be called after any
+  // layout changes have been applied.
+  void clear_dirty() { dirty_ = false; }
 
   // Returns the layout's total size.
   const pp::Size& size() const { return size_; }
@@ -104,8 +116,22 @@ class DocumentLayout final {
     pp::Rect inner_rect;
   };
 
+  // Copies |source_rect| to |destination_rect|, setting |dirty_| to true if
+  // |destination_rect| is modified as a result.
+  void CopyRectIfModified(const pp::Rect& source_rect,
+                          pp::Rect* destination_rect);
+
   Options options_;
 
+  // Indicates if the layout has changed in an externally-observable way,
+  // usually as a result of calling |ComputeLayout()| with different inputs.
+  //
+  // Some operations that may trigger layout changes:
+  // * Changing page sizes
+  // * Adding or removing pages
+  // * Changing page orientations
+  bool dirty_ = false;
+
   // Layout's total size.
   pp::Size size_;
 

pdf/document_layout_unittest.cc

@@ -92,6 +92,7 @@ TEST_F(DocumentLayoutOptionsTest, RotatePagesCounterclockwise) {
 TEST_F(DocumentLayoutTest, DefaultConstructor) {
   EXPECT_EQ(layout_.options().default_page_orientation(),
             PageOrientation::kOriginal);
+  EXPECT_FALSE(layout_.dirty());
   EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(0, 0));
   EXPECT_EQ(layout_.page_count(), 0u);
 }
@@ -102,12 +103,22 @@ TEST_F(DocumentLayoutTest, SetOptionsDoesNotRecomputeLayout) {
 
   DocumentLayout::Options options;
   options.RotatePagesClockwise();
-  layout_.set_options(options);
+  layout_.SetOptions(options);
   EXPECT_EQ(layout_.options().default_page_orientation(),
             PageOrientation::kClockwise90);
   EXPECT_PRED2(PpSizeEq, layout_.size(), pp::Size(100, 200));
 }
 
+TEST_F(DocumentLayoutTest, DirtySetOnOrientationChange) {
+  DocumentLayout::Options options;
+  layout_.SetOptions(options);
+  EXPECT_FALSE(layout_.dirty());
+
+  options.RotatePagesClockwise();
+  layout_.SetOptions(options);
+  EXPECT_TRUE(layout_.dirty());
+}
+
 TEST_F(DocumentLayoutTest, ComputeSingleViewLayout) {
   std::vector<pp::Size> page_sizes{
       {300, 400}, {400, 500}, {300, 400}, {200, 300}};
@@ -208,6 +219,49 @@ TEST_F(DocumentLayoutTest, ComputeTwoUpViewLayout) {
   EXPECT_PRED2(PpSizeEq, pp::Size(800, 1300), layout_.size());
 }
 
+TEST_F(DocumentLayoutTest, DirtySetOnSingleViewLayoutInputChange) {
+  layout_.ComputeSingleViewLayout({pp::Size(100, 200)});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+  EXPECT_FALSE(layout_.dirty());
+
+  layout_.ComputeSingleViewLayout({pp::Size(100, 200)});
+  EXPECT_FALSE(layout_.dirty());
+
+  layout_.ComputeSingleViewLayout({pp::Size(200, 100)});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+
+  layout_.ComputeSingleViewLayout({pp::Size(200, 100), pp::Size(300, 300)});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+
+  layout_.ComputeSingleViewLayout({pp::Size(200, 100)});
+  EXPECT_TRUE(layout_.dirty());
+}
+
+TEST_F(DocumentLayoutTest, DirtySetOnTwoUpViewLayoutInputChange) {
+  layout_.ComputeTwoUpViewLayout({pp::Size(100, 200), pp::Size(200, 100)});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+  EXPECT_FALSE(layout_.dirty());
+
+  layout_.ComputeTwoUpViewLayout({pp::Size(100, 200), pp::Size(200, 100)});
+  EXPECT_FALSE(layout_.dirty());
+
+  layout_.ComputeTwoUpViewLayout({pp::Size(200, 100), pp::Size(100, 200)});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+
+  layout_.ComputeTwoUpViewLayout(
+      {pp::Size(200, 100), pp::Size(100, 200), pp::Size(300, 300)});
+  EXPECT_TRUE(layout_.dirty());
+  layout_.clear_dirty();
+
+  layout_.ComputeTwoUpViewLayout({pp::Size(200, 100), pp::Size(100, 200)});
+  EXPECT_TRUE(layout_.dirty());
+}
+
 }  // namespace
 
 }  // namespace chrome_pdf

pdf/pdfium/pdfium_engine.cc

@@ -2475,7 +2475,6 @@ void PDFiumEngine::LoadPageInfo(bool reload) {
   if (pages_.empty() && reload)
     return;
   pending_pages_.clear();
-  pp::Size old_document_size = layout_.size();
   std::vector<pp::Size> page_sizes;
   size_t new_page_count = FPDF_GetPageCount(doc());
 
@@ -2522,8 +2521,10 @@ void PDFiumEngine::LoadPageInfo(bool reload) {
   }
 
   CalculateVisiblePages();
-  if (layout_.size() != old_document_size)
+  if (layout_.dirty()) {
+    layout_.clear_dirty();
     client_->DocumentSizeUpdated(layout_.size());
+  }
 }
 
 void PDFiumEngine::LoadBody() {
@@ -3368,7 +3369,7 @@ void PDFiumEngine::RotateInternal() {
   // Save the current page.
   int most_visible_page = most_visible_page_;
 
-  layout_.set_options(desired_layout_options_);
+  layout_.SetOptions(desired_layout_options_);
   InvalidateAllPages();
 
   // Restore find results.