Changed the return type for annotation APIs

Before:
    When returning FPDF_ANNOTATION, the APIs would take in a
    FPDF_ANNTOATION* and write the handle of the annotation to it, while
    returning a boolean as status.

This CL:
    This CL changes the APIs to directly return FPDF_ANNOTATION, which
    would be null on failure.
    Also adds more null checks within the annotation APIs.

Bug=pdfium:737

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

3 files changed, 97 insertions, 81 deletions

diff --git a/fpdfsdk/fpdfannot.cpp b/fpdfsdk/fpdfannot.cpp
index 6d47b04be2..169c31c236 100644
--- a/fpdfsdk/fpdfannot.cpp
+++ b/fpdfsdk/fpdfannot.cpp
@@ -6,6 +6,7 @@
 
 #include "public/fpdf_annot.h"
 
+#include <memory>
 #include <utility>
 
 #include "core/fpdfapi/page/cpdf_form.h"
@@ -131,10 +132,6 @@ class CPDF_AnnotContext {
   CFX_UnownedPtr<CPDF_Page> m_pPage;
 };
 
-FPDF_ANNOTATION FPDFAnnotationFromCPDFAnnotContext(CPDF_AnnotContext* pAnnot) {
-  return static_cast<FPDF_ANNOTATION>(pAnnot);
-}
-
 CPDF_AnnotContext* CPDFAnnotContextFromFPDFAnnotation(FPDF_ANNOTATION annot) {
   return static_cast<CPDF_AnnotContext*>(annot);
 }
@@ -150,13 +147,11 @@ FPDFAnnot_IsSupportedSubtype(FPDF_ANNOTATION_SUBTYPE subtype) {
          subtype == FPDF_ANNOT_UNDERLINE;
 }
 
-DLLEXPORT FPDF_BOOL STDCALL
-FPDFPage_CreateAnnot(FPDF_PAGE page,
-                     FPDF_ANNOTATION_SUBTYPE subtype,
-                     FPDF_ANNOTATION* annot) {
+DLLEXPORT FPDF_ANNOTATION STDCALL
+FPDFPage_CreateAnnot(FPDF_PAGE page, FPDF_ANNOTATION_SUBTYPE subtype) {
   CPDF_Page* pPage = CPDFPageFromFPDFPage(page);
   if (!pPage || !FPDFAnnot_IsSupportedSubtype(subtype))
-    return false;
+    return nullptr;
 
   auto pDict = pdfium::MakeUnique<CPDF_Dictionary>(
       pPage->m_pDocument->GetByteStringPool());
@@ -164,18 +159,15 @@ FPDFPage_CreateAnnot(FPDF_PAGE page,
   pDict->SetNewFor<CPDF_Name>("Subtype",
                               CPDF_Annot::AnnotSubtypeToString(
                                   static_cast<CPDF_Annot::Subtype>(subtype)));
-  if (annot) {
-    auto pNewAnnot =
-        pdfium::MakeUnique<CPDF_AnnotContext>(pDict.get(), pPage, nullptr);
-    *annot = FPDFAnnotationFromCPDFAnnotContext(pNewAnnot.release());
-  }
+  auto pNewAnnot =
+      pdfium::MakeUnique<CPDF_AnnotContext>(pDict.get(), pPage, nullptr);
 
   CPDF_Array* pAnnotList = pPage->m_pFormDict->GetArrayFor("Annots");
   if (!pAnnotList)
     pAnnotList = pPage->m_pFormDict->SetNewFor<CPDF_Array>("Annots");
 
   pAnnotList->Add(std::move(pDict));
-  return true;
+  return pNewAnnot.release();
 }
 
 DLLEXPORT int STDCALL FPDFPage_GetAnnotCount(FPDF_PAGE page) {
@@ -187,21 +179,18 @@ DLLEXPORT int STDCALL FPDFPage_GetAnnotCount(FPDF_PAGE page) {
   return pAnnots ? pAnnots->GetCount() : 0;
 }
 
-DLLEXPORT FPDF_BOOL STDCALL FPDFPage_GetAnnot(FPDF_PAGE page,
-                                              int index,
-                                              FPDF_ANNOTATION* annot) {
+DLLEXPORT FPDF_ANNOTATION STDCALL FPDFPage_GetAnnot(FPDF_PAGE page, int index) {
   CPDF_Page* pPage = CPDFPageFromFPDFPage(page);
-  if (!pPage || !pPage->m_pFormDict || index < 0 || !annot)
-    return false;
+  if (!pPage || !pPage->m_pFormDict || index < 0)
+    return nullptr;
 
   CPDF_Array* pAnnots = pPage->m_pFormDict->GetArrayFor("Annots");
   if (!pAnnots || static_cast<size_t>(index) >= pAnnots->GetCount())
-    return false;
+    return nullptr;
 
   CPDF_Dictionary* pDict = ToDictionary(pAnnots->GetDirectObjectAt(index));
   auto pNewAnnot = pdfium::MakeUnique<CPDF_AnnotContext>(pDict, pPage, nullptr);
-  *annot = FPDFAnnotationFromCPDFAnnotContext(pNewAnnot.release());
-  return true;
+  return pNewAnnot.release();
 }
 
 DLLEXPORT void STDCALL FPDFPage_CloseAnnot(FPDF_ANNOTATION annot) {
@@ -210,6 +199,9 @@ DLLEXPORT void STDCALL FPDFPage_CloseAnnot(FPDF_ANNOTATION annot) {
 
 DLLEXPORT FPDF_ANNOTATION_SUBTYPE STDCALL
 FPDFAnnot_GetSubtype(FPDF_ANNOTATION annot) {
+  if (!annot)
+    return FPDF_ANNOT_UNKNOWN;
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
   if (!pAnnotDict)
@@ -225,9 +217,12 @@ DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_SetColor(FPDF_ANNOTATION annot,
                                                unsigned int G,
                                                unsigned int B,
                                                unsigned int A) {
+  if (!annot || R > 255 || G > 255 || B > 255 || A > 255)
+    return false;
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
-  if (!pAnnotDict || R > 255 || G > 255 || B > 255 || A > 255)
+  if (!pAnnotDict)
     return false;
 
   // Set the opacity of the annotation.
@@ -254,9 +249,12 @@ DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_GetColor(FPDF_ANNOTATION annot,
                                                unsigned int* G,
                                                unsigned int* B,
                                                unsigned int* A) {
+  if (!annot || !R || !G || !B || !A)
+    return false;
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
-  if (!pAnnotDict || !R || !G || !B || !A)
+  if (!pAnnotDict)
     return false;
 
   CPDF_Array* pColor = pAnnotDict->GetArrayFor(
@@ -323,6 +321,9 @@ FPDFAnnot_SetAttachmentPoints(FPDF_ANNOTATION annot,
 
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
+  if (!pAnnotDict)
+    return false;
+
   CPDF_Array* pQuadPoints = pAnnotDict->GetArrayFor("QuadPoints");
   if (pQuadPoints)
     pQuadPoints->Clear();
@@ -340,31 +341,37 @@ FPDFAnnot_SetAttachmentPoints(FPDF_ANNOTATION annot,
   return true;
 }
 
-DLLEXPORT FPDF_BOOL STDCALL
-FPDFAnnot_GetAttachmentPoints(FPDF_ANNOTATION annot,
-                              FS_QUADPOINTSF* quadPoints) {
-  if (!annot || !quadPoints || !FPDFAnnot_HasAttachmentPoints(annot))
-    return false;
+DLLEXPORT FS_QUADPOINTSF STDCALL
+FPDFAnnot_GetAttachmentPoints(FPDF_ANNOTATION annot) {
+  if (!annot || !FPDFAnnot_HasAttachmentPoints(annot))
+    return FS_QUADPOINTSF();
 
-  CPDF_Array* pArray =
-      CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict()->GetArrayFor(
-          "QuadPoints");
-  if (!pArray)
-    return false;
+  CPDF_Dictionary* pAnnotDict =
+      CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
+  if (!pAnnotDict)
+    return FS_QUADPOINTSF();
 
-  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;
+  CPDF_Array* pArray = pAnnotDict->GetArrayFor("QuadPoints");
+  if (!pArray)
+    return FS_QUADPOINTSF();
+
+  FS_QUADPOINTSF quadPoints;
+  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 quadPoints;
 }
 
 DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_SetRect(FPDF_ANNOTATION annot,
                                               FS_RECTF rect) {
+  if (!annot)
+    return false;
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
   if (!pAnnotDict)
@@ -383,27 +390,33 @@ DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_SetRect(FPDF_ANNOTATION annot,
   return true;
 }
 
-DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_GetRect(FPDF_ANNOTATION annot,
-                                              FS_RECTF* rect) {
+DLLEXPORT FS_RECTF STDCALL FPDFAnnot_GetRect(FPDF_ANNOTATION annot) {
+  if (!annot)
+    return FS_RECTF();
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
-  if (!rect || !pAnnotDict)
-    return false;
+  if (!pAnnotDict)
+    return FS_RECTF();
 
   CFX_FloatRect rt = pAnnotDict->GetRectFor("Rect");
   if (rt.IsEmpty())
-    return false;
-
-  rect->left = rt.left;
-  rect->bottom = rt.bottom;
-  rect->right = rt.right;
-  rect->top = rt.top;
-  return true;
+    return FS_RECTF();
+
+  FS_RECTF rect;
+  rect.left = rt.left;
+  rect.bottom = rt.bottom;
+  rect.right = rt.right;
+  rect.top = rt.top;
+  return rect;
 }
 
 DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_SetText(FPDF_ANNOTATION annot,
                                               FPDFANNOT_TEXTTYPE type,
                                               FPDF_WIDESTRING text) {
+  if (!annot)
+    return false;
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
   if (!pAnnotDict)
@@ -420,6 +433,9 @@ DLLEXPORT unsigned long STDCALL FPDFAnnot_GetText(FPDF_ANNOTATION annot,
                                                   FPDFANNOT_TEXTTYPE type,
                                                   void* buffer,
                                                   unsigned long buflen) {
+  if (!annot)
+    return 0;
+
   CPDF_Dictionary* pAnnotDict =
       CPDFAnnotContextFromFPDFAnnotation(annot)->GetAnnotDict();
   if (!pAnnotDict)
diff --git a/fpdfsdk/fpdfannot_embeddertest.cpp b/fpdfsdk/fpdfannot_embeddertest.cpp
index b7dee88b78..4d6ae1c6d8 100644
--- a/fpdfsdk/fpdfannot_embeddertest.cpp
+++ b/fpdfsdk/fpdfannot_embeddertest.cpp
@@ -40,8 +40,8 @@ TEST_F(FPDFAnnotEmbeddertest, ExtractHighlightLongContent) {
   EXPECT_EQ(1, FPDFPage_GetAnnotCount(page));
 
   // Check that the annotation is of type "highlight".
-  FPDF_ANNOTATION annot;
-  ASSERT_TRUE(FPDFPage_GetAnnot(page, 0, &annot));
+  FPDF_ANNOTATION annot = FPDFPage_GetAnnot(page, 0);
+  ASSERT_TRUE(annot);
   EXPECT_EQ(FPDF_ANNOT_HIGHLIGHT, FPDFAnnot_GetSubtype(annot));
 
   // Check that the annotation color is yellow.
@@ -98,8 +98,7 @@ TEST_F(FPDFAnnotEmbeddertest, ExtractHighlightLongContent) {
                    .c_str());
 
   // Check that the quadpoints are correct.
-  FS_QUADPOINTSF quadpoints;
-  ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(annot, &quadpoints));
+  FS_QUADPOINTSF quadpoints = FPDFAnnot_GetAttachmentPoints(annot);
   EXPECT_EQ(115.802643f, quadpoints.x1);
   EXPECT_EQ(718.913940f, quadpoints.y1);
   EXPECT_EQ(157.211182f, quadpoints.x4);
@@ -119,8 +118,8 @@ TEST_F(FPDFAnnotEmbeddertest, ExtractInkMultiple) {
   EXPECT_EQ(3, FPDFPage_GetAnnotCount(page));
 
   // Check that the third annotation is of type "ink".
-  FPDF_ANNOTATION annot;
-  ASSERT_TRUE(FPDFPage_GetAnnot(page, 2, &annot));
+  FPDF_ANNOTATION annot = FPDFPage_GetAnnot(page, 2);
+  ASSERT_TRUE(annot);
   EXPECT_EQ(FPDF_ANNOT_INK, FPDFAnnot_GetSubtype(annot));
 
   // Check that the annotation color is blue with opacity.
@@ -141,8 +140,7 @@ TEST_F(FPDFAnnotEmbeddertest, ExtractInkMultiple) {
 
   // Check that the rectange coordinates are correct.
   // Note that upon rendering, the rectangle coordinates will be adjusted.
-  FS_RECTF rect;
-  ASSERT_TRUE(FPDFAnnot_GetRect(annot, &rect));
+  FS_RECTF rect = FPDFAnnot_GetRect(annot);
   EXPECT_EQ(351.820404f, rect.left);
   EXPECT_EQ(583.830688f, rect.bottom);
   EXPECT_EQ(475.336090f, rect.right);
@@ -159,8 +157,7 @@ TEST_F(FPDFAnnotEmbeddertest, AddIllegalSubtypeAnnotation) {
   ASSERT_TRUE(page);
 
   // Add an annotation with an illegal subtype.
-  FPDF_ANNOTATION annot;
-  ASSERT_FALSE(FPDFPage_CreateAnnot(page, -1, &annot));
+  ASSERT_FALSE(FPDFPage_CreateAnnot(page, -1));
 
   UnloadPage(page);
 }
@@ -173,8 +170,8 @@ TEST_F(FPDFAnnotEmbeddertest, AddFirstTextAnnotation) {
   EXPECT_EQ(0, FPDFPage_GetAnnotCount(page));
 
   // Add a text annotation to the page.
-  FPDF_ANNOTATION annot;
-  ASSERT_TRUE(FPDFPage_CreateAnnot(page, FPDF_ANNOT_TEXT, &annot));
+  FPDF_ANNOTATION annot = FPDFPage_CreateAnnot(page, FPDF_ANNOT_TEXT);
+  ASSERT_TRUE(annot);
 
   // Check that there is now 1 annotations on this page.
   EXPECT_EQ(1, FPDFPage_GetAnnotCount(page));
@@ -183,7 +180,8 @@ TEST_F(FPDFAnnotEmbeddertest, AddFirstTextAnnotation) {
   EXPECT_EQ(FPDF_ANNOT_TEXT, FPDFAnnot_GetSubtype(annot));
   FPDFPage_CloseAnnot(annot);
 
-  ASSERT_TRUE(FPDFPage_GetAnnot(page, 0, &annot));
+  annot = FPDFPage_GetAnnot(page, 0);
+  ASSERT_TRUE(annot);
   EXPECT_EQ(FPDF_ANNOT_TEXT, FPDFAnnot_GetSubtype(annot));
 
   // Set the color of the annotation.
@@ -213,15 +211,16 @@ TEST_F(FPDFAnnotEmbeddertest, AddFirstTextAnnotation) {
   EXPECT_EQ(51u, A);
 
   // Set the annotation rectangle.
-  FS_RECTF rect;
-  EXPECT_FALSE(FPDFAnnot_GetRect(annot, &rect));
+  FS_RECTF rect = FPDFAnnot_GetRect(annot);
+  EXPECT_EQ(0.f, rect.left);
+  EXPECT_EQ(0.f, rect.right);
   rect.left = 35;
   rect.bottom = 150;
   rect.right = 53;
   rect.top = 165;
   ASSERT_TRUE(FPDFAnnot_SetRect(annot, rect));
   // Check that the annotation rectangle has been set correctly.
-  ASSERT_TRUE(FPDFAnnot_GetRect(annot, &rect));
+  rect = FPDFAnnot_GetRect(annot);
   EXPECT_EQ(35.f, rect.left);
   EXPECT_EQ(150.f, rect.bottom);
   EXPECT_EQ(53.f, rect.right);
@@ -256,10 +255,9 @@ TEST_F(FPDFAnnotEmbeddertest, AddAndSaveUnderlineAnnotation) {
   // Check that there is a total of one annotation on its first page, and verify
   // its quadpoints.
   EXPECT_EQ(1, FPDFPage_GetAnnotCount(page));
-  FPDF_ANNOTATION annot;
-  ASSERT_TRUE(FPDFPage_GetAnnot(page, 0, &annot));
-  FS_QUADPOINTSF quadpoints;
-  ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(annot, &quadpoints));
+  FPDF_ANNOTATION annot = FPDFPage_GetAnnot(page, 0);
+  ASSERT_TRUE(annot);
+  FS_QUADPOINTSF quadpoints = FPDFAnnot_GetAttachmentPoints(annot);
   EXPECT_EQ(115.802643f, quadpoints.x1);
   EXPECT_EQ(718.913940f, quadpoints.y1);
   EXPECT_EQ(157.211182f, quadpoints.x4);
@@ -267,7 +265,8 @@ TEST_F(FPDFAnnotEmbeddertest, AddAndSaveUnderlineAnnotation) {
   FPDFPage_CloseAnnot(annot);
 
   // Add an underline annotation to the page and set its quadpoints.
-  ASSERT_TRUE(FPDFPage_CreateAnnot(page, FPDF_ANNOT_UNDERLINE, &annot));
+  annot = FPDFPage_CreateAnnot(page, FPDF_ANNOT_UNDERLINE);
+  ASSERT_TRUE(annot);
   quadpoints.x1 = 140.802643f;
   quadpoints.x3 = 140.802643f;
   ASSERT_TRUE(FPDFAnnot_SetAttachmentPoints(annot, quadpoints));
@@ -294,11 +293,10 @@ TEST_F(FPDFAnnotEmbeddertest, AddAndSaveUnderlineAnnotation) {
 
   // Check that the second annotation is an underline annotation and verify
   // its quadpoints.
-  FPDF_ANNOTATION new_annot;
-  ASSERT_TRUE(FPDFPage_GetAnnot(new_page, 1, &new_annot));
+  FPDF_ANNOTATION new_annot = FPDFPage_GetAnnot(new_page, 1);
+  ASSERT_TRUE(new_annot);
   EXPECT_EQ(FPDF_ANNOT_UNDERLINE, FPDFAnnot_GetSubtype(new_annot));
-  FS_QUADPOINTSF new_quadpoints;
-  ASSERT_TRUE(FPDFAnnot_GetAttachmentPoints(new_annot, &new_quadpoints));
+  FS_QUADPOINTSF new_quadpoints = FPDFAnnot_GetAttachmentPoints(new_annot);
   EXPECT_NEAR(quadpoints.x1, new_quadpoints.x1, 0.001f);
   EXPECT_NEAR(quadpoints.y1, new_quadpoints.y1, 0.001f);
   EXPECT_NEAR(quadpoints.x4, new_quadpoints.x4, 0.001f);
diff --git a/fpdfsdk/fpdfedit_embeddertest.cpp b/fpdfsdk/fpdfedit_embeddertest.cpp
index fcf7e3903b..ad3e0d15a8 100644
--- a/fpdfsdk/fpdfedit_embeddertest.cpp
+++ b/fpdfsdk/fpdfedit_embeddertest.cpp
@@ -900,12 +900,14 @@ TEST_F(FPDFEditEmbeddertest, TransformAnnot) {
   ASSERT_TRUE(page);
 
   // Add an underline annotation to the page without specifying its rectangle.
-  ASSERT_TRUE(FPDFPage_CreateAnnot(page, FPDF_ANNOT_UNDERLINE, nullptr));
+  FPDF_ANNOTATION annot = FPDFPage_CreateAnnot(page, FPDF_ANNOT_UNDERLINE);
+  ASSERT_TRUE(annot);
 
   // FPDFPage_TransformAnnots() should run without errors when modifying
   // annotation rectangles.
   FPDFPage_TransformAnnots(page, 1, 2, 3, 4, 5, 6);
 
+  FPDFPage_CloseAnnot(annot);
   UnloadPage(page);
 }