Simplify PDFiumPage constructor

Drops PDFiumPage constructor arguments that are mutable after
construction. Long lists of constructor arguments are less clear than
named calls, and callers can avoid setting arguments they don't care
about. For example, the page rectangle can be initialized by a layout
calculation, rather than at construction time.

In the same spirit of removing undesirable degrees of freedom, the
set_available(boolean) method has been replaced with a one-way
MarkAvailable() method. (It doesn't make sense for an available page to
become unavailable.)

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

pdf/pdfium/pdfium_engine.cc

@@ -699,7 +699,7 @@ void PDFiumEngine::FinishLoadingDocument() {
     if (pages_[i]->available())
       continue;
 
-    pages_[i]->set_available(true);
+    pages_[i]->MarkAvailable();
     // We still need to call IsPageAvail() even if the whole document is
     // already downloaded.
     FPDFAvail_IsPageAvail(fpdf_availability(), i, &download_hints);
@@ -2255,8 +2255,8 @@ void PDFiumEngine::AppendBlankPages(size_t num_pages) {
                                             page_0_height_in_points));
     }
 
-    auto page =
-        std::make_unique<PDFiumPage>(this, i, pp::Rect(), /*available=*/true);
+    auto page = std::make_unique<PDFiumPage>(this, i);
+    page->MarkAvailable();
     pages_.push_back(std::move(page));
   }
 
@@ -2367,16 +2367,20 @@ void PDFiumEngine::ApplyCurrentLayoutToPages(bool reload) {
   for (size_t i = 0; i < layout_.page_count(); ++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
+      // The page is not marked as being available even if |doc_complete| is
       // true because FPDFAvail_IsPageAvail() still has to be called for this
       // page, which will be done in FinishLoadingDocument().
-      pages_.push_back(std::make_unique<PDFiumPage>(this, i, page_rect, false));
+      auto page = std::make_unique<PDFiumPage>(this, i);
+      page->set_rect(page_rect);
+      pages_.push_back(std::move(page));
     } else if (i < pages_.size()) {
       pages_[i]->set_rect(page_rect);
     } else {
-      bool available = FPDFAvail_IsPageAvail(fpdf_availability(), i, nullptr);
-      pages_.push_back(
-          std::make_unique<PDFiumPage>(this, i, page_rect, available));
+      auto page = std::make_unique<PDFiumPage>(this, i);
+      page->set_rect(page_rect);
+      if (FPDFAvail_IsPageAvail(fpdf_availability(), i, nullptr))
+        page->MarkAvailable();
+      pages_.push_back(std::move(page));
     }
   }
 }
@@ -2578,7 +2582,7 @@ bool PDFiumEngine::CheckPageAvailable(int index, std::vector<int>* pending) {
   }
 
   if (index < num_pages)
-    pages_[index]->set_available(true);
+    pages_[index]->MarkAvailable();
   if (default_page_size_.IsEmpty())
     default_page_size_ = GetPageSize(index);
   return true;

pdf/pdfium/pdfium_page.cc

@@ -192,14 +192,8 @@ PDFiumPage::LinkTarget::LinkTarget(const LinkTarget& other) = default;
 
 PDFiumPage::LinkTarget::~LinkTarget() = default;
 
-PDFiumPage::PDFiumPage(PDFiumEngine* engine,
-                       int i,
-                       const pp::Rect& r,
-                       bool available)
-    : engine_(engine),
-      index_(i),
-      rect_(r),
-      available_(available) {}
+PDFiumPage::PDFiumPage(PDFiumEngine* engine, int i)
+    : engine_(engine), index_(i), available_(false) {}
 
 PDFiumPage::PDFiumPage(PDFiumPage&& that) = default;
 

pdf/pdfium/pdfium_page.h

@@ -31,7 +31,7 @@ class PDFiumEngine;
 // Wrapper around a page from the document.
 class PDFiumPage {
  public:
-  PDFiumPage(PDFiumEngine* engine, int i, const pp::Rect& r, bool available);
+  PDFiumPage(PDFiumEngine* engine, int i);
   PDFiumPage(PDFiumPage&& that);
   ~PDFiumPage();
 
@@ -123,10 +123,15 @@ class PDFiumPage {
   const PDFEngine::PageFeatures* GetPageFeatures();
 
   int index() const { return index_; }
+
   const pp::Rect& rect() const { return rect_; }
   void set_rect(const pp::Rect& r) { rect_ = r; }
+
+  // Availability is a one-way transition: A page can become available, but it
+  // cannot become unavailable (unless deleted entirely).
   bool available() const { return available_; }
-  void set_available(bool available) { available_ = available; }
+  void MarkAvailable() { available_ = true; }
+
   void set_calculated_links(bool calculated_links) {
     calculated_links_ = calculated_links;
   }

pdf/pdfium/pdfium_page_unittest.cc

@@ -17,14 +17,14 @@ namespace chrome_pdf {
 
 namespace {
 
-TEST(PDFiumPageTest, ToPDFiumRotation) {
+TEST(PDFiumPageHelperTest, ToPDFiumRotation) {
   EXPECT_EQ(ToPDFiumRotation(PageOrientation::kOriginal), 0);
   EXPECT_EQ(ToPDFiumRotation(PageOrientation::kClockwise90), 1);
   EXPECT_EQ(ToPDFiumRotation(PageOrientation::kClockwise180), 2);
   EXPECT_EQ(ToPDFiumRotation(PageOrientation::kClockwise270), 3);
 }
 
-TEST(PDFiumPageDeathTest, ToPDFiumRotation) {
+TEST(PDFiumPageHelperDeathTest, ToPDFiumRotation) {
   PageOrientation invalid_orientation = static_cast<PageOrientation>(-1);
 #if DCHECK_IS_ON()
   EXPECT_DCHECK_DEATH(ToPDFiumRotation(invalid_orientation));
@@ -35,6 +35,15 @@ TEST(PDFiumPageDeathTest, ToPDFiumRotation) {
 
 }  // namespace
 
+using PDFiumPageTest = PDFiumTestBase;
+
+TEST_F(PDFiumPageTest, Constructor) {
+  PDFiumPage page(/*engine=*/nullptr, 2);
+  EXPECT_EQ(page.index(), 2);
+  EXPECT_TRUE(page.rect().IsEmpty());
+  EXPECT_FALSE(page.available());
+}
+
 class PDFiumPageLinkTest : public PDFiumTestBase {
  public:
   PDFiumPageLinkTest() = default;