From 1c62054a42cf0759148501a36c541de5d5769d32 Mon Sep 17 00:00:00 2001 From: tsepez Date: Mon, 12 Sep 2016 09:47:52 -0700 Subject: 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 --- BUILD.gn | 2 + core/fxcrt/cfx_observable_unittest.cpp | 114 +++++++++++++++++++++++++++++++++ core/fxcrt/include/cfx_observable.h | 74 ++++++++++++--------- fpdfsdk/javascript/Annot.cpp | 9 ++- fpdfsdk/javascript/Annot.h | 4 +- fpdfsdk/javascript/Field.cpp | 10 +-- fpdfsdk/javascript/app.cpp | 7 +- fpdfsdk/javascript/cjs_runtime.cpp | 2 +- third_party/base/stl_util.h | 1 + 9 files changed, 174 insertions(+), 49 deletions(-) create mode 100644 core/fxcrt/cfx_observable_unittest.cpp 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 + +#include "testing/fx_string_testhelpers.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +class PseudoObservable : public CFX_Observable { + 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 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 m_Observers; + std::set 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(pAnnot)->OnFormat(bFormatted); - if (pAnnot) { - static_cast(pAnnot)->ResetAppearance( - bFormatted ? &sValue : nullptr, FALSE); + static_cast(pObserved.Get())->OnFormat(bFormatted); + if (pObserved) { + static_cast(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 +#include #include #include "third_party/base/numerics/safe_conversions.h" -- cgit v1.2.3