Delay rotating PDF layout until after zoom calculation
Instead of applying rotation layout changes immediately, rotations will compute and propose the new layout to the JavaScript layer using the "documentDimensions" message. At that point, the JavaScript layer will have the chance to compute a new zoom level (such as for various "fit-to-X" options) before the rotation is applied by posting back a "viewport" message. The overall effect is to eliminate a distracting visual flash when the rotated layout is applied immediately, followed by a zoom adjustment once the layout change is passed to the JavaScript layer. Layout changes are divided into two types: Immediate layout changes reflecting changes in the PDF plugin's state (such as new pages), implemented by RefreshCurrentDocumentLayout(); and deferred layout changes reflecting user requests (such as rotations), implemented by ProposeNextDocumentLayout(), followed by ApplyDocumentLayout(). There are two particularly subtle edge cases: 1. Layout proposals must not be skipped if there would be no change in the layout, as asynchronous layout implies proposals may still be in flight. Proposals care about the future layout state, not the current layout state. 2. Applying a layout may trigger notifications back to JavaScript, which could trigger further layout applications. The implementation must be careful not to enter an infinite loop. For simplicity, this implementation redundantly computes layout at many points. Improvements in this area are deferred to crbug.com/1013800. Bug: 885110 Fixed: 885110 Change-Id: I7b43bd3409eb14159cbd5eb5d3efbd393878257f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1747170 Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: K Moon <kmoon@chromium.org> Cr-Commit-Position: refs/heads/master@{#708462}
This commit is contained in:
@ -563,6 +563,15 @@ void OutOfProcessInstance::HandleMessage(const pp::Var& message) {
|
||||
std::string type = dict.Get(kType).AsString();
|
||||
|
||||
if (type == kJSViewportType) {
|
||||
pp::Var layout_options_var = dict.Get(kJSLayoutOptions);
|
||||
if (!layout_options_var.is_undefined()) {
|
||||
DocumentLayout::Options layout_options;
|
||||
layout_options.FromVar(layout_options_var);
|
||||
// TODO(crbug.com/1013800): Eliminate need to get document size from here.
|
||||
document_size_ = engine_->ApplyDocumentLayout(layout_options);
|
||||
OnGeometryChanged(zoom_, device_scale_);
|
||||
}
|
||||
|
||||
if (!(dict.Get(pp::Var(kJSXOffset)).is_number() &&
|
||||
dict.Get(pp::Var(kJSYOffset)).is_number() &&
|
||||
dict.Get(pp::Var(kJSZoom)).is_number() &&
|
||||
@ -1293,12 +1302,10 @@ void OutOfProcessInstance::FillRect(const pp::Rect& rect, uint32_t color) {
|
||||
}
|
||||
|
||||
void OutOfProcessInstance::ProposeDocumentLayout(const DocumentLayout& layout) {
|
||||
document_size_ = layout.size();
|
||||
|
||||
pp::VarDictionary dimensions;
|
||||
dimensions.Set(kType, kJSDocumentDimensionsType);
|
||||
dimensions.Set(kJSDocumentWidth, pp::Var(document_size_.width()));
|
||||
dimensions.Set(kJSDocumentHeight, pp::Var(document_size_.height()));
|
||||
dimensions.Set(kJSDocumentWidth, pp::Var(layout.size().width()));
|
||||
dimensions.Set(kJSDocumentHeight, pp::Var(layout.size().height()));
|
||||
dimensions.Set(kJSLayoutOptions, layout.options().ToVar());
|
||||
pp::VarArray page_dimensions_array;
|
||||
size_t num_pages = layout.page_count();
|
||||
@ -1316,8 +1323,6 @@ void OutOfProcessInstance::ProposeDocumentLayout(const DocumentLayout& layout) {
|
||||
}
|
||||
dimensions.Set(kJSPageDimensions, page_dimensions_array);
|
||||
PostMessage(dimensions);
|
||||
|
||||
OnGeometryChanged(zoom_, device_scale_);
|
||||
}
|
||||
|
||||
void OutOfProcessInstance::Invalidate(const pp::Rect& rect) {
|
||||
|
@ -17,6 +17,7 @@
|
||||
#include "base/strings/string16.h"
|
||||
#include "base/time/time.h"
|
||||
#include "build/build_config.h"
|
||||
#include "pdf/document_layout.h"
|
||||
#include "ppapi/c/dev/pp_cursor_type_dev.h"
|
||||
#include "ppapi/c/dev/ppp_printing_dev.h"
|
||||
#include "ppapi/c/ppb_input_event.h"
|
||||
@ -55,8 +56,6 @@ class VarDictionary;
|
||||
|
||||
namespace chrome_pdf {
|
||||
|
||||
class DocumentLayout;
|
||||
|
||||
// Do one time initialization of the SDK.
|
||||
// If |enable_v8| is false, then the PDFEngine will not be able to run
|
||||
// JavaScript.
|
||||
@ -138,9 +137,9 @@ class PDFEngine {
|
||||
public:
|
||||
virtual ~Client() {}
|
||||
|
||||
// Proposes a document layout to the client.
|
||||
// TODO(crbug.com/885110): Layout still occurs immediately for now. In the
|
||||
// future, the client will need to accept the layout before it takes effect.
|
||||
// Proposes a document layout to the client. For the proposed layout to
|
||||
// become effective, the client must call PDFEngine::ApplyDocumentLayout()
|
||||
// with the new layout options (although this call can be asynchronous).
|
||||
virtual void ProposeDocumentLayout(const DocumentLayout& layout) = 0;
|
||||
|
||||
// Informs the client that the given rect needs to be repainted.
|
||||
@ -322,6 +321,13 @@ class PDFEngine {
|
||||
virtual void ZoomUpdated(double new_zoom_level) = 0;
|
||||
virtual void RotateClockwise() = 0;
|
||||
virtual void RotateCounterclockwise() = 0;
|
||||
|
||||
// Applies the document layout options proposed by a call to
|
||||
// PDFEngine::Client::ProposeDocumentLayout(), returning the overall size of
|
||||
// the new effective layout.
|
||||
virtual pp::Size ApplyDocumentLayout(
|
||||
const DocumentLayout::Options& options) = 0;
|
||||
|
||||
virtual std::string GetSelectedText() = 0;
|
||||
// Returns true if focus is within an editable form text area.
|
||||
virtual bool CanEditText() = 0;
|
||||
|
@ -1917,19 +1917,19 @@ void PDFiumEngine::ZoomUpdated(double new_zoom_level) {
|
||||
|
||||
void PDFiumEngine::RotateClockwise() {
|
||||
desired_layout_options_.RotatePagesClockwise();
|
||||
RotateInternal();
|
||||
ProposeNextDocumentLayout();
|
||||
}
|
||||
|
||||
void PDFiumEngine::RotateCounterclockwise() {
|
||||
desired_layout_options_.RotatePagesCounterclockwise();
|
||||
RotateInternal();
|
||||
ProposeNextDocumentLayout();
|
||||
}
|
||||
|
||||
void PDFiumEngine::InvalidateAllPages() {
|
||||
CancelPaints();
|
||||
StopFind();
|
||||
DCHECK(document_loaded_);
|
||||
LoadPageInfo();
|
||||
RefreshCurrentDocumentLayout();
|
||||
client_->Invalidate(pp::Rect(plugin_size_));
|
||||
}
|
||||
|
||||
@ -2488,15 +2488,18 @@ void PDFiumEngine::ContinueLoadingDocument(const std::string& password) {
|
||||
}
|
||||
|
||||
void PDFiumEngine::LoadPageInfo() {
|
||||
std::vector<pp::Size> page_sizes = LoadPageSizes();
|
||||
if (page_sizes.empty())
|
||||
return;
|
||||
RefreshCurrentDocumentLayout();
|
||||
|
||||
if (two_up_view_) {
|
||||
layout_.ComputeTwoUpViewLayout(page_sizes);
|
||||
} else {
|
||||
layout_.ComputeSingleViewLayout(page_sizes);
|
||||
}
|
||||
// TODO(crbug.com/1013800): RefreshCurrentDocumentLayout() should send some
|
||||
// sort of "current layout changed" notification, instead of proposing a new
|
||||
// layout. Proposals are never rejected currently, so this is OK for now.
|
||||
ProposeNextDocumentLayout();
|
||||
}
|
||||
|
||||
void PDFiumEngine::RefreshCurrentDocumentLayout() {
|
||||
UpdateDocumentLayout(&layout_);
|
||||
if (!layout_.dirty())
|
||||
return;
|
||||
|
||||
DCHECK_EQ(pages_.size(), layout_.page_count());
|
||||
for (size_t i = 0; i < layout_.page_count(); ++i) {
|
||||
@ -2504,14 +2507,35 @@ void PDFiumEngine::LoadPageInfo() {
|
||||
pages_[i]->set_rect(layout_.page_bounds_rect(i));
|
||||
}
|
||||
|
||||
layout_.clear_dirty();
|
||||
|
||||
CalculateVisiblePages();
|
||||
if (layout_.dirty()) {
|
||||
layout_.clear_dirty();
|
||||
client_->ProposeDocumentLayout(layout_);
|
||||
}
|
||||
|
||||
void PDFiumEngine::ProposeNextDocumentLayout() {
|
||||
DocumentLayout next_layout;
|
||||
next_layout.SetOptions(desired_layout_options_);
|
||||
UpdateDocumentLayout(&next_layout);
|
||||
|
||||
// The time windows between proposal and application may overlap, so we must
|
||||
// always propose a new layout, regardless of the current layout state.
|
||||
client_->ProposeDocumentLayout(next_layout);
|
||||
}
|
||||
|
||||
void PDFiumEngine::UpdateDocumentLayout(DocumentLayout* layout) {
|
||||
std::vector<pp::Size> page_sizes = LoadPageSizes(layout->options());
|
||||
if (page_sizes.empty())
|
||||
return;
|
||||
|
||||
if (two_up_view_) {
|
||||
layout->ComputeTwoUpViewLayout(page_sizes);
|
||||
} else {
|
||||
layout->ComputeSingleViewLayout(page_sizes);
|
||||
}
|
||||
}
|
||||
|
||||
std::vector<pp::Size> PDFiumEngine::LoadPageSizes() {
|
||||
std::vector<pp::Size> PDFiumEngine::LoadPageSizes(
|
||||
const DocumentLayout::Options& layout_options) {
|
||||
std::vector<pp::Size> page_sizes;
|
||||
if (!doc_loader_)
|
||||
return page_sizes;
|
||||
@ -2541,7 +2565,10 @@ std::vector<pp::Size> PDFiumEngine::LoadPageSizes() {
|
||||
page_available = doc_complete;
|
||||
}
|
||||
|
||||
pp::Size size = page_available ? GetPageSize(i) : default_page_size_;
|
||||
// TODO(crbug.com/1013800): It'd be better if page size were independent of
|
||||
// layout options, and handled in the layout code.
|
||||
pp::Size size = page_available ? GetPageSizeForLayout(i, layout_options)
|
||||
: default_page_size_;
|
||||
EnlargePage(i, new_page_count, &size);
|
||||
page_sizes.push_back(size);
|
||||
}
|
||||
@ -2717,6 +2744,12 @@ bool PDFiumEngine::CheckPageAvailable(int index, std::vector<int>* pending) {
|
||||
}
|
||||
|
||||
pp::Size PDFiumEngine::GetPageSize(int index) {
|
||||
return GetPageSizeForLayout(index, layout_.options());
|
||||
}
|
||||
|
||||
pp::Size PDFiumEngine::GetPageSizeForLayout(
|
||||
int index,
|
||||
const DocumentLayout::Options& layout_options) {
|
||||
pp::Size size;
|
||||
double width_in_points = 0;
|
||||
double height_in_points = 0;
|
||||
@ -2729,7 +2762,7 @@ pp::Size PDFiumEngine::GetPageSize(int index) {
|
||||
int height_in_pixels = static_cast<int>(
|
||||
ConvertUnitDouble(height_in_points, kPointsPerInch, kPixelsPerInch));
|
||||
|
||||
switch (layout_.options().default_page_orientation()) {
|
||||
switch (layout_options.default_page_orientation()) {
|
||||
case PageOrientation::kOriginal:
|
||||
case PageOrientation::kClockwise180:
|
||||
// No axis swap needed.
|
||||
@ -3403,7 +3436,20 @@ void PDFiumEngine::OnSelectionPositionChanged() {
|
||||
client_->SelectionChanged(left, right);
|
||||
}
|
||||
|
||||
void PDFiumEngine::RotateInternal() {
|
||||
pp::Size PDFiumEngine::ApplyDocumentLayout(
|
||||
const DocumentLayout::Options& options) {
|
||||
// We need to return early if the layout would not change, otherwise calling
|
||||
// client_->ScrollToPage() would send another "viewport" message, triggering
|
||||
// an infinite loop.
|
||||
//
|
||||
// TODO(crbug.com/1013800): The current implementation computes layout twice
|
||||
// (here, and in InvalidateAllPages()). This shouldn't be too expensive at
|
||||
// realistic page counts, but could be avoided.
|
||||
layout_.SetOptions(options);
|
||||
UpdateDocumentLayout(&layout_);
|
||||
if (!layout_.dirty())
|
||||
return layout_.size();
|
||||
|
||||
// Store the current find index so that we can resume finding at that
|
||||
// particular index after we have recomputed the find results.
|
||||
std::string current_find_text = current_find_text_;
|
||||
@ -3412,7 +3458,6 @@ void PDFiumEngine::RotateInternal() {
|
||||
// Save the current page.
|
||||
int most_visible_page = most_visible_page_;
|
||||
|
||||
layout_.SetOptions(desired_layout_options_);
|
||||
InvalidateAllPages();
|
||||
|
||||
// Restore find results.
|
||||
@ -3426,6 +3471,8 @@ void PDFiumEngine::RotateInternal() {
|
||||
// the scroll position has not. Re-adjust.
|
||||
// TODO(thestig): It would be better to also restore the position on the page.
|
||||
client_->ScrollToPage(most_visible_page);
|
||||
|
||||
return layout_.size();
|
||||
}
|
||||
|
||||
void PDFiumEngine::SetSelecting(bool selecting) {
|
||||
|
@ -87,6 +87,7 @@ class PDFiumEngine : public PDFEngine,
|
||||
void ZoomUpdated(double new_zoom_level) override;
|
||||
void RotateClockwise() override;
|
||||
void RotateCounterclockwise() override;
|
||||
pp::Size ApplyDocumentLayout(const DocumentLayout::Options& options) override;
|
||||
std::string GetSelectedText() override;
|
||||
bool CanEditText() override;
|
||||
bool HasEditableText() override;
|
||||
@ -248,6 +249,16 @@ class PDFiumEngine : public PDFEngine,
|
||||
// Loads information about the pages in the document and performs layout.
|
||||
void LoadPageInfo();
|
||||
|
||||
// Refreshes the document layout using the current pages and layout options.
|
||||
void RefreshCurrentDocumentLayout();
|
||||
|
||||
// Proposes the next document layout using the current pages and
|
||||
// |desired_layout_options_|.
|
||||
void ProposeNextDocumentLayout();
|
||||
|
||||
// Updates |layout| using the current page sizes.
|
||||
void UpdateDocumentLayout(DocumentLayout* layout);
|
||||
|
||||
// Loads information about the pages in the document, calculating and
|
||||
// returning the individual page sizes.
|
||||
//
|
||||
@ -256,7 +267,8 @@ class PDFiumEngine : public PDFEngine,
|
||||
//
|
||||
// TODO(kmoon): LoadPageSizes() is a bit misnomer, but LoadPageInfo() is
|
||||
// taken right now...
|
||||
std::vector<pp::Size> LoadPageSizes();
|
||||
std::vector<pp::Size> LoadPageSizes(
|
||||
const DocumentLayout::Options& layout_options);
|
||||
|
||||
void LoadBody();
|
||||
|
||||
@ -284,6 +296,8 @@ class PDFiumEngine : public PDFEngine,
|
||||
// Helper function to get a given page's size in pixels. This is not part of
|
||||
// PDFiumPage because we might not have that structure when we need this.
|
||||
pp::Size GetPageSize(int index);
|
||||
pp::Size GetPageSizeForLayout(int index,
|
||||
const DocumentLayout::Options& layout_options);
|
||||
|
||||
// Helper function for getting the inset sizes for the current layout. If
|
||||
// |two_up_view_| is true, the configuration of inset sizes depends on
|
||||
|
@ -18,6 +18,7 @@ namespace {
|
||||
|
||||
using ::testing::InSequence;
|
||||
using ::testing::NiceMock;
|
||||
using ::testing::Return;
|
||||
|
||||
MATCHER_P2(LayoutWithSize, width, height, "") {
|
||||
return arg.size() == pp::Size(width, height);
|
||||
@ -34,6 +35,7 @@ class MockTestClient : public TestClient {
|
||||
|
||||
// TODO(crbug.com/989095): MOCK_METHOD() triggers static_assert on Windows.
|
||||
MOCK_METHOD1(ProposeDocumentLayout, void(const DocumentLayout& layout));
|
||||
MOCK_METHOD1(ScrollToPage, void(int page));
|
||||
};
|
||||
|
||||
class PDFiumEngineTest : public PDFiumTestBase {
|
||||
@ -49,7 +51,15 @@ class PDFiumEngineTest : public PDFiumTestBase {
|
||||
|
||||
TEST_F(PDFiumEngineTest, InitializeWithRectanglesMultiPagesPdf) {
|
||||
NiceMock<MockTestClient> client;
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)));
|
||||
|
||||
// ProposeDocumentLayout() gets called twice during loading because
|
||||
// PDFiumEngine::ContinueLoadingDocument() calls LoadBody() (which eventually
|
||||
// triggers a layout proposal), and then calls FinishLoadingDocument() (since
|
||||
// the document is complete), which calls LoadBody() again. Coalescing these
|
||||
// proposals is not correct unless we address the issue covered by
|
||||
// PDFiumEngineTest.ProposeDocumentLayoutWithOverlap.
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)))
|
||||
.Times(2);
|
||||
|
||||
std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
|
||||
&client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
|
||||
@ -67,7 +77,8 @@ TEST_F(PDFiumEngineTest, AppendBlankPagesWithFewerPages) {
|
||||
NiceMock<MockTestClient> client;
|
||||
{
|
||||
InSequence normal_then_append;
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)));
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)))
|
||||
.Times(2);
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(276, 1037)));
|
||||
}
|
||||
|
||||
@ -87,7 +98,8 @@ TEST_F(PDFiumEngineTest, AppendBlankPagesWithMorePages) {
|
||||
NiceMock<MockTestClient> client;
|
||||
{
|
||||
InSequence normal_then_append;
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)));
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)))
|
||||
.Times(2);
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(276, 2425)));
|
||||
}
|
||||
|
||||
@ -107,5 +119,36 @@ TEST_F(PDFiumEngineTest, AppendBlankPagesWithMorePages) {
|
||||
ExpectPageRect(engine.get(), 6, {5, 2085, 266, 333});
|
||||
}
|
||||
|
||||
TEST_F(PDFiumEngineTest, ProposeDocumentLayoutWithOverlap) {
|
||||
NiceMock<MockTestClient> client;
|
||||
std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
|
||||
&client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
|
||||
ASSERT_TRUE(engine);
|
||||
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1463)))
|
||||
.WillOnce(Return());
|
||||
engine->RotateClockwise();
|
||||
|
||||
EXPECT_CALL(client, ProposeDocumentLayout(LayoutWithSize(343, 1664)))
|
||||
.WillOnce(Return());
|
||||
engine->RotateCounterclockwise();
|
||||
}
|
||||
|
||||
TEST_F(PDFiumEngineTest, ApplyDocumentLayoutAvoidsInfiniteLoop) {
|
||||
NiceMock<MockTestClient> client;
|
||||
std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
|
||||
&client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
|
||||
ASSERT_TRUE(engine);
|
||||
|
||||
DocumentLayout::Options options;
|
||||
EXPECT_CALL(client, ScrollToPage(-1)).Times(0);
|
||||
CompareSize({343, 1664}, engine->ApplyDocumentLayout(options));
|
||||
|
||||
options.RotatePagesClockwise();
|
||||
EXPECT_CALL(client, ScrollToPage(-1)).Times(1);
|
||||
CompareSize({343, 1463}, engine->ApplyDocumentLayout(options));
|
||||
CompareSize({343, 1463}, engine->ApplyDocumentLayout(options));
|
||||
}
|
||||
|
||||
} // namespace
|
||||
} // namespace chrome_pdf
|
||||
|
@ -13,7 +13,11 @@ TestClient::TestClient() = default;
|
||||
TestClient::~TestClient() = default;
|
||||
|
||||
void TestClient::ProposeDocumentLayout(const DocumentLayout& layout) {
|
||||
CHECK(engine());
|
||||
// Most tests will want to accept the proposed layout immediately: Applying
|
||||
// layout asynchronously is more accurate, but in most cases, doing so adds
|
||||
// complexity without much gain. Instead, we can override this behavior just
|
||||
// where it matters (like PDFiumEngineTest.ProposeDocumentLayoutWithOverlap).
|
||||
engine()->ApplyDocumentLayout(layout.options());
|
||||
}
|
||||
|
||||
bool TestClient::Confirm(const std::string& message) {
|
||||
|
Reference in New Issue
Block a user