Fixed the return values of FPDFAnnot_Get{Rect|AttachmentPoints}

Currently, FPDFAnnot_Get{Rect|AttachmentPoints} would return BBox if the
annotation's appearance stream is defined, which does not make much
sense to the user since BBox is defined in the form space; upon
rendering, the annotation's position in the device space is solely
defined by its rectangle.

This CL removes references to BBox in FPDFAnnot_Get{Rect|
AttachmentPoints}; instead, they will only return rectangle or
quadpoints as requested. The embedder test is also modified to reflect
this change, and also to demonstrate that an annotation's position is
changed only if its rectangle is changed.

Bug=pdfium:861

Change-Id: I489876511aa5d93131dd695170d46bbc49d16574
Reviewed-on: https://pdfium-review.googlesource.com/11050
Commit-Queue: Jane Liu <janeliulwq@google.com>
Reviewed-by: dsinclair <dsinclair@chromium.org>

2 files changed, 65 insertions, 90 deletions

diff --git a/fpdfsdk/fpdfannot.cpp b/fpdfsdk/fpdfannot.cpp
index fee99c844b..5c4aae2075 100644
--- a/fpdfsdk/fpdfannot.cpp
+++ b/fpdfsdk/fpdfannot.cpp
@@ -601,45 +601,18 @@ FPDFAnnot_GetAttachmentPoints(FPDF_ANNOTATION annot,
   if (!pAnnotDict)
     return false;
 
-  // If the annotation's appearance stream is defined, then retrieve the
-  // quadpoints defined by the "BBox" entry in the AP dictionary, since its
-  // "BBox" entry comes from annotation dictionary's "QuadPoints" entry, but
-  // takes priority over "QuadPoints" when rendering. Otherwise, retrieve
-  // the "Quadpoints" entry from the annotation dictionary.
-  CPDF_Array* pArray;
-  CPDF_Stream* pStream =
-      FPDFDOC_GetAnnotAP(pAnnotDict, CPDF_Annot::AppearanceMode::Normal);
-  if (pStream) {
-    pArray = pStream->GetDict()->GetArrayFor("BBox");
-    if (!pArray)
-      return false;
-
-    // Convert the BBox array into quadpoint coordinates. BBox array follows the
-    // order of a rectangle array: (left, bottom, right, up); and quadpoints
-    // follows the following order: (top-left vertex, top-right vertex, bottom-
-    // left vertex, bottom-right vertex).
-    quadPoints->x1 = pArray->GetNumberAt(0);
-    quadPoints->y1 = pArray->GetNumberAt(3);
-    quadPoints->x2 = pArray->GetNumberAt(2);
-    quadPoints->y2 = pArray->GetNumberAt(3);
-    quadPoints->x3 = pArray->GetNumberAt(0);
-    quadPoints->y3 = pArray->GetNumberAt(1);
-    quadPoints->x4 = pArray->GetNumberAt(2);
-    quadPoints->y4 = pArray->GetNumberAt(1);
-  } else {
-    pArray = pAnnotDict->GetArrayFor("QuadPoints");
-    if (!pArray)
-      return false;
-
-    quadPoints->x1 = pArray->GetNumberAt(0);
-    quadPoints->y1 = pArray->GetNumberAt(1);
-    quadPoints->x2 = pArray->GetNumberAt(2);
-    quadPoints->y2 = pArray->GetNumberAt(3);
-    quadPoints->x3 = pArray->GetNumberAt(4);
-    quadPoints->y3 = pArray->GetNumberAt(5);
-    quadPoints->x4 = pArray->GetNumberAt(6);
-    quadPoints->y4 = pArray->GetNumberAt(7);
-  }
+  CPDF_Array* pArray = pAnnotDict->GetArrayFor("QuadPoints");
+  if (!pArray)
+    return false;
+
+  quadPoints->x1 = pArray->GetNumberAt(0);
+  quadPoints->y1 = pArray->GetNumberAt(1);
+  quadPoints->x2 = pArray->GetNumberAt(2);
+  quadPoints->y2 = pArray->GetNumberAt(3);
+  quadPoints->x3 = pArray->GetNumberAt(4);
+  quadPoints->y3 = pArray->GetNumberAt(5);
+  quadPoints->x4 = pArray->GetNumberAt(6);
+  quadPoints->y4 = pArray->GetNumberAt(7);
   return true;
 }
 
@@ -683,20 +656,7 @@ FPDF_EXPORT FPDF_BOOL FPDF_CALLCONV FPDFAnnot_GetRect(FPDF_ANNOTATION annot,
   if (!pAnnotDict)
     return false;
 
-  // If the annotation's appearance stream is defined and the annotation is of
-  // a type that does not have quadpoints, then retrieve the rectangle defined
-  // by the "BBox" entry in the AP dictionary, since its "BBox" entry comes
-  // from annotation dictionary's "Rect" entry, but takes priority over "Rect"
-  // when rendering. Otherwise, retrieve the "Rect" entry from the annotation
-  // dictionary.
-  CFX_FloatRect rt;
-  CPDF_Stream* pStream =
-      FPDFDOC_GetAnnotAP(pAnnotDict, CPDF_Annot::AppearanceMode::Normal);
-  if (!pStream || FPDFAnnot_HasAttachmentPoints(annot))
-    rt = pAnnotDict->GetRectFor("Rect");
-  else
-    rt = pStream->GetDict()->GetRectFor("BBox");
-
+  CFX_FloatRect rt = pAnnotDict->GetRectFor("Rect");
   rect->left = rt.left;
   rect->bottom = rt.bottom;
   rect->right = rt.right;
diff --git a/fpdfsdk/fpdfannot_embeddertest.cpp b/fpdfsdk/fpdfannot_embeddertest.cpp
index 32edd7a503..4aca839226 100644
--- a/fpdfsdk/fpdfannot_embeddertest.cpp
+++ b/fpdfsdk/fpdfannot_embeddertest.cpp
@@ -317,12 +317,31 @@ TEST_F(FPDFAnnotEmbeddertest, AddAndSaveUnderlineAnnotation) {
 }
 
 TEST_F(FPDFAnnotEmbeddertest, ModifyRectQuadpointsWithAP) {
+#if _FXM_PLATFORM_ == _FXM_PLATFORM_APPLE_
+  const char md5_original[] = "63af8432fab95a67cdebb7cd0e514941";
+  const char md5_modified_highlight[] = "aec26075011349dec9bace891856b5f2";
+  const char md5_modified_square[] = "057f57a32be95975775e5ec513fdcb56";
+#elif _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
+  const char md5_original[] = "ade6762a70e85605546ce067e7d2148f";
+  const char md5_modified_highlight[] = "fb8440ed1a070b53ed5598ce7451cfad";
+  const char md5_modified_square[] = "7925f6726b343393f258e8b4e93dd65d";
+#else
+  const char md5_original[] = "ade6762a70e85605546ce067e7d2148f";
+  const char md5_modified_highlight[] = "fb8440ed1a070b53ed5598ce7451cfad";
+  const char md5_modified_square[] = "7925f6726b343393f258e8b4e93dd65d";
+#endif
+
   // Open a file with four annotations and load its first page.
   ASSERT_TRUE(OpenDocument("annotation_highlight_square_with_ap.pdf"));
   FPDF_PAGE page = FPDF_LoadPage(document(), 0);
   ASSERT_TRUE(page);
   EXPECT_EQ(4, FPDFPage_GetAnnotCount(page));
 
+  // Check that the original file renders correctly.
+  FPDF_BITMAP bitmap = RenderPageWithFlags(page, form_handle_, FPDF_ANNOT);
+  CompareBitmap(bitmap, 612, 792, md5_original);
+  FPDFBitmap_Destroy(bitmap);
+
   // Retrieve the highlight annotation which has its AP stream already defined.
   FPDF_ANNOTATION annot = FPDFPage_GetAnnot(page, 0);
   ASSERT_TRUE(annot);
@@ -332,41 +351,33 @@ TEST_F(FPDFAnnotEmbeddertest, ModifyRectQuadpointsWithAP) {
   EXPECT_FALSE(
       FPDFAnnot_SetColor(annot, FPDFANNOT_COLORTYPE_Color, 51, 102, 153, 204));
 
-  // Check that when getting the attachment points, bounding box points are
-  // returned since this is a markup annotation with AP defined.
+  // Verify its attachment points.
   FS_QUADPOINTSF quadpoints;
   ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(annot, &quadpoints));
-  EXPECT_NEAR(0.f, quadpoints.x1, 0.001f);
-  EXPECT_NEAR(16.9955f, quadpoints.y1, 0.001f);
-  EXPECT_NEAR(68.5953f, quadpoints.x4, 0.001f);
-  EXPECT_NEAR(0.f, quadpoints.y4, 0.001f);
-
-  // Check that when new attachment points define a smaller bounding box, the
-  // bounding box does not get updated.
-  quadpoints.x1 = 1.0f;
-  quadpoints.x3 = 1.0f;
+  EXPECT_NEAR(72.0000f, quadpoints.x1, 0.001f);
+  EXPECT_NEAR(720.792f, quadpoints.y1, 0.001f);
+  EXPECT_NEAR(132.055f, quadpoints.x4, 0.001f);
+  EXPECT_NEAR(704.796f, quadpoints.y4, 0.001f);
+
+  // Check that updating the attachment points would succeed.
+  quadpoints.x1 -= 50.f;
+  quadpoints.x2 -= 50.f;
+  quadpoints.x3 -= 50.f;
+  quadpoints.x4 -= 50.f;
   ASSERT_TRUE(FPDFAnnot_SetAttachmentPoints(annot, &quadpoints));
   FS_QUADPOINTSF new_quadpoints;
   ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(annot, &new_quadpoints));
-  EXPECT_NE(quadpoints.x1, new_quadpoints.x1);
-
-  // Check that the bounding box gets updated successfully when valid attachment
-  // points are set.
-  quadpoints.x1 = 0.f;
-  quadpoints.y1 = 721.792f;
-  quadpoints.x2 = 133.055f;
-  quadpoints.y2 = 721.792f;
-  quadpoints.x3 = 0.f;
-  quadpoints.x4 = 133.055f;
-  ASSERT_TRUE(FPDFAnnot_SetAttachmentPoints(annot, &quadpoints));
-  ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(annot, &new_quadpoints));
   EXPECT_EQ(quadpoints.x1, new_quadpoints.x1);
   EXPECT_EQ(quadpoints.y1, new_quadpoints.y1);
   EXPECT_EQ(quadpoints.x4, new_quadpoints.x4);
   EXPECT_EQ(quadpoints.y4, new_quadpoints.y4);
 
-  // Check that when getting the annotation rectangle, rectangle points are
-  // returned, but not bounding box points.
+  // Check that updating quadpoints does not change the annotation's position.
+  bitmap = RenderPageWithFlags(page, form_handle_, FPDF_ANNOT);
+  CompareBitmap(bitmap, 612, 792, md5_original);
+  FPDFBitmap_Destroy(bitmap);
+
+  // Verify its annotation rectangle.
   FS_RECTF rect;
   ASSERT_TRUE(FPDFAnnot_GetRect(annot, &rect));
   EXPECT_NEAR(67.7299f, rect.left, 0.001f);
@@ -374,34 +385,38 @@ TEST_F(FPDFAnnotEmbeddertest, ModifyRectQuadpointsWithAP) {
   EXPECT_NEAR(136.325f, rect.right, 0.001f);
   EXPECT_NEAR(721.292f, rect.top, 0.001f);
 
-  // Check that the rectangle gets updated successfully when a valid rectangle
-  // is set, and that the bounding box is not modified.
-  rect.left = 0.f;
-  rect.bottom = 0.f;
-  rect.right = 134.055f;
-  rect.top = 722.792f;
+  // Check that updating the rectangle would succeed.
+  rect.left -= 60.f;
+  rect.right -= 60.f;
   ASSERT_TRUE(FPDFAnnot_SetRect(annot, &rect));
   FS_RECTF new_rect;
   ASSERT_TRUE(FPDFAnnot_GetRect(annot, &new_rect));
   EXPECT_EQ(rect.right, new_rect.right);
-  ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(annot, &new_quadpoints));
-  EXPECT_NE(rect.right, new_quadpoints.x2);
-
   FPDFPage_CloseAnnot(annot);
 
+  // Check that updating the rectangle changes the annotation's position.
+  bitmap = RenderPageWithFlags(page, form_handle_, FPDF_ANNOT);
+  CompareBitmap(bitmap, 612, 792, md5_modified_highlight);
+  FPDFBitmap_Destroy(bitmap);
+
   // Retrieve the square annotation which has its AP stream already defined.
   annot = FPDFPage_GetAnnot(page, 2);
   ASSERT_TRUE(annot);
   EXPECT_EQ(FPDF_ANNOT_SQUARE, FPDFAnnot_GetSubtype(annot));
 
-  // Check that the rectangle and the bounding box get updated successfully when
-  // a valid rectangle is set, since this is not a markup annotation.
+  // Check that updating the rectangle would succeed.
   ASSERT_TRUE(FPDFAnnot_GetRect(annot, &rect));
-  rect.right += 1.f;
+  rect.left += 70.f;
+  rect.right += 70.f;
   ASSERT_TRUE(FPDFAnnot_SetRect(annot, &rect));
   ASSERT_TRUE(FPDFAnnot_GetRect(annot, &new_rect));
   EXPECT_EQ(rect.right, new_rect.right);
 
+  // Check that updating the rectangle changes the square annotation's position.
+  bitmap = RenderPageWithFlags(page, form_handle_, FPDF_ANNOT);
+  CompareBitmap(bitmap, 612, 792, md5_modified_square);
+  FPDFBitmap_Destroy(bitmap);
+
   FPDFPage_CloseAnnot(annot);
   UnloadPage(page);
 }