Reland "[unseasoned-pdf] Block 100% loading on PDF layout"

Relands commit 76d6ade99f, fixing
annotation mode exit by updating currentController before reloading the
plugin, rather than after.

Original description:

Blocks 100% loading progress until PDF page layout has been negotiated
between the internal plugin and the UI extension.

This fixes ExtensionApiTest.TemporaryAddressSpoof by avoiding a race
between mouse events and layout negotiation.

Bug: 1237952
Change-Id: I817a55bc4decbcbba3797dc135435b866be30f93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3131558
Auto-Submit: K. Moon <kmoon@chromium.org>
Commit-Queue: John Lee <johntlee@chromium.org>
Reviewed-by: Hui Yingst <nigi@chromium.org>
Reviewed-by: John Lee <johntlee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#917117}

chrome/browser/resources/pdf/pdf_viewer.js

@@ -534,7 +534,6 @@ export class PDFViewerElement extends PDFViewerBaseElement {
       // This runs separately to allow other consumers of `loaded` to queue
       // up after this task.
       this.loaded.then(() => {
-        this.currentController = this.pluginController_;
         this.inkController_.unload();
       });
       // TODO(dstockwell): handle save failure
@@ -543,9 +542,8 @@ export class PDFViewerElement extends PDFViewerBaseElement {
       // Data always exists when save is called with requestType = ANNOTATION.
       const result = /** @type {!RequiredSaveResult} */ (saveResult);
       await this.restoreSidenav_();
+      this.currentController = this.pluginController_;
       await this.pluginController_.load(result.fileName, result.dataToSave);
-      // Ensure the plugin gets the initial viewport.
-      this.pluginController_.afterZoom();
     }
   }
 

pdf/BUILD.gn

@@ -412,6 +412,7 @@ if (enable_pdf) {
       ":ppapi_migration",
       "//base",
       "//ppapi/cpp:objects",
+      "//testing/gmock",
       "//testing/gtest",
       "//third_party/blink/public/common:headers",
       "//ui/gfx:geometry_skia",

pdf/pdf_view_plugin_base.cc

@@ -365,7 +365,6 @@ void PdfViewPluginBase::DocumentLoadComplete() {
   SendAttachments();
   SendBookmarks();
   SendMetadata();
-  SendLoadingProgress(/*percentage=*/100);
 
   if (accessibility_state_ == AccessibilityState::kPending)
     LoadAccessibility();
@@ -428,7 +427,6 @@ void PdfViewPluginBase::DocumentLoadProgress(uint32_t available,
   if (progress <= last_progress_sent_ + 1)
     return;
 
-  last_progress_sent_ = progress;
   SendLoadingProgress(progress);
 }
 
@@ -606,6 +604,7 @@ void PdfViewPluginBase::ConsumeSaveToken(const std::string& token) {
 
 void PdfViewPluginBase::SendLoadingProgress(double percentage) {
   DCHECK(percentage == -1 || (percentage >= 0 && percentage <= 100));
+  last_progress_sent_ = percentage;
 
   base::Value message(base::Value::Type::DICTIONARY);
   message.SetStringKey("type", "loadProgress");
@@ -1205,6 +1204,10 @@ void PdfViewPluginBase::HandleViewportMessage(const base::Value& message) {
     // TODO(crbug.com/1013800): Eliminate need to get document size from here.
     document_size_ = engine()->ApplyDocumentLayout(layout_options);
     OnGeometryChanged(zoom_, device_scale_);
+
+    // Send 100% loading progress only after initial layout negotiated.
+    if (last_progress_sent_ < 100)
+      SendLoadingProgress(/*percentage=*/100);
   }
 
   gfx::PointF scroll_position(message.FindDoubleKey("xOffset").value(),

pdf/pdf_view_plugin_base_unittest.cc

@@ -19,6 +19,7 @@
 #include "pdf/buildflags.h"
 #include "pdf/content_restriction.h"
 #include "pdf/document_attachment_info.h"
+#include "pdf/document_layout.h"
 #include "pdf/document_metadata.h"
 #include "pdf/pdf_engine.h"
 #include "pdf/ppapi_migration/callback.h"
@@ -35,6 +36,7 @@ namespace chrome_pdf {
 
 namespace {
 
+using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using ::testing::SaveArg;
 
@@ -343,13 +345,6 @@ base::Value CreateExpectedNoMetadataResponse() {
   return message;
 }
 
-base::Value CreateExpectedLoadingProgressResponse() {
-  base::Value message(base::Value::Type::DICTIONARY);
-  message.SetStringKey("type", "loadProgress");
-  message.SetDoubleKey("progress", 100);
-  return message;
-}
-
 base::Value CreateSaveRequestMessage(PdfViewPluginBase::SaveRequestType type,
                                      const std::string& token) {
   base::Value message(base::Value::Type::DICTIONARY);
@@ -386,8 +381,8 @@ class PdfViewPluginBaseTest : public testing::Test {
 class PdfViewPluginBaseWithEngineTest : public PdfViewPluginBaseTest {
  public:
   void SetUp() override {
-    std::unique_ptr<TestPDFiumEngine> engine =
-        std::make_unique<TestPDFiumEngine>(&fake_plugin_);
+    auto engine =
+        std::make_unique<testing::NiceMock<TestPDFiumEngine>>(&fake_plugin_);
     fake_plugin_.InitializeEngine(std::move(engine));
   }
 };
@@ -544,7 +539,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
             fake_plugin_.accessibility_state());
 
   // Check all the sent messages.
-  ASSERT_EQ(5u, fake_plugin_.sent_messages().size());
+  ASSERT_EQ(4u, fake_plugin_.sent_messages().size());
   EXPECT_EQ(CreateExpectedFormTextFieldFocusChangeResponse(),
             fake_plugin_.sent_messages()[0]);
   EXPECT_EQ(CreateExpectedAttachmentsResponse(),
@@ -553,8 +548,6 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
       CreateExpectedBookmarksResponse(fake_plugin_.engine()->GetBookmarks()),
       fake_plugin_.sent_messages()[2]);
   EXPECT_EQ(CreateExpectedMetadataResponse(), fake_plugin_.sent_messages()[3]);
-  EXPECT_EQ(CreateExpectedLoadingProgressResponse(),
-            fake_plugin_.sent_messages()[4]);
 }
 
 TEST_F(PdfViewPluginBaseWithDocInfoTest,
@@ -586,7 +579,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
             fake_plugin_.accessibility_state());
 
   // Check all the sent messages.
-  ASSERT_EQ(5u, fake_plugin_.sent_messages().size());
+  ASSERT_EQ(4u, fake_plugin_.sent_messages().size());
   EXPECT_EQ(CreateExpectedFormTextFieldFocusChangeResponse(),
             fake_plugin_.sent_messages()[0]);
   EXPECT_EQ(CreateExpectedAttachmentsResponse(),
@@ -595,8 +588,6 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
       CreateExpectedBookmarksResponse(fake_plugin_.engine()->GetBookmarks()),
       fake_plugin_.sent_messages()[2]);
   EXPECT_EQ(CreateExpectedMetadataResponse(), fake_plugin_.sent_messages()[3]);
-  EXPECT_EQ(CreateExpectedLoadingProgressResponse(),
-            fake_plugin_.sent_messages()[4]);
 }
 
 TEST_F(PdfViewPluginBaseWithDocInfoTest,
@@ -620,7 +611,7 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
             fake_plugin_.document_load_state());
 
   // Check all the sent messages.
-  ASSERT_EQ(5u, fake_plugin_.sent_messages().size());
+  ASSERT_EQ(4u, fake_plugin_.sent_messages().size());
   EXPECT_EQ(CreateExpectedFormTextFieldFocusChangeResponse(),
             fake_plugin_.sent_messages()[0]);
   EXPECT_EQ(CreateExpectedAttachmentsResponse(),
@@ -629,8 +620,6 @@ TEST_F(PdfViewPluginBaseWithDocInfoTest,
       CreateExpectedBookmarksResponse(fake_plugin_.engine()->GetBookmarks()),
       fake_plugin_.sent_messages()[2]);
   EXPECT_EQ(CreateExpectedMetadataResponse(), fake_plugin_.sent_messages()[3]);
-  EXPECT_EQ(CreateExpectedLoadingProgressResponse(),
-            fake_plugin_.sent_messages()[4]);
 }
 
 TEST_F(PdfViewPluginBaseWithoutDocInfoTest, DocumentLoadCompletePostMessages) {
@@ -648,13 +637,11 @@ TEST_F(PdfViewPluginBaseWithoutDocInfoTest, DocumentLoadCompletePostMessages) {
 
   // Check the sent messages when the document doesn't have any metadata,
   // attachments or bookmarks.
-  ASSERT_EQ(3u, fake_plugin_.sent_messages().size());
+  ASSERT_EQ(2u, fake_plugin_.sent_messages().size());
   EXPECT_EQ(CreateExpectedFormTextFieldFocusChangeResponse(),
             fake_plugin_.sent_messages()[0]);
   EXPECT_EQ(CreateExpectedNoMetadataResponse(),
             fake_plugin_.sent_messages()[1]);
-  EXPECT_EQ(CreateExpectedLoadingProgressResponse(),
-            fake_plugin_.sent_messages()[2]);
 }
 
 TEST_F(PdfViewPluginBaseTest, DocumentLoadFailedWithNotifiedRenderFrame) {
@@ -895,6 +882,66 @@ TEST_F(PdfViewPluginBaseTest, HandleSetBackgroundColorMessage) {
   EXPECT_EQ(kNewBackgroundColor, fake_plugin_.GetBackgroundColor());
 }
 
+TEST_F(PdfViewPluginBaseWithEngineTest, HandleViewportMessageInitially) {
+  auto* engine = static_cast<TestPDFiumEngine*>(fake_plugin_.engine());
+  EXPECT_CALL(*engine, ApplyDocumentLayout(DocumentLayout::Options()));
+
+  fake_plugin_.HandleMessage(base::test::ParseJson(R"({
+    "type": "viewport",
+    "userInitiated": false,
+    "zoom": 1,
+    "layoutOptions": {
+      "defaultPageOrientation": 0,
+      "twoUpViewEnabled": false,
+    },
+    "xOffset": 0,
+    "yOffset": 0,
+    "pinchPhase": 0,
+  })"));
+
+  EXPECT_THAT(fake_plugin_.sent_messages(), ElementsAre(base::test::IsJson(R"({
+    "type": "loadProgress",
+    "progress": 100.0,
+  })")));
+}
+
+TEST_F(PdfViewPluginBaseWithEngineTest, HandleViewportMessageSubsequently) {
+  auto* engine = static_cast<TestPDFiumEngine*>(fake_plugin_.engine());
+
+  fake_plugin_.HandleMessage(base::test::ParseJson(R"({
+    "type": "viewport",
+    "userInitiated": false,
+    "zoom": 1,
+    "layoutOptions": {
+      "defaultPageOrientation": 0,
+      "twoUpViewEnabled": false,
+    },
+    "xOffset": 0,
+    "yOffset": 0,
+    "pinchPhase": 0,
+  })"));
+  fake_plugin_.clear_sent_messages();
+
+  DocumentLayout::Options two_up_options;
+  two_up_options.set_page_spread(DocumentLayout::PageSpread::kTwoUpOdd);
+  EXPECT_CALL(*engine, ApplyDocumentLayout(two_up_options));
+
+  fake_plugin_.HandleMessage(base::test::ParseJson(R"({
+    "type": "viewport",
+    "userInitiated": false,
+    "zoom": 1,
+    "layoutOptions": {
+      "defaultPageOrientation": 0,
+      "twoUpViewEnabled": true,
+    },
+    "xOffset": 0,
+    "yOffset": 0,
+    "pinchPhase": 0,
+  })"));
+
+  EXPECT_THAT(fake_plugin_.sent_messages(), IsEmpty());
+}
+
 TEST_F(PdfViewPluginBaseWithEngineTest, GetContentRestrictions) {
   auto* engine = static_cast<TestPDFiumEngine*>(fake_plugin_.engine());
   static constexpr int kContentRestrictionCutPaste =

pdf/test/test_pdfium_engine.h

@@ -16,6 +16,7 @@
 #include "pdf/document_metadata.h"
 #include "pdf/pdf_engine.h"
 #include "pdf/pdfium/pdfium_engine.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/common/input/web_mouse_event.h"
 
 namespace blink {
@@ -40,6 +41,11 @@ class TestPDFiumEngine : public PDFiumEngine {
 
   ~TestPDFiumEngine() override;
 
+  MOCK_METHOD(gfx::Size,
+              ApplyDocumentLayout,
+              (const DocumentLayout::Options&),
+              (override));
+
   // Sets a scaled mouse event for testing.
   bool HandleInputEvent(const blink::WebInputEvent& scaled_event) override;