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 6525ff620cc282ffb24e54d50fc31091395d7722..8dc3e3d170bd8ec0623c66de8d7d482d8297c009 100644
--- a/core/fpdfdoc/cpdf_annot.cpp
+++ b/core/fpdfdoc/cpdf_annot.cpp
@@ -18,10 +18,22 @@
 #include "core/fxge/include/cfx_pathdata.h"
 #include "core/fxge/include/cfx_renderdevice.h"
 
+namespace {

[Lei Zhang, 2016/09/01 21:12:59]

When not forward declaring, add blank lines after the start of a namespace and
before the end of the namespace.

[tonikitoo, 2016/09/01 21:50:41]

Done.

+bool ShouldGenerateAPForAnnotation(CPDF_Dictionary* pAnnotDict) {
+  // If AP dictionary exists, we use the appearance defined in the
+  // existing AP dictionary.
+  if (pAnnotDict->KeyExist("AP"))
+    return false;
+
+  return !CPDF_Annot::IsAnnotationHidden(pAnnotDict);
+}
+}  // namespace
+
 CPDF_Annot::CPDF_Annot(CPDF_Dictionary* pDict, CPDF_Document* pDocument)
     : m_pAnnotDict(pDict),
       m_pDocument(pDocument),
       m_bOpenState(false),
+      m_bHasGeneratedAP(false),
       m_pPopupAnnot(nullptr) {
   m_nSubtype = StringToAnnotSubtype(m_pAnnotDict->GetStringBy("Subtype"));
   GenerateAPIfNeeded();
@@ -32,24 +44,29 @@ CPDF_Annot::~CPDF_Annot() {
 }
 
 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() {
@@ -60,11 +77,40 @@ CPDF_Annot::Subtype CPDF_Annot::GetSubtype() const {
   return m_nSubtype;
 }
 
+bool CPDF_Annot::ShouldUseQuadPointsCoords(CPDF_Array*& pArray) const {

[Tom Sepez, 2016/09/01 20:53:08]

nit: can we make this a CPDF_Array** argument, since the style guide prefers
that rather than references for out parameters?

[tonikitoo, 2016/09/01 21:50:41]

Done.

+  if (!m_pAnnotDict)
+    return false;
+
+  pArray = m_pAnnotDict->GetArrayBy("QuadPoints");
+  if (!pArray)
+    return false;
+
+  return m_bHasGeneratedAP;
+}
+
 CFX_FloatRect CPDF_Annot::GetRect() const {
   if (!m_pAnnotDict)
     return CFX_FloatRect();
 
-  CFX_FloatRect rect = m_pAnnotDict->GetRectBy("Rect");
+  CFX_FloatRect rect;
+  CPDF_Array* pArray = nullptr;
+  if (ShouldUseQuadPointsCoords(pArray)) {
+    // QuadPoints are defined with 4 pairs of numbers
+    // ([ pair0, pair1, pair2, pair3 ]), where
+    // pair0= top_left

[Lei Zhang, 2016/09/01 21:12:59]

add a space before '=', to be consistent with lines 106/107.

[tonikitoo, 2016/09/01 21:50:41]

Done.

+    // 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.
+    rect = CFX_FloatRect(pArray->GetNumberAt(4), pArray->GetNumberAt(5),
+                         pArray->GetNumberAt(2), pArray->GetNumberAt(3));
+  } else {
+    rect = m_pAnnotDict->GetRectBy("Rect");
+  }
+

[jaepark, 2016/09/01 20:52:00]

Even if we changed the rect for CPDF_Annot::GetRect() here,
CPVT_GenerateAP::Generate*AP still uses pAnnotDict->GetRectBy("Rect"). So the
appearance stream for all the annotations are still unchanged. So how did this
affect modification of current test cases?

[tonikitoo, 2016/09/01 21:50:41]

This is a good point. I will check further how the generated "/bbox" plays
together with quadpoints/rect.

For completeness, this is the stack trace of when ::GetRect is called when
loading [1] in chromium/pdfium:

#0 0x562838b5d03e base::debug::StackTrace::StackTrace()
#1 0x56283b6a5585 CPDF_Annot::GetRect()
#2 0x56283b6a5e35 FPDFDOC_Annot_GetMatrix()
#3 0x56283b6a5cf8 CPDF_Annot::DrawAppearance()
#4 0x56283b630c99 CPDFSDK_PageView::PageView_OnDraw()
#5 0x56283b62b7ce FPDF_FFLDraw
#6 0x56283882ed7c chrome_pdf::PDFiumEngine::FinishPaint()
#7 0x56283882e852 chrome_pdf::PDFiumEngine::Paint()
#8 0x562838842b26 chrome_pdf::OutOfProcessInstance::OnPaint()
#9 0x562838847b82 PaintManager::DoPaint()
<..>


[1] https://storage.googleapis.com/pdf-test/test.pdf?calcrtid=Tag-1

   rect.Normalize();
   return rect;
 }