0

Send UI direction to internal PDF plugin

Extends chrome_pdf::DocumentLayout::Options with a field for the UI
direction (left-to-right or right-to-left). This allows the viewer UI to
communicate the UI direction to the plugin. (This information is
available from base::i18n::IsRTL() in unseasoned mode, but is not
available to the Pepper plugin. In any case, it could be useful to make
it togglable in the PDF viewer.)

The UI direction field could have been added to the viewport message
directly, but it likely will be useful to support RTL page layouts in
the future.

Also adds some serialization and deserialization tests, now that Options
serializes to and from base::Value. Also had to fix a bug with dirty
layout options triggering layout before document loading finished.

Bug: 1158670
Change-Id: Iaa7cfccacfe54858a53d6e28b07f786332d7e9e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3202535
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#927933}
This commit is contained in:
K. Moon
2021-10-05 00:37:29 +00:00
committed by Chromium LUCI CQ
parent ff5551a051
commit e2dda8f27c
9 changed files with 147 additions and 22 deletions

@ -4,7 +4,7 @@
import {assert, assertNotReached} from 'chrome://resources/js/assert.m.js';
import {EventTracker} from 'chrome://resources/js/event_tracker.m.js';
import {$, hasKeyModifiers} from 'chrome://resources/js/util.m.js';
import {$, hasKeyModifiers, isRTL} from 'chrome://resources/js/util.m.js';
import {FittingType, Point} from './constants.js';
import {Gesture, GestureDetector, PinchEventDetail} from './gesture_detector.js';
@ -22,6 +22,7 @@ let DocumentDimensions;
/**
* @typedef {{
* direction: number,
* defaultPageOrientation: number,
* twoUpViewEnabled: boolean,
* }}
@ -1237,6 +1238,18 @@ export class Viewport {
this.mightZoom_(() => {
const initialDimensions = !this.documentDimensions_;
this.documentDimensions_ = documentDimensions;
// Override layout direction based on isRTL().
if (this.documentDimensions_.layoutOptions) {
if (isRTL()) {
// `base::i18n::TextDirection::RIGHT_TO_LEFT`
this.documentDimensions_.layoutOptions.direction = 1;
} else {
// `base::i18n::TextDirection::LEFT_TO_RIGHT`
this.documentDimensions_.layoutOptions.direction = 2;
}
}
this.pageDimensions_ = this.documentDimensions_.pageDimensions;
if (initialDimensions) {
this.setZoomInternal_(Math.min(

@ -11,6 +11,7 @@ const tests = [
document.body.querySelector('#viewer'));
chrome.test.assertEq(
{
direction: 2,
defaultPageOrientation: 0,
twoUpViewEnabled: false,
},

@ -219,7 +219,8 @@ const tests = [
const viewport = getZoomableViewport(mockWindow, new MockSizer(), 0, 1);
const documentDimensions = new MockDocumentDimensions(
100, 100, {defaultPageOrientation: 0, twoUpViewEnabled: true});
100, 100,
{direction: 0, defaultPageOrientation: 0, twoUpViewEnabled: true});
documentDimensions.addPageForTwoUpView(100, 0, 300, 400);
documentDimensions.addPageForTwoUpView(400, 0, 400, 300);
documentDimensions.addPageForTwoUpView(0, 400, 400, 250);
@ -630,7 +631,8 @@ const tests = [
viewport.setViewportChangedCallback(mockCallback.callback);
const documentDimensions = new MockDocumentDimensions(
800, 750, {defaultPageOrientation: 0, twoUpViewEnabled: true});
800, 750,
{direction: 0, defaultPageOrientation: 0, twoUpViewEnabled: true});
documentDimensions.addPageForTwoUpView(200, 0, 200, 150);
documentDimensions.addPageForTwoUpView(400, 0, 400, 200);
documentDimensions.addPageForTwoUpView(100, 200, 300, 250);
@ -735,7 +737,8 @@ const tests = [
viewport.setViewportChangedCallback(mockCallback.callback);
const documentDimensions = new MockDocumentDimensions(
800, 750, {defaultPageOrientation: 0, twoUpViewEnabled: true});
800, 750,
{direction: 0, defaultPageOrientation: 0, twoUpViewEnabled: true});
documentDimensions.addPageForTwoUpView(200, 0, 200, 150);
documentDimensions.addPageForTwoUpView(400, 0, 400, 200);
documentDimensions.addPageForTwoUpView(100, 200, 300, 250);
@ -1077,9 +1080,10 @@ const tests = [
chrome.test.assertEq(undefined, viewport.getLayoutOptions());
viewport.setDocumentDimensions(new MockDocumentDimensions(
50, 50, {defaultPageOrientation: 1, twoUpViewEnabled: true}));
50, 50,
{direction: 1, defaultPageOrientation: 1, twoUpViewEnabled: true}));
chrome.test.assertEq(
{defaultPageOrientation: 1, twoUpViewEnabled: true},
{direction: 2, defaultPageOrientation: 1, twoUpViewEnabled: true},
viewport.getLayoutOptions());
viewport.setDocumentDimensions(new MockDocumentDimensions(50, 50));

@ -16,6 +16,7 @@ namespace chrome_pdf {
namespace {
constexpr char kDirection[] = "direction";
constexpr char kDefaultPageOrientation[] = "defaultPageOrientation";
constexpr char kTwoUpViewEnabled[] = "twoUpViewEnabled";
@ -51,6 +52,7 @@ DocumentLayout::Options::~Options() = default;
base::Value DocumentLayout::Options::ToValue() const {
base::Value dictionary(base::Value::Type::DICTIONARY);
dictionary.SetIntKey(kDirection, direction_);
dictionary.SetIntKey(kDefaultPageOrientation,
static_cast<int32_t>(default_page_orientation_));
dictionary.SetBoolKey(kTwoUpViewEnabled,
@ -61,6 +63,11 @@ base::Value DocumentLayout::Options::ToValue() const {
void DocumentLayout::Options::FromValue(const base::Value& value) {
DCHECK(value.is_dict());
int32_t direction = value.FindIntKey(kDirection).value();
DCHECK_GE(direction, base::i18n::UNKNOWN_DIRECTION);
DCHECK_LE(direction, base::i18n::TEXT_DIRECTION_MAX);
direction_ = static_cast<base::i18n::TextDirection>(direction);
int32_t default_page_orientation =
value.FindIntKey(kDefaultPageOrientation).value();
DCHECK_GE(default_page_orientation,

@ -9,6 +9,7 @@
#include <vector>
#include "base/check_op.h"
#include "base/i18n/rtl.h"
#include "pdf/draw_utils/coordinates.h"
#include "pdf/page_orientation.h"
#include "ui/gfx/geometry/rect.h"
@ -46,8 +47,9 @@ class DocumentLayout final {
~Options();
friend bool operator==(const Options& lhs, const Options& rhs) {
return lhs.page_spread() == rhs.page_spread() &&
lhs.default_page_orientation() == rhs.default_page_orientation();
return lhs.direction() == rhs.direction() &&
lhs.default_page_orientation() == rhs.default_page_orientation() &&
lhs.page_spread() == rhs.page_spread();
}
friend bool operator!=(const Options& lhs, const Options& rhs) {
@ -60,6 +62,14 @@ class DocumentLayout final {
// Deserializes layout options from a base::Value.
void FromValue(const base::Value& value);
// Page layout direction. This is tied to the direction of the user's UI,
// rather than the direction of individual pages.
base::i18n::TextDirection direction() const { return direction_; }
void set_direction(base::i18n::TextDirection direction) {
direction_ = direction;
}
PageOrientation default_page_orientation() const {
return default_page_orientation_;
}
@ -76,6 +86,7 @@ class DocumentLayout final {
void set_page_spread(PageSpread spread) { page_spread_ = spread; }
private:
base::i18n::TextDirection direction_ = base::i18n::UNKNOWN_DIRECTION;
PageOrientation default_page_orientation_ = PageOrientation::kOriginal;
PageSpread page_spread_ = PageSpread::kOneUp;
};

@ -4,6 +4,11 @@
#include "pdf/document_layout.h"
#include "base/i18n/rtl.h"
#include "base/test/values_test_util.h"
#include "base/values.h"
#include "pdf/page_orientation.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/gfx/geometry/rect.h"
#include "ui/gfx/geometry/size.h"
@ -18,34 +23,43 @@ class DocumentLayoutOptionsTest : public testing::Test {
};
TEST_F(DocumentLayoutOptionsTest, DefaultConstructor) {
EXPECT_EQ(options_.direction(), base::i18n::UNKNOWN_DIRECTION);
EXPECT_EQ(options_.default_page_orientation(), PageOrientation::kOriginal);
EXPECT_EQ(options_.page_spread(), DocumentLayout::PageSpread::kOneUp);
}
TEST_F(DocumentLayoutOptionsTest, CopyConstructor) {
options_.set_direction(base::i18n::RIGHT_TO_LEFT);
options_.RotatePagesClockwise();
options_.set_page_spread(DocumentLayout::PageSpread::kTwoUpOdd);
DocumentLayout::Options copy(options_);
EXPECT_EQ(copy.direction(), base::i18n::RIGHT_TO_LEFT);
EXPECT_EQ(copy.default_page_orientation(), PageOrientation::kClockwise90);
EXPECT_EQ(copy.page_spread(), DocumentLayout::PageSpread::kTwoUpOdd);
options_.set_direction(base::i18n::LEFT_TO_RIGHT);
options_.RotatePagesClockwise();
options_.set_page_spread(DocumentLayout::PageSpread::kOneUp);
EXPECT_EQ(copy.direction(), base::i18n::RIGHT_TO_LEFT);
EXPECT_EQ(copy.default_page_orientation(), PageOrientation::kClockwise90);
EXPECT_EQ(copy.page_spread(), DocumentLayout::PageSpread::kTwoUpOdd);
}
TEST_F(DocumentLayoutOptionsTest, CopyAssignment) {
options_.set_direction(base::i18n::RIGHT_TO_LEFT);
options_.RotatePagesClockwise();
options_.set_page_spread(DocumentLayout::PageSpread::kTwoUpOdd);
DocumentLayout::Options copy = options_;
EXPECT_EQ(copy.direction(), base::i18n::RIGHT_TO_LEFT);
EXPECT_EQ(copy.default_page_orientation(), PageOrientation::kClockwise90);
EXPECT_EQ(copy.page_spread(), DocumentLayout::PageSpread::kTwoUpOdd);
options_.set_direction(base::i18n::LEFT_TO_RIGHT);
options_.RotatePagesClockwise();
options_.set_page_spread(DocumentLayout::PageSpread::kOneUp);
EXPECT_EQ(copy.direction(), base::i18n::RIGHT_TO_LEFT);
EXPECT_EQ(copy.default_page_orientation(), PageOrientation::kClockwise90);
EXPECT_EQ(copy.page_spread(), DocumentLayout::PageSpread::kTwoUpOdd);
}
@ -56,6 +70,12 @@ TEST_F(DocumentLayoutOptionsTest, Equals) {
DocumentLayout::Options copy;
EXPECT_TRUE(copy == options_);
options_.set_direction(base::i18n::RIGHT_TO_LEFT);
EXPECT_FALSE(copy == options_);
copy.set_direction(base::i18n::RIGHT_TO_LEFT);
EXPECT_TRUE(copy == options_);
options_.RotatePagesClockwise();
EXPECT_FALSE(copy == options_);
@ -73,12 +93,6 @@ TEST_F(DocumentLayoutOptionsTest, Equals) {
copy.set_page_spread(DocumentLayout::PageSpread::kTwoUpOdd);
EXPECT_TRUE(copy == options_);
options_.set_page_spread(DocumentLayout::PageSpread::kOneUp);
EXPECT_FALSE(copy == options_);
copy.set_page_spread(DocumentLayout::PageSpread::kOneUp);
EXPECT_TRUE(copy == options_);
}
TEST_F(DocumentLayoutOptionsTest, NotEquals) {
@ -96,6 +110,51 @@ TEST_F(DocumentLayoutOptionsTest, NotEquals) {
EXPECT_FALSE(copy != options_);
}
TEST_F(DocumentLayoutOptionsTest, ToValueDefault) {
base::Value value = options_.ToValue();
EXPECT_THAT(value, base::test::IsJson(R"({
"direction": 0,
"defaultPageOrientation": 0,
"twoUpViewEnabled": false,
})"));
}
TEST_F(DocumentLayoutOptionsTest, ToValueModified) {
options_.set_direction(base::i18n::LEFT_TO_RIGHT);
options_.RotatePagesClockwise();
options_.set_page_spread(DocumentLayout::PageSpread::kTwoUpOdd);
base::Value value = options_.ToValue();
EXPECT_THAT(value, base::test::IsJson(R"({
"direction": 2,
"defaultPageOrientation": 1,
"twoUpViewEnabled": true,
})"));
}
TEST_F(DocumentLayoutOptionsTest, FromValueDefault) {
options_.FromValue(base::test::ParseJson(R"({
"direction": 0,
"defaultPageOrientation": 0,
"twoUpViewEnabled": false,
})"));
EXPECT_EQ(options_, DocumentLayout::Options());
}
TEST_F(DocumentLayoutOptionsTest, FromValueModified) {
options_.FromValue(base::test::ParseJson(R"({
"direction": 2,
"defaultPageOrientation": 1,
"twoUpViewEnabled": true,
})"));
EXPECT_EQ(options_.direction(), base::i18n::LEFT_TO_RIGHT);
EXPECT_EQ(options_.default_page_orientation(), PageOrientation::kClockwise90);
EXPECT_EQ(options_.page_spread(), DocumentLayout::PageSpread::kTwoUpOdd);
}
TEST_F(DocumentLayoutOptionsTest, RotatePagesClockwise) {
options_.RotatePagesClockwise();
EXPECT_EQ(options_.default_page_orientation(), PageOrientation::kClockwise90);

@ -887,6 +887,7 @@ TEST_F(PdfViewPluginBaseWithEngineTest, HandleViewportMessageInitially) {
"userInitiated": false,
"zoom": 1,
"layoutOptions": {
"direction": 0,
"defaultPageOrientation": 0,
"twoUpViewEnabled": false,
},
@ -909,6 +910,7 @@ TEST_F(PdfViewPluginBaseWithEngineTest, HandleViewportMessageSubsequently) {
"userInitiated": false,
"zoom": 1,
"layoutOptions": {
"direction": 0,
"defaultPageOrientation": 0,
"twoUpViewEnabled": false,
},
@ -927,6 +929,7 @@ TEST_F(PdfViewPluginBaseWithEngineTest, HandleViewportMessageSubsequently) {
"userInitiated": false,
"zoom": 1,
"layoutOptions": {
"direction": 0,
"defaultPageOrientation": 0,
"twoUpViewEnabled": true,
},

@ -3746,6 +3746,12 @@ void PDFiumEngine::OnSelectionPositionChanged() {
gfx::Size PDFiumEngine::ApplyDocumentLayout(
const DocumentLayout::Options& options) {
layout_.SetOptions(options);
// Don't actually update layout until the document finishes loading.
if (!document_loaded_)
return layout_.size();
// We need to return early if the layout would not change, otherwise calling
// client_->ScrollToPage() would send another "viewport" message, triggering
// an infinite loop.
@ -3753,7 +3759,6 @@ gfx::Size PDFiumEngine::ApplyDocumentLayout(
// 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();

@ -133,6 +133,17 @@ class PDFiumEngineTest : public PDFiumTestBase {
return loaded_incrementally;
}
void FinishWithPluginSizeUpdated(MockTestClient& client,
PDFiumEngine& engine) {
ResultCallback callback;
EXPECT_CALL(client, ScheduleTaskOnMainThread)
.WillOnce(MoveArg<1>(&callback));
engine.PluginSizeUpdated({});
ASSERT_TRUE(callback);
std::move(callback).Run(0);
}
// Counts the number of available pages. Returns `int` instead of `size_t` for
// consistency with `PDFiumEngine::GetNumberOfPages()`.
int CountAvailablePages(const PDFiumEngine& engine) {
@ -253,6 +264,23 @@ TEST_F(PDFiumEngineTest, ProposeDocumentLayoutWithOverlap) {
engine->RotateCounterclockwise();
}
TEST_F(PDFiumEngineTest, ApplyDocumentLayoutBeforePluginSizeUpdated) {
NiceMock<MockTestClient> client;
InitializeEngineResult initialize_result = InitializeEngineWithoutLoading(
&client, FILE_PATH_LITERAL("rectangles_multi_pages.pdf"));
ASSERT_TRUE(initialize_result.engine);
initialize_result.FinishLoading();
PDFiumEngine& engine = *initialize_result.engine;
DocumentLayout::Options options;
options.RotatePagesClockwise();
EXPECT_CALL(client, ScrollToPage(-1)).Times(0);
EXPECT_EQ(gfx::Size(343, 1664), engine.ApplyDocumentLayout(options));
EXPECT_CALL(client, ScrollToPage(-1)).Times(1);
ASSERT_NO_FATAL_FAILURE(FinishWithPluginSizeUpdated(client, engine));
}
TEST_F(PDFiumEngineTest, ApplyDocumentLayoutAvoidsInfiniteLoop) {
NiceMock<MockTestClient> client;
std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
@ -481,14 +509,8 @@ TEST_F(PDFiumEngineTest, PluginSizeUpdatedAfterLoad) {
ASSERT_TRUE(initialize_result.engine);
PDFiumEngine& engine = *initialize_result.engine;
ResultCallback callback;
EXPECT_CALL(client, ScheduleTaskOnMainThread).WillOnce(MoveArg<1>(&callback));
initialize_result.FinishLoading();
engine.PluginSizeUpdated({});
ASSERT_TRUE(callback);
std::move(callback).Run(0);
ASSERT_NO_FATAL_FAILURE(FinishWithPluginSizeUpdated(client, engine));
EXPECT_EQ(engine.GetNumberOfPages(), CountAvailablePages(engine));
}