Use observed pointers in CPDFSDK_AnnotIterator.chromium/2979

Rename to CPDFSDK_AnnotIteration, as it is now an ordered set of
annots, and not the iterator itself.

Review-Url: https://codereview.chromium.org/2626073005

8 files changed, 99 insertions, 120 deletions

diff --git a/BUILD.gn b/BUILD.gn
index 9a5c29c82e..ebb821ccff 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -74,8 +74,8 @@ static_library("pdfium") {
     "fpdfsdk/cpdfsdk_annot.h",
     "fpdfsdk/cpdfsdk_annothandlermgr.cpp",
     "fpdfsdk/cpdfsdk_annothandlermgr.h",
-    "fpdfsdk/cpdfsdk_annotiterator.cpp",
-    "fpdfsdk/cpdfsdk_annotiterator.h",
+    "fpdfsdk/cpdfsdk_annotiteration.cpp",
+    "fpdfsdk/cpdfsdk_annotiteration.h",
     "fpdfsdk/cpdfsdk_baannot.cpp",
     "fpdfsdk/cpdfsdk_baannot.h",
     "fpdfsdk/cpdfsdk_baannothandler.cpp",
diff --git a/fpdfsdk/cpdfsdk_annotiteration.cpp b/fpdfsdk/cpdfsdk_annotiteration.cpp
new file mode 100644
index 0000000000..dd99ade509
--- /dev/null
+++ b/fpdfsdk/cpdfsdk_annotiteration.cpp
@@ -0,0 +1,40 @@
+// Copyright 2017 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
+
+#include "fpdfsdk/cpdfsdk_annotiteration.h"
+
+#include <algorithm>
+#include <utility>
+
+#include "fpdfsdk/cpdfsdk_annot.h"
+#include "fpdfsdk/cpdfsdk_pageview.h"
+
+CPDFSDK_AnnotIteration::CPDFSDK_AnnotIteration(CPDFSDK_PageView* pPageView,
+                                               bool bReverse) {
+  // Copying/sorting ObservedPtrs is expensive, so do it once at the end.
+  std::vector<CPDFSDK_Annot*> copiedList = pPageView->GetAnnotList();
+  std::stable_sort(copiedList.begin(), copiedList.end(),
+                   [](const CPDFSDK_Annot* p1, const CPDFSDK_Annot* p2) {
+                     return p1->GetLayoutOrder() < p2->GetLayoutOrder();
+                   });
+
+  CPDFSDK_Annot* pTopMostAnnot = pPageView->GetFocusAnnot();
+  if (pTopMostAnnot) {
+    auto it = std::find(copiedList.begin(), copiedList.end(), pTopMostAnnot);
+    if (it != copiedList.end()) {
+      copiedList.erase(it);
+      copiedList.insert(copiedList.begin(), pTopMostAnnot);
+    }
+  }
+  if (bReverse)
+    std::reverse(copiedList.begin(), copiedList.end());
+
+  m_List.reserve(copiedList.size());
+  for (const auto& pAnnot : copiedList)
+    m_List.emplace_back(pAnnot);
+}
+
+CPDFSDK_AnnotIteration::~CPDFSDK_AnnotIteration() {}
diff --git a/fpdfsdk/cpdfsdk_annotiteration.h b/fpdfsdk/cpdfsdk_annotiteration.h
new file mode 100644
index 0000000000..70edfd10ae
--- /dev/null
+++ b/fpdfsdk/cpdfsdk_annotiteration.h
@@ -0,0 +1,31 @@
+// Copyright 2017 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
+
+#ifndef FPDFSDK_CPDFSDK_ANNOTITERATION_H_
+#define FPDFSDK_CPDFSDK_ANNOTITERATION_H_
+
+#include <vector>
+
+#include "fpdfsdk/cpdfsdk_annot.h"
+
+class CPDFSDK_PageView;
+
+class CPDFSDK_AnnotIteration {
+ public:
+  using const_iterator =
+      std::vector<CPDFSDK_Annot::ObservedPtr>::const_iterator;
+
+  CPDFSDK_AnnotIteration(CPDFSDK_PageView* pPageView, bool bReverse);
+  ~CPDFSDK_AnnotIteration();
+
+  const_iterator begin() const { return m_List.begin(); }
+  const_iterator end() const { return m_List.end(); }
+
+ private:
+  std::vector<CPDFSDK_Annot::ObservedPtr> m_List;
+};
+
+#endif  // FPDFSDK_CPDFSDK_ANNOTITERATION_H_
diff --git a/fpdfsdk/cpdfsdk_annotiterator.cpp b/fpdfsdk/cpdfsdk_annotiterator.cpp
deleted file mode 100644
index f2c997b7f1..0000000000
--- a/fpdfsdk/cpdfsdk_annotiterator.cpp
+++ /dev/null
@@ -1,54 +0,0 @@
-// Copyright 2016 PDFium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
-
-#include "fpdfsdk/cpdfsdk_annotiterator.h"
-
-#include <algorithm>
-
-#include "fpdfsdk/cpdfsdk_annot.h"
-#include "fpdfsdk/cpdfsdk_pageview.h"
-
-CPDFSDK_AnnotIterator::CPDFSDK_AnnotIterator(CPDFSDK_PageView* pPageView,
-                                             bool bReverse)
-    : m_bReverse(bReverse), m_pos(0) {
-  const std::vector<CPDFSDK_Annot*>& annots = pPageView->GetAnnotList();
-  m_iteratorAnnotList.insert(m_iteratorAnnotList.begin(), annots.rbegin(),
-                             annots.rend());
-  std::stable_sort(m_iteratorAnnotList.begin(), m_iteratorAnnotList.end(),
-                   [](CPDFSDK_Annot* p1, CPDFSDK_Annot* p2) {
-                     return p1->GetLayoutOrder() < p2->GetLayoutOrder();
-                   });
-
-  CPDFSDK_Annot* pTopMostAnnot = pPageView->GetFocusAnnot();
-  if (!pTopMostAnnot)
-    return;
-
-  auto it = std::find(m_iteratorAnnotList.begin(), m_iteratorAnnotList.end(),
-                      pTopMostAnnot);
-  if (it != m_iteratorAnnotList.end()) {
-    CPDFSDK_Annot* pReaderAnnot = *it;
-    m_iteratorAnnotList.erase(it);
-    m_iteratorAnnotList.insert(m_iteratorAnnotList.begin(), pReaderAnnot);
-  }
-}
-
-CPDFSDK_AnnotIterator::~CPDFSDK_AnnotIterator() {}
-
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::NextAnnot() {
-  if (m_pos < m_iteratorAnnotList.size())
-    return m_iteratorAnnotList[m_pos++];
-  return nullptr;
-}
-
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::PrevAnnot() {
-  if (m_pos < m_iteratorAnnotList.size())
-    return m_iteratorAnnotList[m_iteratorAnnotList.size() - ++m_pos];
-  return nullptr;
-}
-
-CPDFSDK_Annot* CPDFSDK_AnnotIterator::Next() {
-  return m_bReverse ? PrevAnnot() : NextAnnot();
-}
diff --git a/fpdfsdk/cpdfsdk_annotiterator.h b/fpdfsdk/cpdfsdk_annotiterator.h
deleted file mode 100644
index 9918630b11..0000000000
--- a/fpdfsdk/cpdfsdk_annotiterator.h
+++ /dev/null
@@ -1,31 +0,0 @@
-// Copyright 2016 PDFium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
-
-#ifndef FPDFSDK_CPDFSDK_ANNOTITERATOR_H_
-#define FPDFSDK_CPDFSDK_ANNOTITERATOR_H_
-
-#include <vector>
-
-class CPDFSDK_Annot;
-class CPDFSDK_PageView;
-
-class CPDFSDK_AnnotIterator {
- public:
-  CPDFSDK_AnnotIterator(CPDFSDK_PageView* pPageView, bool bReverse);
-  ~CPDFSDK_AnnotIterator();
-
-  CPDFSDK_Annot* Next();
-
- private:
-  CPDFSDK_Annot* NextAnnot();
-  CPDFSDK_Annot* PrevAnnot();
-
-  std::vector<CPDFSDK_Annot*> m_iteratorAnnotList;
-  const bool m_bReverse;
-  std::size_t m_pos;
-};
-
-#endif  // FPDFSDK_CPDFSDK_ANNOTITERATOR_H_
diff --git a/fpdfsdk/cpdfsdk_baannot.h b/fpdfsdk/cpdfsdk_baannot.h
index 10f27e6dec..2da5723eb4 100644
--- a/fpdfsdk/cpdfsdk_baannot.h
+++ b/fpdfsdk/cpdfsdk_baannot.h
@@ -100,7 +100,7 @@ class CPDFSDK_BAAnnot : public CPDFSDK_Annot {
   void SetOpenState(bool bState);
 
  protected:
-  CPDF_Annot* m_pAnnot;
+  CPDF_Annot* const m_pAnnot;
 };
 
 #endif  // FPDFSDK_CPDFSDK_BAANNOT_H_
diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp
index fedc95a6ad..416adffbe6 100644
--- a/fpdfsdk/cpdfsdk_pageview.cpp
+++ b/fpdfsdk/cpdfsdk_pageview.cpp
@@ -15,7 +15,7 @@
 #include "core/fpdfdoc/cpdf_interform.h"
 #include "fpdfsdk/cpdfsdk_annot.h"
 #include "fpdfsdk/cpdfsdk_annothandlermgr.h"
-#include "fpdfsdk/cpdfsdk_annotiterator.h"
+#include "fpdfsdk/cpdfsdk_annotiteration.h"
 #include "fpdfsdk/cpdfsdk_formfillenvironment.h"
 #include "fpdfsdk/cpdfsdk_interform.h"
 #include "third_party/base/ptr_util.h"
@@ -124,48 +124,44 @@ void CPDFSDK_PageView::PageView_OnDraw(CFX_RenderDevice* pDevice,
 #endif  // PDF_ENABLE_XFA
 
   // for pdf/static xfa.
-  CPDFSDK_AnnotIterator annotIterator(this, true);
-  while (CPDFSDK_Annot* pSDKAnnot = annotIterator.Next()) {
-    CPDFSDK_AnnotHandlerMgr* pAnnotHandlerMgr =
-        m_pFormFillEnv->GetAnnotHandlerMgr();
-    pAnnotHandlerMgr->Annot_OnDraw(this, pSDKAnnot, pDevice, pUser2Device,
-                                   pOptions->m_bDrawAnnots);
+  CPDFSDK_AnnotIteration annotIteration(this, true);
+  for (const auto& pSDKAnnot : annotIteration) {
+    m_pFormFillEnv->GetAnnotHandlerMgr()->Annot_OnDraw(
+        this, pSDKAnnot.Get(), pDevice, pUser2Device, pOptions->m_bDrawAnnots);
   }
 }
 
 CPDFSDK_Annot* CPDFSDK_PageView::GetFXAnnotAtPoint(FX_FLOAT pageX,
                                                    FX_FLOAT pageY) {
   CPDFSDK_AnnotHandlerMgr* pAnnotMgr = m_pFormFillEnv->GetAnnotHandlerMgr();
-  CPDFSDK_AnnotIterator annotIterator(this, false);
-  while (CPDFSDK_Annot* pSDKAnnot = annotIterator.Next()) {
-    CFX_FloatRect rc = pAnnotMgr->Annot_OnGetViewBBox(this, pSDKAnnot);
+  CPDFSDK_AnnotIteration annotIteration(this, false);
+  for (const auto& pSDKAnnot : annotIteration) {
+    CFX_FloatRect rc = pAnnotMgr->Annot_OnGetViewBBox(this, pSDKAnnot.Get());
     if (pSDKAnnot->GetAnnotSubtype() == CPDF_Annot::Subtype::POPUP)
       continue;
     if (rc.Contains(pageX, pageY))
-      return pSDKAnnot;
+      return pSDKAnnot.Get();
   }
-
   return nullptr;
 }
 
 CPDFSDK_Annot* CPDFSDK_PageView::GetFXWidgetAtPoint(FX_FLOAT pageX,
                                                     FX_FLOAT pageY) {
   CPDFSDK_AnnotHandlerMgr* pAnnotMgr = m_pFormFillEnv->GetAnnotHandlerMgr();
-  CPDFSDK_AnnotIterator annotIterator(this, false);
-  while (CPDFSDK_Annot* pSDKAnnot = annotIterator.Next()) {
+  CPDFSDK_AnnotIteration annotIteration(this, false);
+  for (const auto& pSDKAnnot : annotIteration) {
     bool bHitTest = pSDKAnnot->GetAnnotSubtype() == CPDF_Annot::Subtype::WIDGET;
 #ifdef PDF_ENABLE_XFA
     bHitTest = bHitTest ||
                pSDKAnnot->GetAnnotSubtype() == CPDF_Annot::Subtype::XFAWIDGET;
 #endif  // PDF_ENABLE_XFA
     if (bHitTest) {
-      pAnnotMgr->Annot_OnGetViewBBox(this, pSDKAnnot);
+      pAnnotMgr->Annot_OnGetViewBBox(this, pSDKAnnot.Get());
       CFX_FloatPoint point(pageX, pageY);
-      if (pAnnotMgr->Annot_OnHitTest(this, pSDKAnnot, point))
-        return pSDKAnnot;
+      if (pAnnotMgr->Annot_OnHitTest(this, pSDKAnnot.Get(), point))
+        return pSDKAnnot.Get();
     }
   }
-
   return nullptr;
 }
 
diff --git a/fpdfsdk/javascript/Document.cpp b/fpdfsdk/javascript/Document.cpp
index 7e4dc260f2..79d68ec3a7 100644
--- a/fpdfsdk/javascript/Document.cpp
+++ b/fpdfsdk/javascript/Document.cpp
@@ -17,7 +17,7 @@
 #include "core/fpdfapi/parser/fpdf_parser_decode.h"
 #include "core/fpdfdoc/cpdf_interform.h"
 #include "core/fpdfdoc/cpdf_nametree.h"
-#include "fpdfsdk/cpdfsdk_annotiterator.h"
+#include "fpdfsdk/cpdfsdk_annotiteration.h"
 #include "fpdfsdk/cpdfsdk_formfillenvironment.h"
 #include "fpdfsdk/cpdfsdk_interform.h"
 #include "fpdfsdk/cpdfsdk_pageview.h"
@@ -1112,16 +1112,16 @@ bool Document::getAnnot(IJS_Context* cc,
   if (!pPageView)
     return false;
 
-  CPDFSDK_AnnotIterator annotIterator(pPageView, false);
+  CPDFSDK_AnnotIteration annotIteration(pPageView, false);
   CPDFSDK_BAAnnot* pSDKBAAnnot = nullptr;
-  while (CPDFSDK_Annot* pSDKAnnotCur = annotIterator.Next()) {
-    CPDFSDK_BAAnnot* pBAAnnot = static_cast<CPDFSDK_BAAnnot*>(pSDKAnnotCur);
+  for (const auto& pSDKAnnotCur : annotIteration) {
+    CPDFSDK_BAAnnot* pBAAnnot =
+        static_cast<CPDFSDK_BAAnnot*>(pSDKAnnotCur.Get());
     if (pBAAnnot && pBAAnnot->GetAnnotName() == swAnnotName) {
       pSDKBAAnnot = pBAAnnot;
       break;
     }
   }
-
   if (!pSDKBAAnnot)
     return false;
 
@@ -1140,7 +1140,6 @@ bool Document::getAnnot(IJS_Context* cc,
     return false;
 
   pAnnot->SetSDKAnnot(pSDKBAAnnot);
-
   vRet = CJS_Value(pRuntime, pJS_Annot);
   return true;
 }
@@ -1167,13 +1166,12 @@ bool Document::getAnnots(IJS_Context* cc,
     if (!pPageView)
       return false;
 
-    CPDFSDK_AnnotIterator annotIterator(pPageView, false);
-    while (CPDFSDK_Annot* pSDKAnnotCur = annotIterator.Next()) {
-      CPDFSDK_BAAnnot* pSDKBAAnnot =
-          static_cast<CPDFSDK_BAAnnot*>(pSDKAnnotCur);
-      if (!pSDKBAAnnot)
+    CPDFSDK_AnnotIteration annotIteration(pPageView, false);
+    for (const auto& pSDKAnnotCur : annotIteration) {
+      if (!pSDKAnnotCur) {
+        sError = JSGetStringFromID(IDS_STRING_JSBADOBJECT);
         return false;
-
+      }
       v8::Local<v8::Object> pObj =
           pRuntime->NewFxDynamicObj(CJS_Annot::g_nObjDefnID);
       if (pObj.IsEmpty())
@@ -1188,11 +1186,10 @@ bool Document::getAnnots(IJS_Context* cc,
       if (!pAnnot)
         return false;
 
-      pAnnot->SetSDKAnnot(pSDKBAAnnot);
+      pAnnot->SetSDKAnnot(static_cast<CPDFSDK_BAAnnot*>(pSDKAnnotCur.Get()));
       annots.SetElement(pRuntime, i, CJS_Value(pRuntime, pJS_Annot));
     }
   }
-
   vRet = CJS_Value(pRuntime, annots);
   return true;
 }