Make CFX_Obeservable::Observer into a pointer-ish type;

This may be a better design because it avoids having a level
of indirection that the Observer required.

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

9 files changed, 174 insertions, 49 deletions

diff --git a/BUILD.gn b/BUILD.gn
index 4e430ab86e..0de7241470 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -703,6 +703,7 @@ static_library("fxcrt") {
     "core/fxcrt/fxcrt_windows.cpp",
     "core/fxcrt/fxcrt_windows.h",
     "core/fxcrt/include/cfx_count_ref.h",
+    "core/fxcrt/include/cfx_observable.h",
     "core/fxcrt/include/cfx_retain_ptr.h",
     "core/fxcrt/include/fx_basic.h",
     "core/fxcrt/include/fx_coordinates.h",
@@ -1645,6 +1646,7 @@ test("pdfium_unittests") {
     "core/fxcodec/codec/fx_codec_jpx_unittest.cpp",
     "core/fxcodec/jbig2/JBig2_Image_unittest.cpp",
     "core/fxcrt/cfx_count_ref_unittest.cpp",
+    "core/fxcrt/cfx_observable_unittest.cpp",
     "core/fxcrt/cfx_retain_ptr_unittest.cpp",
     "core/fxcrt/fx_basic_bstring_unittest.cpp",
     "core/fxcrt/fx_basic_gcc_unittest.cpp",
diff --git a/core/fxcrt/cfx_observable_unittest.cpp b/core/fxcrt/cfx_observable_unittest.cpp
new file mode 100644
index 0000000000..eaadd95bb6
--- /dev/null
+++ b/core/fxcrt/cfx_observable_unittest.cpp
@@ -0,0 +1,114 @@
+// 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.
+
+#include "core/fxcrt/include/cfx_observable.h"
+
+#include <utility>
+
+#include "testing/fx_string_testhelpers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class PseudoObservable : public CFX_Observable<PseudoObservable> {
+ public:
+  PseudoObservable() {}
+  int SomeMethod() { return 42; }
+  size_t ActiveObservedPtrs() const { return ActiveObservedPtrsForTesting(); }
+};
+
+}  // namespace
+
+TEST(fxcrt, ObservePtrNull) {
+  PseudoObservable::ObservedPtr ptr;
+  EXPECT_EQ(nullptr, ptr.Get());
+}
+
+TEST(fxcrt, ObservePtrLivesLonger) {
+  PseudoObservable* pObs = new PseudoObservable;
+  PseudoObservable::ObservedPtr ptr(pObs);
+  EXPECT_NE(nullptr, ptr.Get());
+  EXPECT_EQ(1u, pObs->ActiveObservedPtrs());
+  delete pObs;
+  EXPECT_EQ(nullptr, ptr.Get());
+}
+
+TEST(fxcrt, ObservePtrLivesShorter) {
+  PseudoObservable obs;
+  {
+    PseudoObservable::ObservedPtr ptr(&obs);
+    EXPECT_NE(nullptr, ptr.Get());
+    EXPECT_EQ(1u, obs.ActiveObservedPtrs());
+  }
+  EXPECT_EQ(0u, obs.ActiveObservedPtrs());
+}
+
+TEST(fxcrt, ObservePtrResetNull) {
+  PseudoObservable obs;
+  PseudoObservable::ObservedPtr ptr(&obs);
+  EXPECT_EQ(1u, obs.ActiveObservedPtrs());
+  ptr.Reset();
+  EXPECT_EQ(0u, obs.ActiveObservedPtrs());
+}
+
+TEST(fxcrt, ObservePtrReset) {
+  PseudoObservable obs1;
+  PseudoObservable obs2;
+  PseudoObservable::ObservedPtr ptr(&obs1);
+  EXPECT_EQ(1u, obs1.ActiveObservedPtrs());
+  EXPECT_EQ(0u, obs2.ActiveObservedPtrs());
+  ptr.Reset(&obs2);
+  EXPECT_EQ(0u, obs1.ActiveObservedPtrs());
+  EXPECT_EQ(1u, obs2.ActiveObservedPtrs());
+}
+
+TEST(fxcrt, ObservePtrEquals) {
+  PseudoObservable obj1;
+  PseudoObservable obj2;
+  PseudoObservable::ObservedPtr null_ptr1;
+  PseudoObservable::ObservedPtr obj1_ptr1(&obj1);
+  PseudoObservable::ObservedPtr obj2_ptr1(&obj2);
+  {
+    PseudoObservable::ObservedPtr null_ptr2;
+    EXPECT_TRUE(null_ptr1 == null_ptr2);
+
+    PseudoObservable::ObservedPtr obj1_ptr2(&obj1);
+    EXPECT_TRUE(obj1_ptr1 == obj1_ptr2);
+
+    PseudoObservable::ObservedPtr obj2_ptr2(&obj2);
+    EXPECT_TRUE(obj2_ptr1 == obj2_ptr2);
+  }
+  EXPECT_FALSE(null_ptr1 == obj1_ptr1);
+  EXPECT_FALSE(null_ptr1 == obj2_ptr1);
+  EXPECT_FALSE(obj1_ptr1 == obj2_ptr1);
+}
+
+TEST(fxcrt, ObservePtrNotEquals) {
+  PseudoObservable obj1;
+  PseudoObservable obj2;
+  PseudoObservable::ObservedPtr null_ptr1;
+  PseudoObservable::ObservedPtr obj1_ptr1(&obj1);
+  PseudoObservable::ObservedPtr obj2_ptr1(&obj2);
+  {
+    PseudoObservable::ObservedPtr null_ptr2;
+    PseudoObservable::ObservedPtr obj1_ptr2(&obj1);
+    PseudoObservable::ObservedPtr obj2_ptr2(&obj2);
+    EXPECT_FALSE(null_ptr1 != null_ptr2);
+    EXPECT_FALSE(obj1_ptr1 != obj1_ptr2);
+    EXPECT_FALSE(obj2_ptr1 != obj2_ptr2);
+  }
+  EXPECT_TRUE(null_ptr1 != obj1_ptr1);
+  EXPECT_TRUE(null_ptr1 != obj2_ptr1);
+  EXPECT_TRUE(obj1_ptr1 != obj2_ptr1);
+}
+
+TEST(fxcrt, ObservePtrBool) {
+  PseudoObservable obj1;
+  PseudoObservable::ObservedPtr null_ptr;
+  PseudoObservable::ObservedPtr obj1_ptr(&obj1);
+  bool null_bool = !!null_ptr;
+  bool obj1_bool = !!obj1_ptr;
+  EXPECT_FALSE(null_bool);
+  EXPECT_TRUE(obj1_bool);
+}
diff --git a/core/fxcrt/include/cfx_observable.h b/core/fxcrt/include/cfx_observable.h
index 99a9951284..a5053f3952 100644
--- a/core/fxcrt/include/cfx_observable.h
+++ b/core/fxcrt/include/cfx_observable.h
@@ -13,56 +13,66 @@
 template <class T>
 class CFX_Observable {
  public:
-  class Observer {
+  class ObservedPtr {
    public:
-    Observer() : m_pWatchedPtr(nullptr) {}
-    Observer(T** pWatchedPtr) : m_pWatchedPtr(pWatchedPtr) {
-      if (m_pWatchedPtr)
-        (*m_pWatchedPtr)->AddObserver(this);
+    ObservedPtr() : m_pObservedPtr(nullptr) {}
+    explicit ObservedPtr(T* pObservedPtr) : m_pObservedPtr(pObservedPtr) {
+      if (m_pObservedPtr)
+        m_pObservedPtr->AddObservedPtr(this);
     }
-    Observer(const Observer& that) = delete;
-    ~Observer() {
-      if (m_pWatchedPtr)
-        (*m_pWatchedPtr)->RemoveObserver(this);
+    ObservedPtr(const ObservedPtr& that) = delete;
+    ~ObservedPtr() {
+      if (m_pObservedPtr)
+        m_pObservedPtr->RemoveObservedPtr(this);
     }
-    void SetWatchedPtr(T** pWatchedPtr) {
-      if (m_pWatchedPtr)
-        (*m_pWatchedPtr)->RemoveObserver(this);
-      m_pWatchedPtr = pWatchedPtr;
-      if (m_pWatchedPtr)
-        (*m_pWatchedPtr)->AddObserver(this);
+    void Reset(T* pObservedPtr = nullptr) {
+      if (m_pObservedPtr)
+        m_pObservedPtr->RemoveObservedPtr(this);
+      m_pObservedPtr = pObservedPtr;
+      if (m_pObservedPtr)
+        m_pObservedPtr->AddObservedPtr(this);
     }
     void OnDestroy() {
-      ASSERT(m_pWatchedPtr);
-      *m_pWatchedPtr = nullptr;
-      m_pWatchedPtr = nullptr;
+      ASSERT(m_pObservedPtr);
+      m_pObservedPtr = nullptr;
     }
-    Observer& operator=(const Observer& that) = delete;
+    ObservedPtr& operator=(const ObservedPtr& that) = delete;
+    bool operator==(const ObservedPtr& that) const {
+      return m_pObservedPtr == that.m_pObservedPtr;
+    }
+    bool operator!=(const ObservedPtr& that) const { return !(*this == that); }
+    explicit operator bool() const { return !!m_pObservedPtr; }
+    T* Get() const { return m_pObservedPtr; }
+    T& operator*() const { return *m_pObservedPtr; }
+    T* operator->() const { return m_pObservedPtr; }
 
    private:
-    T** m_pWatchedPtr;
+    T* m_pObservedPtr;
   };
 
   CFX_Observable() {}
   CFX_Observable(const CFX_Observable& that) = delete;
-  ~CFX_Observable() { NotifyObservers(); }
-  void AddObserver(Observer* pObserver) {
-    ASSERT(!pdfium::ContainsKey(m_Observers, pObserver));
-    m_Observers.insert(pObserver);
+  ~CFX_Observable() { NotifyObservedPtrs(); }
+  void AddObservedPtr(ObservedPtr* pObservedPtr) {
+    ASSERT(!pdfium::ContainsKey(m_ObservedPtrs, pObservedPtr));
+    m_ObservedPtrs.insert(pObservedPtr);
   }
-  void RemoveObserver(Observer* pObserver) {
-    ASSERT(pdfium::ContainsKey(m_Observers, pObserver));
-    m_Observers.erase(pObserver);
+  void RemoveObservedPtr(ObservedPtr* pObservedPtr) {
+    ASSERT(pdfium::ContainsKey(m_ObservedPtrs, pObservedPtr));
+    m_ObservedPtrs.erase(pObservedPtr);
   }
-  void NotifyObservers() {
-    for (auto* pObserver : m_Observers)
-      pObserver->OnDestroy();
-    m_Observers.clear();
+  void NotifyObservedPtrs() {
+    for (auto* pObservedPtr : m_ObservedPtrs)
+      pObservedPtr->OnDestroy();
+    m_ObservedPtrs.clear();
   }
   CFX_Observable& operator=(const CFX_Observable& that) = delete;
 
+ protected:
+  size_t ActiveObservedPtrsForTesting() const { return m_ObservedPtrs.size(); }
+
  private:
-  std::set<Observer*> m_Observers;
+  std::set<ObservedPtr*> m_ObservedPtrs;
 };
 
 #endif  // CORE_FXCRT_INCLUDE_CFX_OBSERVABLE_H_
diff --git a/fpdfsdk/javascript/Annot.cpp b/fpdfsdk/javascript/Annot.cpp
index ec77000449..8e6a775dcd 100644
--- a/fpdfsdk/javascript/Annot.cpp
+++ b/fpdfsdk/javascript/Annot.cpp
@@ -40,7 +40,7 @@ Annot::~Annot() {}
 FX_BOOL Annot::hidden(IJS_Context* cc,
                       CJS_PropValue& vp,
                       CFX_WideString& sError) {
-  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot);
+  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get());
   if (!baAnnot)
     return FALSE;
 
@@ -72,7 +72,7 @@ FX_BOOL Annot::hidden(IJS_Context* cc,
 FX_BOOL Annot::name(IJS_Context* cc,
                     CJS_PropValue& vp,
                     CFX_WideString& sError) {
-  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot);
+  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get());
   if (!baAnnot)
     return FALSE;
 
@@ -95,7 +95,7 @@ FX_BOOL Annot::type(IJS_Context* cc,
     return FALSE;
   }
 
-  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot);
+  CPDFSDK_BAAnnot* baAnnot = ToBAAnnot(m_pAnnot.Get());
   if (!baAnnot)
     return FALSE;
 
@@ -104,6 +104,5 @@ FX_BOOL Annot::type(IJS_Context* cc,
 }
 
 void Annot::SetSDKAnnot(CPDFSDK_BAAnnot* annot) {
-  m_pAnnot = annot;
-  SetWatchedPtr(&m_pAnnot);
+  m_pAnnot.Reset(annot);
 }
diff --git a/fpdfsdk/javascript/Annot.h b/fpdfsdk/javascript/Annot.h
index c8b0afb556..21ad6d017a 100644
--- a/fpdfsdk/javascript/Annot.h
+++ b/fpdfsdk/javascript/Annot.h
@@ -12,7 +12,7 @@
 #include "fpdfsdk/include/cpdfsdk_baannot.h"
 #include "fpdfsdk/javascript/JS_Define.h"
 
-class Annot : public CJS_EmbedObj, public CPDFSDK_Annot::Observer {
+class Annot : public CJS_EmbedObj {
  public:
   explicit Annot(CJS_Object* pJSObject);
   ~Annot() override;
@@ -24,7 +24,7 @@ class Annot : public CJS_EmbedObj, public CPDFSDK_Annot::Observer {
   void SetSDKAnnot(CPDFSDK_BAAnnot* annot);
 
  private:
-  CPDFSDK_Annot* m_pAnnot = nullptr;
+  CPDFSDK_Annot::ObservedPtr m_pAnnot;
 };
 
 class CJS_Annot : public CJS_Object {
diff --git a/fpdfsdk/javascript/Field.cpp b/fpdfsdk/javascript/Field.cpp
index 9ecdec6936..85a43d6cb6 100644
--- a/fpdfsdk/javascript/Field.cpp
+++ b/fpdfsdk/javascript/Field.cpp
@@ -270,12 +270,12 @@ void Field::UpdateFormField(CPDFSDK_Document* pDocument,
     if (nFieldType == FIELDTYPE_COMBOBOX || nFieldType == FIELDTYPE_TEXTFIELD) {
       for (CPDFSDK_Annot* pAnnot : widgets) {
         FX_BOOL bFormatted = FALSE;
-        CPDFSDK_Annot::Observer observer(&pAnnot);
+        CPDFSDK_Annot::ObservedPtr pObserved(pAnnot);
         CFX_WideString sValue =
-            static_cast<CPDFSDK_Widget*>(pAnnot)->OnFormat(bFormatted);
-        if (pAnnot) {
-          static_cast<CPDFSDK_Widget*>(pAnnot)->ResetAppearance(
-              bFormatted ? &sValue : nullptr, FALSE);
+            static_cast<CPDFSDK_Widget*>(pObserved.Get())->OnFormat(bFormatted);
+        if (pObserved) {
+          static_cast<CPDFSDK_Widget*>(pObserved.Get())
+              ->ResetAppearance(bFormatted ? &sValue : nullptr, FALSE);
         }
       }
     } else {
diff --git a/fpdfsdk/javascript/app.cpp b/fpdfsdk/javascript/app.cpp
index d7086d211e..1e3ea2f04a 100644
--- a/fpdfsdk/javascript/app.cpp
+++ b/fpdfsdk/javascript/app.cpp
@@ -21,7 +21,7 @@
 #include "fpdfsdk/javascript/resource.h"
 #include "third_party/base/stl_util.h"
 
-class GlobalTimer : public CJS_Runtime::Observer {
+class GlobalTimer {
  public:
   GlobalTimer(app* pObj,
               CPDFDoc_Environment* pApp,
@@ -38,7 +38,7 @@ class GlobalTimer : public CJS_Runtime::Observer {
   bool IsOneShot() const { return m_nType == 1; }
   uint32_t GetTimeOut() const { return m_dwTimeOut; }
   int GetTimerID() const { return m_nTimerID; }
-  CJS_Runtime* GetRuntime() const { return m_pRuntime; }
+  CJS_Runtime* GetRuntime() const { return m_pRuntime.Get(); }
   CFX_WideString GetJScript() const { return m_swJScript; }
 
  private:
@@ -53,7 +53,7 @@ class GlobalTimer : public CJS_Runtime::Observer {
   const int m_nType;  // 0:Interval; 1:TimeOut
   const uint32_t m_dwTimeOut;
   const CFX_WideString m_swJScript;
-  CJS_Runtime* m_pRuntime;
+  CJS_Runtime::ObservedPtr m_pRuntime;
   CPDFDoc_Environment* const m_pApp;
 };
 
@@ -75,7 +75,6 @@ GlobalTimer::GlobalTimer(app* pObj,
   CFX_SystemHandler* pHandler = m_pApp->GetSysHandler();
   m_nTimerID = pHandler->SetTimer(dwElapse, Trigger);
   (*GetGlobalTimerMap())[m_nTimerID] = this;
-  SetWatchedPtr(&m_pRuntime);
 }
 
 GlobalTimer::~GlobalTimer() {
diff --git a/fpdfsdk/javascript/cjs_runtime.cpp b/fpdfsdk/javascript/cjs_runtime.cpp
index 67d45d5e4a..bfed82efe1 100644
--- a/fpdfsdk/javascript/cjs_runtime.cpp
+++ b/fpdfsdk/javascript/cjs_runtime.cpp
@@ -124,7 +124,7 @@ CJS_Runtime::CJS_Runtime(CPDFDoc_Environment* pApp)
 }
 
 CJS_Runtime::~CJS_Runtime() {
-  NotifyObservers();
+  NotifyObservedPtrs();
   ReleaseEngine();
   if (m_isolateManaged) {
     GetIsolate()->Dispose();
diff --git a/third_party/base/stl_util.h b/third_party/base/stl_util.h
index ccf3c09073..0bf442c316 100644
--- a/third_party/base/stl_util.h
+++ b/third_party/base/stl_util.h
@@ -6,6 +6,7 @@
 #define PDFIUM_THIRD_PARTY_BASE_STL_UTIL_H_
 
 #include <algorithm>
+#include <memory>
 #include <set>
 
 #include "third_party/base/numerics/safe_conversions.h"