Handle null coordinates from XYZ named destination of an in-doc link.

For a XYZ fit type named destination, if the x or y coordinate is null,
that specific coordinate of the current viewport should be retained (See
table 151 in ISO 32000-1 for more details.). Also, one null parameter
does not invalidate other parameters, all parameters should be handled
independently.

Currently, the PDF viewer ignores both the x and y coordinates if either
one of them is null, and it always resets the viewport position to (0,0)
(top-left corner) without retaining the current coordinate which receives
a null input.

This CL does the following changes to fix this issue:
- Makes PDFiumPage::GetPageDestinationTarget() return x and y
  coordinates separately, so that each null input can be handled
  individually as an undefined value.
- Introduces retrieveCurrentScreenCoordinates() to calculate the current
  in-screen coordinates of the viewport, so that goToPageAndXY() can use
  its return value to retain x/y coordinate while navigating to a new
  destination.

Bug: 1163701,1157061
Change-Id: I4a4e8f1e193aab8ad2d5c8f3a277d0892e3e72c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632325Reviewed-by: dsinclair <dsinclair@chromium.org>
Commit-Queue: Hui Yingst <nigi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844663}

chrome/browser/resources/pdf/viewport.js

@@ -1337,23 +1337,41 @@ export class Viewport {
         this.fittingType_ === FittingType.FIT_TO_HEIGHT);
   }
 
+  /**
+   * Retrieves the in-screen coordinates of the current viewport position.
+   * @return {!Point} The current viewport position.
+   * @private
+   */
+  retrieveCurrentScreenCoordinates_() {
+    const currentPage = this.getMostVisiblePage();
+    const dimension = this.pageDimensions_[currentPage];
+    let toolbarOffset = 0;
+    if (!this.isPagedMode_()) {
+      toolbarOffset = this.topToolbarHeight_;
+    }
+    const x = this.position.x / this.getZoom() - dimension.x;
+    const y = (this.position.y + toolbarOffset) / this.getZoom() - dimension.y;
+    return {x: x, y: y};
+  }
+
   /**
    * Handles a navigation request to a destination from the current controller.
    * @param {number} page
-   * @param {number} x
-   * @param {number} y
+   * @param {number|undefined} x The in-screen x coordinate for the destination.
+   *     If `x` is undefined, retain current x coordinate value.
+   * @param {number|undefined} y The in-screen y coordinate for the destination.
+   *     If `y` is undefined, retain current y coordinate value.
    * @param {number} zoom
    */
   handleNavigateToDestination(page, x, y, zoom) {
     if (zoom) {
       this.setZoom(zoom);
     }
-
-    if (x || y) {
-      this.goToPageAndXY(page, x ? x : 0, y ? y : 0);
-    } else {
-      this.goToPage(page);
-    }
+    const currentCoords =
+        /** @type {!Point} */ (this.retrieveCurrentScreenCoordinates_());
+    this.goToPageAndXY(
+        page, x === undefined ? currentCoords.x : x,
+        y === undefined ? currentCoords.y : y);
   }
 
   /**

pdf/pdfium/pdfium_engine.cc

@@ -398,20 +398,6 @@ void SetFitRParamsInScreenCoords(PDFiumPage* page, float* params) {
   params[3] = point_2.y();
 }
 
-void SetFitVParamsInScreenCoords(PDFiumPage* page, float* params) {
-  // FitV/FitBV only has 1 parameter for x coordinate.
-  gfx::PointF screen_coords =
-      page->TransformPageToScreenXY(gfx::PointF(params[0], 0));
-  params[0] = screen_coords.x();
-}
-
-void SetFitHParamsInScreenCoords(PDFiumPage* page, float* params) {
-  // FitH/FitBH only has 1 parameter for y coordinate.
-  gfx::PointF screen_coords =
-      page->TransformPageToScreenXY(gfx::PointF(0, params[0]));
-  params[0] = screen_coords.y();
-}
-
 // A helper function that transforms the in-page coordinates in `params` to
 // in-screen coordinates depending on the view's fit type. `params` is both an
 // input and a output parameter.
@@ -428,11 +414,13 @@ void ParamsTransformPageToScreen(unsigned long view_fit_type,
       break;
     case PDFDEST_VIEW_FITBH:
     case PDFDEST_VIEW_FITH:
-      SetFitHParamsInScreenCoords(page, params);
+      // FitH/FitBH only has 1 parameter for y coordinate.
+      params[0] = page->TransformPageToScreenY(params[0]);
       break;
     case PDFDEST_VIEW_FITBV:
     case PDFDEST_VIEW_FITV:
-      SetFitVParamsInScreenCoords(page, params);
+      // FitV/FitBV only has 1 parameter for x coordinate.
+      params[0] = page->TransformPageToScreenX(params[0]);
       break;
     case PDFDEST_VIEW_FITR:
       SetFitRParamsInScreenCoords(page, params);
@@ -2372,16 +2360,17 @@ base::Value PDFiumEngine::TraverseBookmarks(FPDF_BOOKMARK bookmark,
     if (PageIndexInBounds(page_index)) {
       dict.SetIntKey("page", page_index);
 
-      base::Optional<gfx::PointF> xy;
+      base::Optional<float> x;
+      base::Optional<float> y;
       base::Optional<float> zoom;
-      pages_[page_index]->GetPageDestinationTarget(dest, &xy, &zoom);
-      if (xy) {
-        dict.SetIntKey("x", static_cast<int>(xy.value().x()));
-        dict.SetIntKey("y", static_cast<int>(xy.value().y()));
-      }
-      if (zoom) {
+      pages_[page_index]->GetPageDestinationTarget(dest, &x, &y, &zoom);
+
+      if (x)
+        dict.SetIntKey("x", static_cast<int>(x.value()));
+      if (y)
+        dict.SetIntKey("y", static_cast<int>(y.value()));
+      if (zoom)
         dict.SetDoubleKey("zoom", zoom.value());
-      }
     }
   } else {
     // Extract URI for bookmarks linking to an external page.

pdf/pdfium/pdfium_page.cc

@@ -849,21 +849,23 @@ PDFiumPage::Area PDFiumPage::GetDestinationTarget(FPDF_DEST destination,
 
   target->page = page_index;
 
-  base::Optional<gfx::PointF> xy;
-  GetPageDestinationTarget(destination, &xy, &target->zoom);
-  if (xy) {
-    gfx::PointF point = TransformPageToScreenXY(xy.value());
-    target->x_in_pixels = point.x();
-    target->y_in_pixels = point.y();
-  }
+  base::Optional<float> x;
+  base::Optional<float> y;
+  GetPageDestinationTarget(destination, &x, &y, &target->zoom);
+  if (x)
+    target->x_in_pixels = TransformPageToScreenX(x.value());
+  if (y)
+    target->y_in_pixels = TransformPageToScreenY(y.value());
 
   return DOCLINK_AREA;
 }
 
 void PDFiumPage::GetPageDestinationTarget(FPDF_DEST destination,
-                                          base::Optional<gfx::PointF>* xy,
+                                          base::Optional<float>* dest_x,
+                                          base::Optional<float>* dest_y,
                                           base::Optional<float>* zoom_value) {
-  *xy = base::nullopt;
+  *dest_x = base::nullopt;
+  *dest_y = base::nullopt;
   *zoom_value = base::nullopt;
   if (!available_)
     return;
@@ -880,9 +882,10 @@ void PDFiumPage::GetPageDestinationTarget(FPDF_DEST destination,
   if (!success)
     return;
 
-  if (has_x_coord && has_y_coord)
-    *xy = gfx::PointF(x, y);
-
+  if (has_x_coord)
+    *dest_x = x;
+  if (has_y_coord)
+    *dest_y = y;
   if (has_zoom)
     *zoom_value = zoom;
 }
@@ -896,6 +899,14 @@ gfx::PointF PDFiumPage::TransformPageToScreenXY(const gfx::PointF& xy) {
   return gfx::PointF(pixel_rect.x(), pixel_rect.y());
 }
 
+float PDFiumPage::TransformPageToScreenX(float x) {
+  return TransformPageToScreenXY(gfx::PointF(x, 0)).x();
+}
+
+float PDFiumPage::TransformPageToScreenY(float y) {
+  return TransformPageToScreenXY(gfx::PointF(0, y)).y();
+}
+
 PDFiumPage::Area PDFiumPage::GetURITarget(FPDF_ACTION uri_action,
                                           LinkTarget* target) const {
   if (target) {

pdf/pdfium/pdfium_page.h

@@ -111,15 +111,22 @@ class PDFiumPage {
   // NONSELECTABLE_AREA if link detection failed.
   Area GetLinkTarget(FPDF_LINK link, LinkTarget* target);
 
-  // Fills the output params with the (x, y) position in page coordinates and
-  // zoom value of a destination.
+  // Fills the output params with the in-page coordinates and the zoom value of
+  // the destination.
   void GetPageDestinationTarget(FPDF_DEST destination,
-                                base::Optional<gfx::PointF>* xy,
+                                base::Optional<float>* dest_x,
+                                base::Optional<float>* dest_y,
                                 base::Optional<float>* zoom_value);
 
   // Transforms an (x, y) position in page coordinates to screen coordinates.
   gfx::PointF TransformPageToScreenXY(const gfx::PointF& xy);
 
+  // Transforms an in-page x coordinate to its value in screen coordinates.
+  float TransformPageToScreenX(float x);
+
+  // Transforms an in-page y coordinate to its value in screen coordinates.
+  float TransformPageToScreenY(float y);
+
   // Given a point in the document that's in this page, returns its character
   // index if it's near a character, and also the type of text.
   // Target is optional. It will be filled in for WEBLINK_AREA or