Refactor PDFiumPage::GetLinkTarget() into smaller methods.

pdf/pdfium/pdfium_page.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "pdf/pdfium/pdfium_page.h"
 
 #include <math.h>
 #include <stddef.h>
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
 #include "base/logging.h"
 #include "base/numerics/safe_math.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "pdf/pdfium/pdfium_api_string_buffer_adapter.h"
 #include "pdf/pdfium/pdfium_engine.h"
 #include "printing/units.h"
 
-// Used when doing hit detection.
-#define kTolerance 20.0
-
 using printing::ConvertUnitDouble;
 using printing::kPointsPerInch;
 using printing::kPixelsPerInch;
 
 namespace {
 
 pp::FloatRect FloatPageRectToPixelRect(FPDF_PAGE page,
                                        const pp::FloatRect& input) {
   int output_width = FPDF_GetPageWidth(page);
   int output_height = FPDF_GetPageHeight(page);
@@ skipping 204 matching lines @@
                                           int* form_type,
                                           LinkTarget* target) {
   if (!available_)
     return NONSELECTABLE_AREA;
   pp::Point point2 = point - rect_.point();
   double new_x;
   double new_y;
   FPDF_DeviceToPage(GetPage(), 0, 0, rect_.width(), rect_.height(), rotation,
                     point2.x(), point2.y(), &new_x, &new_y);
 
+  // hit detection tolerance, in points.
+  constexpr double kTolerance = 20.0;
   int rv = FPDFText_GetCharIndexAtPos(GetTextPage(), new_x, new_y, kTolerance,
                                       kTolerance);
   *char_index = rv;
 
   FPDF_LINK link = FPDFLink_GetLinkAtPoint(GetPage(), new_x, new_y);
   int control =
       FPDFPage_HasFormFieldAtPoint(engine_->form(), GetPage(), new_x, new_y);
 
   // If there is a control and link at the same point, figure out their z-order
   // to determine which is on top.
   if (link && control > FPDF_FORMFIELD_UNKNOWN) {
     int control_z_order = FPDFPage_FormFieldZOrderAtPoint(
         engine_->form(), GetPage(), new_x, new_y);
     int link_z_order = FPDFLink_GetLinkZOrderAtPoint(GetPage(), new_x, new_y);
     DCHECK_NE(control_z_order, link_z_order);
     if (control_z_order > link_z_order) {
       *form_type = control;
       return FormTypeToArea(*form_type);
     }
 
     // We don't handle all possible link types of the PDF. For example,
     // launch actions, cross-document links, etc.
     // In that case, GetLinkTarget() will return NONSELECTABLE_AREA
     // and we should proceed with area detection.
-    PDFiumPage::Area area = GetLinkTarget(link, target);
-    if (area != PDFiumPage::NONSELECTABLE_AREA)
+    Area area = GetLinkTarget(link, target);
+    if (area != NONSELECTABLE_AREA)
       return area;
   } else if (link) {
     // We don't handle all possible link types of the PDF. For example,
     // launch actions, cross-document links, etc.
     // See identical block above.
-    PDFiumPage::Area area = GetLinkTarget(link, target);
-    if (area != PDFiumPage::NONSELECTABLE_AREA)
+    Area area = GetLinkTarget(link, target);
+    if (area != NONSELECTABLE_AREA)
       return area;
   } else if (control > FPDF_FORMFIELD_UNKNOWN) {
     *form_type = control;
     return FormTypeToArea(*form_type);
   }
 
   if (rv < 0)
     return NONSELECTABLE_AREA;
 
   return GetLink(*char_index, target) != -1 ? WEBLINK_AREA : TEXT_AREA;
 }
 
 // static
 PDFiumPage::Area PDFiumPage::FormTypeToArea(int form_type) {
   switch (form_type) {
     case FPDF_FORMFIELD_COMBOBOX:
     case FPDF_FORMFIELD_TEXTFIELD:
-      return PDFiumPage::FORM_TEXT_AREA;
+      return FORM_TEXT_AREA;
     default:
-      return PDFiumPage::NONSELECTABLE_AREA;
+      return NONSELECTABLE_AREA;
   }
 }
 
 base::char16 PDFiumPage::GetCharAtIndex(int index) {
   if (!available_)
     return L'\0';
   return static_cast<base::char16>(FPDFText_GetUnicode(GetTextPage(), index));
 }
 
 int PDFiumPage::GetCharCount() {
   if (!available_)
     return 0;
   return FPDFText_CountChars(GetTextPage());
 }
 
-PDFiumPage::Area PDFiumPage::GetLinkTarget(
-    FPDF_LINK link,
-    PDFiumPage::LinkTarget* target) const {
+PDFiumPage::Area PDFiumPage::GetLinkTarget(FPDF_LINK link,
+                                           LinkTarget* target) const {
   FPDF_DEST dest = FPDFLink_GetDest(engine_->doc(), link);
   if (dest)
     return GetDestinationTarget(dest, target);
 
   FPDF_ACTION action = FPDFLink_GetAction(link);
-  if (action) {
-    // TODO(gene): We don't support PDFACTION_REMOTEGOTO and
-    // PDFACTION_LAUNCH at the moment.
-    switch (FPDFAction_GetType(action)) {
-      case PDFACTION_GOTO: {
-        FPDF_DEST dest = FPDFAction_GetDest(engine_->doc(), action);
-        if (dest)
-          return GetDestinationTarget(dest, target);
-        // TODO(gene): We don't fully support all types of the in-document
-        // links. Need to implement that. There is a bug to track that:
-        // http://code.google.com/p/chromium/issues/detail?id=55776
-        break;
-      }
-      case PDFACTION_URI: {
-        if (target) {
-          size_t buffer_size =
-              FPDFAction_GetURIPath(engine_->doc(), action, nullptr, 0);
-          if (buffer_size > 0) {
-            PDFiumAPIStringBufferAdapter<std::string> api_string_adapter(
-                &target->url, buffer_size, true);
-            void* data = api_string_adapter.GetData();
-            size_t bytes_written = FPDFAction_GetURIPath(engine_->doc(), action,
-                                                         data, buffer_size);
-            api_string_adapter.Close(bytes_written);
-          }
-        }
-        return WEBLINK_AREA;
-      }
+  if (!action)
+    return NONSELECTABLE_AREA;
+
+  switch (FPDFAction_GetType(action)) {
+    case PDFACTION_GOTO: {
+      FPDF_DEST dest = FPDFAction_GetDest(engine_->doc(), action);
+      if (dest)
+        return GetDestinationTarget(dest, target);
+      // TODO(thestig): We don't fully support all types of the in-document
+      // links. Need to implement that. There is a bug to track that:
+      // https://crbug.com/55776
+      return NONSELECTABLE_AREA;
     }
+    case PDFACTION_URI:
+      return GetURITarget(action, target);
+    // TODO(thestig): Support remaining types for https://crbug.com/55776
+    case PDFACTION_LAUNCH:
+    case PDFACTION_REMOTEGOTO:
+    default:
+      return NONSELECTABLE_AREA;
   }
-
-  return NONSELECTABLE_AREA;
 }
 
-PDFiumPage::Area PDFiumPage::GetDestinationTarget(
-    FPDF_DEST destination,
-    PDFiumPage::LinkTarget* target) const {
+PDFiumPage::Area PDFiumPage::GetDestinationTarget(FPDF_DEST destination,
+                                                  LinkTarget* target) const {
   if (target)
     target->page = FPDFDest_GetPageIndex(engine_->doc(), destination);
   return DOCLINK_AREA;
 }
 
-int PDFiumPage::GetLink(int char_index, PDFiumPage::LinkTarget* target) {
+PDFiumPage::Area PDFiumPage::GetURITarget(FPDF_ACTION uri_action,
+                                          LinkTarget* target) const {
+  if (target) {

[dsinclair, 2017/06/26 19:44:38]

if (!target)
  return WEBLINK_AREA;

[Lei Zhang, 2017/06/26 20:57:59]

I wrote it this way because:
- Early returns ends up taking more lines.
- It's more obvious that this only ever returns WEBLINK_AREA given the single
return statement.
- It's short enough of a function and the nesting isn't too deep.

+    size_t buffer_size =
+        FPDFAction_GetURIPath(engine_->doc(), uri_action, nullptr, 0);
+    if (buffer_size > 0) {

[dsinclair, 2017/06/26 19:44:39]

ditto early exit?

+      PDFiumAPIStringBufferAdapter<std::string> api_string_adapter(
+          &target->url, buffer_size, true);
+      void* data = api_string_adapter.GetData();
+      size_t bytes_written =
+          FPDFAction_GetURIPath(engine_->doc(), uri_action, data, buffer_size);
+      api_string_adapter.Close(bytes_written);
+    }
+  }
+  return WEBLINK_AREA;
+}
+
+int PDFiumPage::GetLink(int char_index, LinkTarget* target) {
   if (!available_)
     return -1;
 
   CalculateLinks();
 
   // Get the bounding box of the rect again, since it might have moved because
   // of the tolerance above.
   double left, right, bottom, top;
   FPDFText_GetCharBox(GetTextPage(), char_index, &left, &right, &bottom, &top);
 
@@ skipping 154 matching lines @@
   page_->loading_count_--;
 }
 
 PDFiumPage::Link::Link() = default;
 
 PDFiumPage::Link::Link(const Link& that) = default;
 
 PDFiumPage::Link::~Link() = default;
 
 }  // namespace chrome_pdf