Handle null coordinates from XYZ named destination from a URL.
Currently PDFiumEngine::GetNamedDestination() uses the public API
FPDFDest_GetView() to get a named destination's view, which outputs 0 if
a parameter is null. However, for the x and y coordinates from an XYZ
named destination, 0 is a valid input while null should be treated as
an empty input.
This CL adds a string `xyz_params` in PDFEngine::NamedDestination so
that null parameters can be passed to the viewport and get treated
differently from 0 for x and y coordinates.
Bug: 1157061, 1163701
Change-Id: I81a5a7c4b9c895263e580473a68a070e8b7965dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2634130
Commit-Queue: Hui Yingst <nigi@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845914}
diff --git a/chrome/browser/resources/pdf/open_pdf_params_parser.js b/chrome/browser/resources/pdf/open_pdf_params_parser.js
index 7ded80f..10e4531 100644
--- a/chrome/browser/resources/pdf/open_pdf_params_parser.js
+++ b/chrome/browser/resources/pdf/open_pdf_params_parser.js
@@ -138,15 +138,16 @@
const x = parseFloat(viewModeComponents[1]);
const y = parseFloat(viewModeComponents[2]);
const zoom = parseFloat(viewModeComponents[3]);
- // If |x|, |y| or |zoom| is NaN, the values of the current positions and
- // zoom level are retained.
- if (!Number.isNaN(x) && !Number.isNaN(y) && !Number.isNaN(zoom)) {
+
+ // If zoom is originally 0 for the XYZ view, it is guaranteed to be
+ // transformed into "null" by the backend.
+ assert(zoom !== 0);
+
+ if (!Number.isNaN(zoom)) {
+ params['zoom'] = zoom;
+ }
+ if (!Number.isNaN(x) || !Number.isNaN(y)) {
params['position'] = {x: x, y: y};
- // A zoom of 0 should be treated as a zoom of null (See table 151 in ISO
- // 32000-1 standard for more details about syntax of "XYZ".
- if (zoom !== 0) {
- params['zoom'] = zoom;
- }
}
return params;
}
diff --git a/chrome/browser/resources/pdf/viewport.js b/chrome/browser/resources/pdf/viewport.js
index 0714defc2..cccb29ce 100644
--- a/chrome/browser/resources/pdf/viewport.js
+++ b/chrome/browser/resources/pdf/viewport.js
@@ -1199,8 +1199,8 @@
/**
* Go to the given y position in the given page index.
* @param {number} page the index of the page to go to. zero-based.
- * @param {number} x the x position in the page to go to.
- * @param {number} y the y position in the page to go to.
+ * @param {number|undefined} x the x position in the page to go to.
+ * @param {number|undefined} y the y position in the page to go to.
*/
goToPageAndXY(page, x, y) {
this.mightZoom_(() => {
@@ -1221,6 +1221,18 @@
if (!this.isPagedMode_()) {
toolbarOffset = this.topToolbarHeight_;
}
+
+ // If `x` or `y` is not a valid number or specified, then that
+ // coordinate of the current viewport position should be retained.
+ const currentCoords =
+ /** @type {!Point} */ (this.retrieveCurrentScreenCoordinates_());
+ if (x === undefined || Number.isNaN(x)) {
+ x = currentCoords.x;
+ }
+ if (y === undefined || Number.isNaN(y)) {
+ y = currentCoords.y;
+ }
+
this.position = {
x: (dimensions.x + x) * this.getZoom(),
y: (dimensions.y + y) * this.getZoom() - toolbarOffset
@@ -1346,11 +1358,7 @@
if (zoom) {
this.setZoom(zoom);
}
- const currentCoords =
- /** @type {!Point} */ (this.retrieveCurrentScreenCoordinates_());
- this.goToPageAndXY(
- page, x === undefined ? currentCoords.x : x,
- y === undefined ? currentCoords.y : y);
+ this.goToPageAndXY(page, x, y);
}
/**
diff --git a/chrome/test/data/pdf/params_parser_test.js b/chrome/test/data/pdf/params_parser_test.js
index b602971a..ea19a70 100644
--- a/chrome/test/data/pdf/params_parser_test.js
+++ b/chrome/test/data/pdf/params_parser_test.js
@@ -34,43 +34,64 @@
pageNumber: 10
});
}
- if (destination === 'DestWithXYZAtZoom0') {
+ if (destination === 'DestWithXYZAtZoomNull') {
return Promise.resolve({
messageId: 'getNamedDestination_5',
- namedDestinationView: 'XYZ,111,222,0',
+ namedDestinationView: 'XYZ,111,222,null',
pageNumber: 10
});
}
- if (destination === 'DestWithXYZWithNullParameter') {
+ if (destination === 'DestWithXYZWithX0') {
return Promise.resolve({
messageId: 'getNamedDestination_6',
- namedDestinationView: 'XYZ,111,null,1.7',
- pageNumber: 13
+ namedDestinationView: 'XYZ,0,200,1.7',
+ pageNumber: 11
+ });
+ }
+ if (destination === 'DestWithXYZWithXNull') {
+ return Promise.resolve({
+ messageId: 'getNamedDestination_7',
+ namedDestinationView: 'XYZ,null,200,1.7',
+ pageNumber: 11
+ });
+ }
+ if (destination === 'DestWithXYZWithY0') {
+ return Promise.resolve({
+ messageId: 'getNamedDestination_8',
+ namedDestinationView: 'XYZ,100,0,1.7',
+ pageNumber: 11
+ });
+ }
+ if (destination === 'DestWithXYZWithYNull') {
+ return Promise.resolve({
+ messageId: 'getNamedDestination_9',
+ namedDestinationView: 'XYZ,100,null,1.7',
+ pageNumber: 11
});
}
if (destination === 'DestWithFitR') {
return Promise.resolve({
- messageId: 'getNamedDestination_7',
+ messageId: 'getNamedDestination_10',
namedDestinationView: 'FitR,20,100,120,300',
pageNumber: 0
});
}
if (destination === 'DestWithFitRReversedCoordinates') {
return Promise.resolve({
- messageId: 'getNamedDestination_8',
+ messageId: 'getNamedDestination_11',
namedDestinationView: 'FitR,120,300,20,100',
pageNumber: 0
});
}
if (destination === 'DestWithFitRWithNull') {
return Promise.resolve({
- messageId: 'getNamedDestination_9',
+ messageId: 'getNamedDestination_12',
namedDestinationView: 'FitR,null,100,100,300',
pageNumber: 0
});
}
return Promise.resolve(
- {messageId: 'getNamedDestination_10', pageNumber: -1});
+ {messageId: 'getNamedDestination_13', pageNumber: -1});
});
const url = 'http://xyz.pdf';
@@ -138,9 +159,9 @@
chrome.test.assertEq(undefined, params.viewPosition);
// Checking #nameddest=name with a nameddest that specifies the view fit
- // type is "XYZ" with a zoom parameter of 0.
+ // type is "XYZ" with a zoom parameter of null.
params = await paramsParser.getViewportFromUrlParams(
- `${url}#nameddest=DestWithXYZAtZoom0`);
+ `${url}#nameddest=DestWithXYZAtZoomNull`);
chrome.test.assertEq(10, params.page);
chrome.test.assertEq(undefined, params.zoom);
chrome.test.assertEq(111, params.position.x);
@@ -148,12 +169,43 @@
chrome.test.assertEq(undefined, params.viewPosition);
// Checking #nameddest=name with a nameddest that specifies the view fit
- // type is "XYZ" and one of its parameters is null.
+ // type is "XYZ" and its X parameter is 0.
params = await paramsParser.getViewportFromUrlParams(
- `${url}#nameddest=DestWithXYZWithNullParameter`);
- chrome.test.assertEq(13, params.page);
- chrome.test.assertEq(undefined, params.zoom);
- chrome.test.assertEq(undefined, params.position);
+ `${url}#nameddest=DestWithXYZWithX0`);
+ chrome.test.assertEq(11, params.page);
+ chrome.test.assertEq(1.7, params.zoom);
+ chrome.test.assertEq(0, params.position.x);
+ chrome.test.assertEq(200, params.position.y);
+ chrome.test.assertEq(undefined, params.viewPosition);
+
+ // Checking #nameddest=name with a nameddest that specifies the view fit
+ // type is "XYZ" and its X parameter is null.
+ params = await paramsParser.getViewportFromUrlParams(
+ `${url}#nameddest=DestWithXYZWithXNull`);
+ chrome.test.assertEq(11, params.page);
+ chrome.test.assertEq(1.7, params.zoom);
+ chrome.test.assertTrue(Number.isNaN(params.position.x));
+ chrome.test.assertEq(200, params.position.y);
+ chrome.test.assertEq(undefined, params.viewPosition);
+
+ // Checking #nameddest=name with a nameddest that specifies the view fit
+ // type is "XYZ" and its Y parameter is 0.
+ params = await paramsParser.getViewportFromUrlParams(
+ `${url}#nameddest=DestWithXYZWithY0`);
+ chrome.test.assertEq(11, params.page);
+ chrome.test.assertEq(1.7, params.zoom);
+ chrome.test.assertEq(100, params.position.x);
+ chrome.test.assertEq(0, params.position.y);
+ chrome.test.assertEq(undefined, params.viewPosition);
+
+ // Checking #nameddest=name with a nameddest that specifies the view fit
+ // type is "XYZ" and its Y parameter is null.
+ params = await paramsParser.getViewportFromUrlParams(
+ `${url}#nameddest=DestWithXYZWithYNull`);
+ chrome.test.assertEq(11, params.page);
+ chrome.test.assertEq(1.7, params.zoom);
+ chrome.test.assertEq(100, params.position.x);
+ chrome.test.assertTrue(Number.isNaN(params.position.y));
chrome.test.assertEq(undefined, params.viewPosition);
// Checking #nameddest=name with a nameddest that specifies the view fit
diff --git a/pdf/out_of_process_instance.cc b/pdf/out_of_process_instance.cc
index 97630d1..0d829ac 100644
--- a/pdf/out_of_process_instance.cc
+++ b/pdf/out_of_process_instance.cc
@@ -1750,9 +1750,13 @@
if (named_destination && !named_destination->view.empty()) {
std::ostringstream view_stream;
view_stream << named_destination->view;
- for (unsigned long i = 0; i < named_destination->num_params; ++i)
- view_stream << "," << named_destination->params[i];
+ if (named_destination->xyz_params.empty()) {
+ for (unsigned long i = 0; i < named_destination->num_params; ++i)
+ view_stream << "," << named_destination->params[i];
+ } else {
+ view_stream << "," << named_destination->xyz_params;
+ }
reply.Set(pp::Var(kJSNamedDestinationView), view_stream.str());
}
PostMessage(reply);
diff --git a/pdf/pdf_engine.h b/pdf/pdf_engine.h
index b41a8527..b0eac060 100644
--- a/pdf/pdf_engine.h
+++ b/pdf/pdf_engine.h
@@ -113,6 +113,13 @@
// into the corresponding in-screen coordinates before it's sent to the
// viewport.
float params[kMaxViewParams];
+
+ // A string of parameters for view fit type XYZ in the format of "x,y,zoom",
+ // where x and y parameters are the in-screen coordinates and zoom is the
+ // zoom level. If a parameter is "null", then current value of that
+ // parameter in the viewport should be retained. Note: This string is empty
+ // if the view's fit type is not XYZ.
+ std::string xyz_params;
};
// Features in a document that are relevant to measure.
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index 1301976..21d6aa78 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -380,6 +380,46 @@
/*check_expected_size=*/true);
}
+std::string GetXYZParamsString(FPDF_DEST dest, PDFiumPage* page) {
+ FPDF_BOOL has_x_coord;
+ FPDF_BOOL has_y_coord;
+ FPDF_BOOL has_zoom;
+ FS_FLOAT x;
+ FS_FLOAT y;
+ FS_FLOAT zoom;
+ if (!FPDFDest_GetLocationInPage(dest, &has_x_coord, &has_y_coord, &has_zoom,
+ &x, &y, &zoom)) {
+ return "";
+ }
+
+ // Convert in-page coordinates to in-screen coordinates.
+ gfx::PointF xy(x, y);
+ gfx::PointF screen_coords = page->TransformPageToScreenXY(xy);
+
+ // Generate a string of the parameters
+ std::string xyz_params;
+ if (has_x_coord)
+ xyz_params = base::NumberToString(screen_coords.x()) + ",";
+ else
+ xyz_params = "null,";
+
+ if (has_y_coord)
+ xyz_params += base::NumberToString(screen_coords.y()) + ",";
+ else
+ xyz_params += "null,";
+
+ if (has_zoom) {
+ if (zoom == 0.0f)
+ NOTREACHED();
+
+ xyz_params += base::NumberToString(zoom);
+ } else {
+ xyz_params += "null";
+ }
+
+ return xyz_params;
+}
+
void SetXYZParamsInScreenCoords(PDFiumPage* page, float* params) {
gfx::PointF page_coords(params[0], params[1]);
gfx::PointF screen_coords = page->TransformPageToScreenXY(page_coords);
@@ -2497,6 +2537,9 @@
PDFiumPage* page_ptr = pages_[page].get();
ParamsTransformPageToScreen(view_int, page_ptr, result.params);
+ if (view_int == PDFDEST_VIEW_XYZ)
+ result.xyz_params = GetXYZParamsString(dest, page_ptr);
+
result.view = ConvertViewIntToViewString(view_int);
return result;
}