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>

3 files changed, 71 insertions, 98 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);
 }
diff --git a/public/fpdf_annot.h b/public/fpdf_annot.h
index 1de99453fc..6cd06dd9db 100644
--- a/public/fpdf_annot.h
+++ b/public/fpdf_annot.h
@@ -283,7 +283,8 @@ FPDFAnnot_HasAttachmentPoints(FPDF_ANNOTATION annot);
 // Experimental API.
 // Set the attachment points (i.e. quadpoints) of an annotation. If the
 // annotation's appearance stream is defined and this annotation is of a type
-// with quadpoints, then update the bounding box too.
+// with quadpoints, then update the bounding box too if the new quadpoints
+// define a bigger one.
 //
 //   annot      - handle to an annotation.
 //   quadPoints - the quadpoints to be set.
@@ -294,9 +295,7 @@ FPDFAnnot_SetAttachmentPoints(FPDF_ANNOTATION annot,
                               const FS_QUADPOINTSF* quadPoints);
 
 // Experimental API.
-// Get the attachment points (i.e. quadpoints) of an annotation. If the
-// annotation's appearance stream is defined and this annotation is of a type
-// with quadpoints, then return the bounding box it specifies instead.
+// Get the attachment points (i.e. quadpoints) of an annotation.
 //
 //   annot      - handle to an annotation.
 //   quadPoints - receives the quadpoints; must not be NULL.
@@ -309,7 +308,8 @@ FPDFAnnot_GetAttachmentPoints(FPDF_ANNOTATION annot,
 // Experimental API.
 // Set the annotation rectangle defining the location of the annotation. If the
 // annotation's appearance stream is defined and this annotation is of a type
-// without quadpoints, then update the bounding box too.
+// without quadpoints, then update the bounding box too if the new rectangle
+// defines a bigger one.
 //
 //   annot  - handle to an annotation.
 //   rect   - the annotation rectangle to be set.
@@ -319,9 +319,7 @@ FPDF_EXPORT FPDF_BOOL FPDF_CALLCONV FPDFAnnot_SetRect(FPDF_ANNOTATION annot,
                                                       const FS_RECTF* rect);
 
 // Experimental API.
-// Get the annotation rectangle defining the location of the annotation. If the
-// annotation's appearance stream is defined and this annotation is of a type
-// without quadpoints, then return the bounding box it specifies instead.
+// Get the annotation rectangle defining the location of the annotation.
 //
 //   annot  - handle to an annotation.
 //   rect   - receives the rectangle; must not be NULL.