Use /RECT or /QuadPoints for annotation coordinates, depending on /AP

core/fpdfdoc/cpdf_annot.cpp

Index: core/fpdfdoc/cpdf_annot.cpp
diff --git a/core/fpdfdoc/cpdf_annot.cpp b/core/fpdfdoc/cpdf_annot.cpp
index 4e0b04e9b623195c36ff47093d66f53cf7d53941..4d52da6604a7af8d3f33c5179c56c81c382c18a2 100644
--- a/core/fpdfdoc/cpdf_annot.cpp
+++ b/core/fpdfdoc/cpdf_annot.cpp
@@ -20,6 +20,13 @@
 
 namespace {
 
+bool IsTextMarkupAnnotation(CPDF_Annot::Subtype type) {
+  return type == CPDF_Annot::Subtype::HIGHLIGHT ||
+         type == CPDF_Annot::Subtype::SQUIGGLY ||
+         type == CPDF_Annot::Subtype::STRIKEOUT ||
+         type == CPDF_Annot::Subtype::UNDERLINE;
+}
+
 bool ShouldGenerateAPForAnnotation(CPDF_Dictionary* pAnnotDict) {
   // If AP dictionary exists, we use the appearance defined in the
   // existing AP dictionary.
@@ -38,8 +45,10 @@ CPDF_Annot::CPDF_Annot(CPDF_Dictionary* pDict,
       m_pAnnotDict(pDict),
       m_pDocument(pDocument),
       m_bOpenState(false),
+      m_bHasGeneratedAP(false),
       m_pPopupAnnot(nullptr) {
   m_nSubtype = StringToAnnotSubtype(m_pAnnotDict->GetStringBy("Subtype"));
+  m_bIsMarkupAnnotation = IsTextMarkupAnnotation(m_nSubtype);

[jaepark, 2016/09/07 17:30:09]

m_bIsTextMarkupAnnotation

[tonikitoo, 2016/09/08 04:10:35]

Done.

   GenerateAPIfNeeded();
 }
 
@@ -53,24 +62,26 @@ void CPDF_Annot::GenerateAPIfNeeded() {
   if (!ShouldGenerateAPForAnnotation(m_pAnnotDict))
     return;
 
+  bool result = false;
   if (m_nSubtype == CPDF_Annot::Subtype::CIRCLE)
-    CPVT_GenerateAP::GenerateCircleAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateCircleAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::HIGHLIGHT)
-    CPVT_GenerateAP::GenerateHighlightAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateHighlightAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::INK)
-    CPVT_GenerateAP::GenerateInkAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateInkAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::POPUP)
-    CPVT_GenerateAP::GeneratePopupAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GeneratePopupAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::SQUARE)
-    CPVT_GenerateAP::GenerateSquareAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateSquareAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::SQUIGGLY)
-    CPVT_GenerateAP::GenerateSquigglyAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateSquigglyAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::STRIKEOUT)
-    CPVT_GenerateAP::GenerateStrikeOutAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateStrikeOutAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::TEXT)
-    CPVT_GenerateAP::GenerateTextAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateTextAP(m_pDocument, m_pAnnotDict);
   else if (m_nSubtype == CPDF_Annot::Subtype::UNDERLINE)
-    CPVT_GenerateAP::GenerateUnderlineAP(m_pDocument, m_pAnnotDict);
+    result = CPVT_GenerateAP::GenerateUnderlineAP(m_pDocument, m_pAnnotDict);
+  m_bHasGeneratedAP = result;
 }
 
 void CPDF_Annot::ClearCachedAP() {
@@ -81,11 +92,22 @@ CPDF_Annot::Subtype CPDF_Annot::GetSubtype() const {
   return m_nSubtype;
 }
 
+CFX_FloatRect CPDF_Annot::RectForDrawing() const {
+  if (!m_pAnnotDict)
+    return CFX_FloatRect();
+
+  bool shouldUseQuadPointsCoords = m_bIsMarkupAnnotation && m_bHasGeneratedAP;

[jaepark, 2016/09/07 17:30:09]

To me, checking m_bHasGeneratedAP in RectForDrawing seems a little bit odd. Why
do we need to check if annot has generated AP? 

RectForDrawing should only check whether an annot is a text markup annotation.
If it is, use QuadPoints, if not, use Rect.

[tonikitoo, 2016/09/08 04:10:35]

Checking for m_bHasGeneratedAP is important here because it toggles using either
QuadPoints or Rect coords.

In other words, when /AP is generated (i.e. it is absent in the PDF source) is
exactly the case we want to use /QuadPoints.

[jaepark, 2016/09/08 17:49:27]

Acknowledged.

+  if (shouldUseQuadPointsCoords)
+    return RectFromQuads(m_pAnnotDict);
+
+  return m_pAnnotDict->GetRectBy("Rect");
+}
+
 CFX_FloatRect CPDF_Annot::GetRect() const {
   if (!m_pAnnotDict)
     return CFX_FloatRect();
 
-  CFX_FloatRect rect = m_pAnnotDict->GetRectBy("Rect");
+  CFX_FloatRect rect = RectForDrawing();
   rect.Normalize();
   return rect;
 }
@@ -166,6 +188,26 @@ static CPDF_Form* FPDFDOC_Annot_GetMatrix(const CPDF_Page* pPage,
 }
 
 // Static.
+CFX_FloatRect CPDF_Annot::RectFromQuads(CPDF_Dictionary* pAnnotDict) {

[jaepark, 2016/09/07 17:30:09]

RectFromQuadPoints

[tonikitoo, 2016/09/08 04:10:35]

Done.

+  CPDF_Array* pArray = pAnnotDict->GetArrayBy("QuadPoints");
+  if (!pArray)
+    return CFX_FloatRect();
+
+  // QuadPoints are defined with 4 pairs of numbers
+  // ([ pair0, pair1, pair2, pair3 ]), where
+  // pair0 = top_left
+  // pair1 = top_right
+  // pair2 = bottom_left
+  // pair3 = bottom_right
+  //
+  // On the other hand, /Rect is define as 2 pairs [pair0, pair1] where:
+  // pair0 = bottom_left
+  // pair1 = top_right.
+  return CFX_FloatRect(pArray->GetNumberAt(4), pArray->GetNumberAt(5),
+                       pArray->GetNumberAt(2), pArray->GetNumberAt(3));
+}
+
+// Static.
 bool CPDF_Annot::IsAnnotationHidden(CPDF_Dictionary* pAnnotDict) {
   return !!(pAnnotDict->GetIntegerBy("F") & ANNOTFLAG_HIDDEN);
 }