[unseasoned-pdf] Move DidOpen() to PdfViewPluginBase

Consolidates DidOpen() implementations in PdfViewPluginBase. The
(unseasoned) PdfViewWebPlugin implementation diverges unnecessarily from
the (Peppery) OutOfProcessInstance implementation.

Also restricts PdfViewPluginBase::document_load_state_ visibility, since
this field only needs to be accessible for testing now.

Bug: 1099022
Change-Id: Id5be4726e2ff208454f85db9d0c2afb3deb8fb26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3200180
Auto-Submit: K. Moon <kmoon@chromium.org>
Commit-Queue: Hui Yingst <nigi@chromium.org>
Reviewed-by: Hui Yingst <nigi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#927709}

pdf/out_of_process_instance.cc

@@ -680,18 +680,6 @@ void OutOfProcessInstance::StopFind() {
   PdfViewPluginBase::StopFind();
 }
 
-void OutOfProcessInstance::DidOpen(std::unique_ptr<UrlLoader> loader,
-                                   int32_t result) {
-  if (result == PP_OK) {
-    if (!engine()->HandleDocumentLoad(std::move(loader), GetURL())) {
-      set_document_load_state(DocumentLoadState::kLoading);
-      DocumentLoadFailed();
-    }
-  } else if (result != PP_ERROR_ABORTED) {  // Can happen in tests.
-    DocumentLoadFailed();
-  }
-}
-
 void OutOfProcessInstance::SendMessage(base::Value message) {
   PostMessage(VarFromValue(message));
 }

pdf/out_of_process_instance.h

@@ -119,7 +119,6 @@ class OutOfProcessInstance : public PdfViewPluginBase,
   // PdfViewPluginBase:
   base::WeakPtr<PdfViewPluginBase> GetWeakPtr() override;
   std::unique_ptr<UrlLoader> CreateUrlLoaderInternal() override;
-  void DidOpen(std::unique_ptr<UrlLoader> loader, int32_t result) override;
   void SendMessage(base::Value message) override;
   void SaveAs() override;
   void InitImageData(const gfx::Size& size) override;

pdf/pdf_view_plugin_base.cc

@@ -1617,9 +1617,21 @@ void PdfViewPluginBase::HistogramCustomCounts(const char* name,
   base::UmaHistogramCustomCounts(name, sample, min, max, bucket_count);
 }
 
+void PdfViewPluginBase::DidOpen(std::unique_ptr<UrlLoader> loader,
+                                int32_t result) {
+  if (result == kSuccess) {
+    if (!engine()->HandleDocumentLoad(std::move(loader), GetURL())) {
+      document_load_state_ = DocumentLoadState::kLoading;
+      DocumentLoadFailed();
+    }
+  } else if (result != kErrorAborted) {
+    DocumentLoadFailed();
+  }
+}
+
 void PdfViewPluginBase::DidOpenPreview(std::unique_ptr<UrlLoader> loader,
                                        int32_t result) {
-  DCHECK_EQ(result, Result::kSuccess);
+  DCHECK_EQ(result, kSuccess);
   preview_client_ = std::make_unique<PreviewModeClient>(this);
   preview_engine_ = std::make_unique<PDFiumEngine>(
       preview_client_.get(), PDFiumFormFiller::ScriptOption::kNoJavaScript);
@@ -1666,7 +1678,7 @@ void PdfViewPluginBase::ProcessPreviewPageInfo(const std::string& url,
 
 void PdfViewPluginBase::LoadAvailablePreviewPage() {
   if (preview_pages_info_.empty() ||
-      document_load_state() != DocumentLoadState::kComplete ||
+      document_load_state_ != DocumentLoadState::kComplete ||
       preview_document_load_state_ == DocumentLoadState::kLoading) {
     return;
   }

pdf/pdf_view_plugin_base.h

@@ -156,6 +156,10 @@ class PdfViewPluginBase : public PDFEngine::Client,
     return notified_browser_about_unsupported_feature_;
   }
 
+  DocumentLoadState document_load_state_for_testing() const {
+    return document_load_state_;
+  }
+
  protected:
   struct BackgroundPart {
     gfx::Rect location;
@@ -193,9 +197,6 @@ class PdfViewPluginBase : public PDFEngine::Client,
   // frame's origin.
   virtual std::unique_ptr<UrlLoader> CreateUrlLoaderInternal() = 0;
 
-  // Handles `LoadUrl()` result.
-  virtual void DidOpen(std::unique_ptr<UrlLoader> loader, int32_t result) = 0;
-
   bool HandleInputEvent(const blink::WebInputEvent& event);
 
   // Handles `postMessage()` calls from the embedder.
@@ -389,11 +390,6 @@ class PdfViewPluginBase : public PDFEngine::Client,
 
   bool stop_scrolling() const { return stop_scrolling_; }
 
-  DocumentLoadState document_load_state() const { return document_load_state_; }
-  void set_document_load_state(DocumentLoadState state) {
-    document_load_state_ = state;
-  }
-
   AccessibilityState accessibility_state() const {
     return accessibility_state_;
   }
@@ -489,6 +485,9 @@ class PdfViewPluginBase : public PDFEngine::Client,
                              int32_t max,
                              uint32_t bucket_count);
 
+  // Handles `LoadUrl()` result.
+  void DidOpen(std::unique_ptr<UrlLoader> loader, int32_t result);
+
   // Handles `LoadUrl()` result for print preview.
   void DidOpenPreview(std::unique_ptr<UrlLoader> loader, int32_t result);
 

pdf/pdf_view_plugin_base_unittest.cc

@@ -135,14 +135,12 @@ class FakePdfViewPluginBase : public PdfViewPluginBase {
  public:
   // Public for testing.
   using PdfViewPluginBase::accessibility_state;
-  using PdfViewPluginBase::document_load_state;
   using PdfViewPluginBase::edit_mode;
   using PdfViewPluginBase::engine;
   using PdfViewPluginBase::full_frame;
   using PdfViewPluginBase::HandleMessage;
   using PdfViewPluginBase::InitializeEngine;
   using PdfViewPluginBase::LoadUrl;
-  using PdfViewPluginBase::set_document_load_state;
   using PdfViewPluginBase::set_full_frame;
 
   MOCK_METHOD(bool, Confirm, (const std::string&), (override));
@@ -186,8 +184,6 @@ class FakePdfViewPluginBase : public PdfViewPluginBase {
               (),
               (override));
 
-  MOCK_METHOD(void, DidOpen, (std::unique_ptr<UrlLoader>, int32_t), (override));
-
   void SendMessage(base::Value message) override {
     sent_messages_.push_back(std::move(message));
   }
@@ -516,7 +512,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
 
   ASSERT_FALSE(fake_plugin_.IsPrintPreview());
   ASSERT_EQ(PdfViewPluginBase::DocumentLoadState::kLoading,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
 
   // Change the accessibility state to pending so that accessibility can be
   // loaded later.
@@ -534,7 +530,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
 
   fake_plugin_.DocumentLoadComplete();
   EXPECT_EQ(PdfViewPluginBase::DocumentLoadState::kComplete,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_EQ(PdfViewPluginBase::AccessibilityState::kLoaded,
             fake_plugin_.accessibility_state());
 
@@ -559,7 +555,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
 
   ASSERT_FALSE(fake_plugin_.IsPrintPreview());
   ASSERT_EQ(PdfViewPluginBase::DocumentLoadState::kLoading,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   ASSERT_EQ(PdfViewPluginBase::AccessibilityState::kOff,
             fake_plugin_.accessibility_state());
 
@@ -574,7 +570,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
 
   fake_plugin_.DocumentLoadComplete();
   EXPECT_EQ(PdfViewPluginBase::DocumentLoadState::kComplete,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_EQ(PdfViewPluginBase::AccessibilityState::kOff,
             fake_plugin_.accessibility_state());
 
@@ -597,7 +593,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
 
   ASSERT_FALSE(fake_plugin_.IsPrintPreview());
   ASSERT_EQ(PdfViewPluginBase::DocumentLoadState::kLoading,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
 
   EXPECT_CALL(fake_plugin_, UserMetricsRecordAction("PDF.LoadSuccess"));
   EXPECT_CALL(fake_plugin_, SetFormFieldInFocus(false));
@@ -608,7 +604,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
 
   fake_plugin_.DocumentLoadComplete();
   EXPECT_EQ(PdfViewPluginBase::DocumentLoadState::kComplete,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
 
   // Check all the sent messages.
   ASSERT_EQ(4u, fake_plugin_.sent_messages().size());
@@ -627,13 +623,13 @@ TEST_F(PdfViewPluginBaseWithoutDocInfoTest, DocumentLoadCompletePostMessages) {
 
   ASSERT_FALSE(fake_plugin_.IsPrintPreview());
   ASSERT_EQ(PdfViewPluginBase::DocumentLoadState::kLoading,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_CALL(fake_plugin_, UserMetricsRecordAction("PDF.LoadSuccess"));
   EXPECT_CALL(fake_plugin_, SetFormFieldInFocus(false));
 
   fake_plugin_.DocumentLoadComplete();
   EXPECT_EQ(PdfViewPluginBase::DocumentLoadState::kComplete,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
 
   // Check the sent messages when the document doesn't have any metadata,
   // attachments or bookmarks.
@@ -651,7 +647,7 @@ TEST_F(PdfViewPluginBaseTest, DocumentLoadFailedWithNotifiedRenderFrame) {
   fake_plugin_.CreateUrlLoader();
 
   ASSERT_EQ(PdfViewPluginBase::DocumentLoadState::kLoading,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_TRUE(fake_plugin_.GetDidCallStartLoadingForTesting());
 
   EXPECT_CALL(fake_plugin_, UserMetricsRecordAction("PDF.LoadFailure"));
@@ -659,7 +655,7 @@ TEST_F(PdfViewPluginBaseTest, DocumentLoadFailedWithNotifiedRenderFrame) {
 
   fake_plugin_.DocumentLoadFailed();
   EXPECT_EQ(PdfViewPluginBase::DocumentLoadState::kFailed,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_FALSE(fake_plugin_.GetDidCallStartLoadingForTesting());
 }
 
@@ -669,13 +665,13 @@ TEST_F(PdfViewPluginBaseTest, DocumentLoadFailedWithoutNotifiedRenderFrame) {
   EXPECT_FALSE(fake_plugin_.GetDidCallStartLoadingForTesting());
 
   ASSERT_EQ(PdfViewPluginBase::DocumentLoadState::kLoading,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_CALL(fake_plugin_, UserMetricsRecordAction("PDF.LoadFailure"));
   EXPECT_CALL(fake_plugin_, PluginDidStopLoading()).Times(0);
 
   fake_plugin_.DocumentLoadFailed();
   EXPECT_EQ(PdfViewPluginBase::DocumentLoadState::kFailed,
-            fake_plugin_.document_load_state());
+            fake_plugin_.document_load_state_for_testing());
   EXPECT_FALSE(fake_plugin_.GetDidCallStartLoadingForTesting());
 }
 

pdf/pdf_view_web_plugin.cc

@@ -776,17 +776,6 @@ std::unique_ptr<UrlLoader> PdfViewWebPlugin::CreateUrlLoaderInternal() {
   return loader;
 }
 
-// Modeled on `OutOfProcessInstance::DidOpen()`.
-void PdfViewWebPlugin::DidOpen(std::unique_ptr<UrlLoader> loader,
-                               int32_t result) {
-  if (result == Result::kSuccess) {
-    if (!engine()->HandleDocumentLoad(std::move(loader), GetURL()))
-      DocumentLoadFailed();
-  } else {
-    NOTIMPLEMENTED();
-  }
-}
-
 void PdfViewWebPlugin::SendMessage(base::Value message) {
   post_message_sender_.Post(std::move(message));
 }

pdf/pdf_view_web_plugin.h

@@ -271,7 +271,6 @@ class PdfViewWebPlugin final : public PdfViewPluginBase,
   // PdfViewPluginBase:
   base::WeakPtr<PdfViewPluginBase> GetWeakPtr() override;
   std::unique_ptr<UrlLoader> CreateUrlLoaderInternal() override;
-  void DidOpen(std::unique_ptr<UrlLoader> loader, int32_t result) override;
   void SendMessage(base::Value message) override;
   void SaveAs() override;
   void InitImageData(const gfx::Size& size) override;