Rework CFX_CountRef in terms of CFX_RetainPtr.

Make use of existing ref count work rather than re-inventing it.

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

8 files changed, 211 insertions, 71 deletions

diff --git a/BUILD.gn b/BUILD.gn
index f603099d0f..c8da0dc35e 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -688,6 +688,7 @@ static_library("fxcrt") {
     "core/fxcrt/fxcrt_stream.cpp",
     "core/fxcrt/fxcrt_windows.cpp",
     "core/fxcrt/fxcrt_windows.h",
+    "core/fxcrt/include/cfx_count_ref.h",
     "core/fxcrt/include/cfx_retain_ptr.h",
     "core/fxcrt/include/fx_basic.h",
     "core/fxcrt/include/fx_coordinates.h",
@@ -1601,6 +1602,7 @@ test("pdfium_unittests") {
     "core/fpdftext/fpdf_text_int_unittest.cpp",
     "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_retain_ptr_unittest.cpp",
     "core/fxcrt/fx_basic_bstring_unittest.cpp",
     "core/fxcrt/fx_basic_gcc_unittest.cpp",
diff --git a/core/fpdfapi/fpdf_page/cpdf_colorstate.h b/core/fpdfapi/fpdf_page/cpdf_colorstate.h
index da4c49d3c9..c7b8854be4 100644
--- a/core/fpdfapi/fpdf_page/cpdf_colorstate.h
+++ b/core/fpdfapi/fpdf_page/cpdf_colorstate.h
@@ -8,6 +8,7 @@
 #define CORE_FPDFAPI_FPDF_PAGE_CPDF_COLORSTATE_H_
 
 #include "core/fpdfapi/fpdf_page/cpdf_colorstatedata.h"
+#include "core/fxcrt/include/cfx_count_ref.h"
 #include "core/fxcrt/include/fx_basic.h"
 #include "core/fxcrt/include/fx_system.h"
 
diff --git a/core/fpdfapi/fpdf_page/cpdf_contentmark.h b/core/fpdfapi/fpdf_page/cpdf_contentmark.h
index 056edc23a8..680aed4b99 100644
--- a/core/fpdfapi/fpdf_page/cpdf_contentmark.h
+++ b/core/fpdfapi/fpdf_page/cpdf_contentmark.h
@@ -8,6 +8,7 @@
 #define CORE_FPDFAPI_FPDF_PAGE_CPDF_CONTENTMARK_H_
 
 #include "core/fpdfapi/fpdf_page/cpdf_contentmarkdata.h"
+#include "core/fxcrt/include/cfx_count_ref.h"
 #include "core/fxcrt/include/fx_basic.h"
 
 class CPDF_ContentMark : public CFX_CountRef<CPDF_ContentMarkData> {
diff --git a/core/fpdfapi/fpdf_page/include/cpdf_path.h b/core/fpdfapi/fpdf_page/include/cpdf_path.h
index 0c077bb443..0e8c491f9e 100644
--- a/core/fpdfapi/fpdf_page/include/cpdf_path.h
+++ b/core/fpdfapi/fpdf_page/include/cpdf_path.h
@@ -27,8 +27,8 @@ class CPDF_Path : public CFX_CountRef<CFX_PathData> {
 
   FX_BOOL IsRect() const { return m_pObject->IsRect(); }
   void Transform(const CFX_Matrix* pMatrix) { GetModify()->Transform(pMatrix); }
-  void Append(CPDF_Path src, const CFX_Matrix* pMatrix) {
-    m_pObject->Append(src.m_pObject, pMatrix);
+  void Append(const CPDF_Path& other, const CFX_Matrix* pMatrix) {
+    m_pObject->Append(other.GetObject(), pMatrix);
   }
 
   void AppendRect(FX_FLOAT left,
diff --git a/core/fxcrt/cfx_count_ref_unittest.cpp b/core/fxcrt/cfx_count_ref_unittest.cpp
new file mode 100644
index 0000000000..11c7c9b86d
--- /dev/null
+++ b/core/fxcrt/cfx_count_ref_unittest.cpp
@@ -0,0 +1,129 @@
+// 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_count_ref.h"
+
+#include <map>
+#include <string>
+
+#include "testing/fx_string_testhelpers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class Observer {
+ public:
+  void OnConstruct(const std::string& name) { construction_counts_[name]++; }
+  void OnDestruct(const std::string& name) { destruction_counts_[name]++; }
+  int GetConstructionCount(const std::string& name) {
+    return construction_counts_[name];
+  }
+  int GetDestructionCount(const std::string& name) {
+    return destruction_counts_[name];
+  }
+
+ private:
+  std::map<std::string, int> construction_counts_;
+  std::map<std::string, int> destruction_counts_;
+};
+
+class Object {
+ public:
+  Object(Observer* observer, const std::string& name)
+      : name_(name), observer_(observer) {
+    observer->OnConstruct(name_);
+  }
+  Object(const Object& that) : name_(that.name_), observer_(that.observer_) {
+    observer_->OnConstruct(name_);
+  }
+  ~Object() { observer_->OnDestruct(name_); }
+
+ private:
+  std::string name_;
+  Observer* observer_;
+};
+
+}  // namespace
+
+TEST(fxcrt, CountRefNull) {
+  Observer observer;
+  {
+    CFX_CountRef<Object> ptr;
+    EXPECT_EQ(nullptr, ptr.GetObject());
+  }
+}
+
+TEST(fxcrt, CountRefCopy) {
+  Observer observer;
+  {
+    CFX_CountRef<Object> ptr1;
+    ptr1.New(&observer, std::string("one"));
+    {
+      CFX_CountRef<Object> ptr2 = ptr1;
+      EXPECT_EQ(1, observer.GetConstructionCount("one"));
+      EXPECT_EQ(0, observer.GetDestructionCount("one"));
+    }
+    {
+      CFX_CountRef<Object> ptr3(ptr1);
+      EXPECT_EQ(1, observer.GetConstructionCount("one"));
+      EXPECT_EQ(0, observer.GetDestructionCount("one"));
+    }
+    EXPECT_EQ(1, observer.GetConstructionCount("one"));
+    EXPECT_EQ(0, observer.GetDestructionCount("one"));
+  }
+  EXPECT_EQ(1, observer.GetDestructionCount("one"));
+}
+
+TEST(fxcrt, CountRefAssignOverOld) {
+  Observer observer;
+  {
+    CFX_CountRef<Object> ptr1;
+    ptr1.New(&observer, std::string("one"));
+    ptr1.New(&observer, std::string("two"));
+    EXPECT_EQ(1, observer.GetConstructionCount("one"));
+    EXPECT_EQ(1, observer.GetConstructionCount("two"));
+    EXPECT_EQ(1, observer.GetDestructionCount("one"));
+    EXPECT_EQ(0, observer.GetDestructionCount("two"));
+  }
+  EXPECT_EQ(1, observer.GetDestructionCount("two"));
+}
+
+TEST(fxcrt, CountRefAssignOverRetained) {
+  Observer observer;
+  {
+    CFX_CountRef<Object> ptr1;
+    ptr1.New(&observer, std::string("one"));
+    CFX_CountRef<Object> ptr2(ptr1);
+    ptr1.New(&observer, std::string("two"));
+    EXPECT_EQ(1, observer.GetConstructionCount("one"));
+    EXPECT_EQ(1, observer.GetConstructionCount("two"));
+    EXPECT_EQ(0, observer.GetDestructionCount("one"));
+    EXPECT_EQ(0, observer.GetDestructionCount("two"));
+  }
+  EXPECT_EQ(1, observer.GetDestructionCount("one"));
+  EXPECT_EQ(1, observer.GetDestructionCount("two"));
+}
+
+TEST(fxcrt, CountRefGetModify) {
+  Observer observer;
+  {
+    CFX_CountRef<Object> ptr;
+    EXPECT_NE(nullptr, ptr.GetModify(&observer, std::string("one")));
+    EXPECT_EQ(1, observer.GetConstructionCount("one"));
+    EXPECT_EQ(0, observer.GetDestructionCount("one"));
+
+    EXPECT_NE(nullptr, ptr.GetModify(&observer, std::string("one")));
+    EXPECT_EQ(1, observer.GetConstructionCount("one"));
+    EXPECT_EQ(0, observer.GetDestructionCount("one"));
+    {
+      CFX_CountRef<Object> other(ptr);
+      EXPECT_NE(nullptr, ptr.GetModify(&observer, std::string("one")));
+      EXPECT_EQ(2, observer.GetConstructionCount("one"));
+      EXPECT_EQ(0, observer.GetDestructionCount("one"));
+    }
+    EXPECT_EQ(2, observer.GetConstructionCount("one"));
+    EXPECT_EQ(1, observer.GetDestructionCount("one"));
+  }
+  EXPECT_EQ(2, observer.GetDestructionCount("one"));
+}
diff --git a/core/fxcrt/include/cfx_count_ref.h b/core/fxcrt/include/cfx_count_ref.h
new file mode 100644
index 0000000000..cc7cf3d9ed
--- /dev/null
+++ b/core/fxcrt/include/cfx_count_ref.h
@@ -0,0 +1,74 @@
+// 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 CORE_FXCRT_INCLUDE_CFX_COUNT_REF_H_
+#define CORE_FXCRT_INCLUDE_CFX_COUNT_REF_H_
+
+#include "core/fxcrt/include/cfx_retain_ptr.h"
+#include "core/fxcrt/include/fx_system.h"
+
+template <class ObjClass>
+class CFX_CountRef {
+ public:
+  CFX_CountRef() {}
+  CFX_CountRef(const CFX_CountRef& other) : m_pObject(other.m_pObject) {}
+  ~CFX_CountRef() {}
+
+  template <typename... Args>
+  ObjClass* New(Args... params) {
+    m_pObject.Reset(new CountedObj(params...));
+    return m_pObject.Get();
+  }
+
+  CFX_CountRef& operator=(const CFX_CountRef& that) {
+    if (*this != that)
+      m_pObject = that.m_pObject;
+    return *this;
+  }
+
+  void SetNull() { m_pObject.Reset(); }
+  bool IsNull() const { return !m_pObject; }
+  bool NotNull() const { return !IsNull(); }
+
+  const ObjClass* GetObject() const { return m_pObject.Get(); }
+
+  template <typename... Args>
+  ObjClass* GetModify(Args... params) {
+    if (!m_pObject)
+      return New(params...);
+    if (!m_pObject->HasOneRef())
+      m_pObject.Reset(new CountedObj(*m_pObject));
+    return m_pObject.Get();
+  }
+
+  bool operator==(const CFX_CountRef& that) const {
+    return m_pObject == that.m_pObject;
+  }
+  bool operator!=(const CFX_CountRef& that) const { return !(*this == that); }
+
+ protected:
+  class CountedObj : public ObjClass {
+   public:
+    template <typename... Args>
+    CountedObj(Args... params) : ObjClass(params...), m_RefCount(0) {}
+
+    CountedObj(const CountedObj& src) : ObjClass(src), m_RefCount(0) {}
+
+    bool HasOneRef() const { return m_RefCount == 1; }
+    void Retain() { m_RefCount++; }
+    void Release() {
+      if (--m_RefCount <= 0)
+        delete this;
+    }
+
+   private:
+    intptr_t m_RefCount;
+  };
+
+  CFX_RetainPtr<CountedObj> m_pObject;
+};
+
+#endif  // CORE_FXCRT_INCLUDE_CFX_COUNT_REF_H_
diff --git a/core/fxcrt/include/fx_basic.h b/core/fxcrt/include/fx_basic.h
index bc05a3479d..6a8988d5dc 100644
--- a/core/fxcrt/include/fx_basic.h
+++ b/core/fxcrt/include/fx_basic.h
@@ -649,75 +649,6 @@ class CFX_BitStream {
   const uint8_t* m_pData;
 };
 
-template <class ObjClass>
-class CFX_CountRef {
- public:
-  using Ref = CFX_CountRef<ObjClass>;
-
-  class CountedObj : public ObjClass {
-   public:
-    CountedObj() {}
-    CountedObj(const CountedObj& src) : ObjClass(src) {}
-
-    int m_RefCount;
-  };
-
-  CFX_CountRef() : m_pObject(nullptr) {}
-  CFX_CountRef(const Ref& ref) : m_pObject(ref.m_pObject) {
-    if (m_pObject)
-      m_pObject->m_RefCount++;
-  }
-
-  ~CFX_CountRef() { SetNull(); }
-
-  ObjClass* New() {
-    SetNull();
-    m_pObject = new CountedObj;
-    m_pObject->m_RefCount = 1;
-    return m_pObject;
-  }
-
-  void operator=(const Ref& ref) {
-    if (ref.m_pObject)
-      ref.m_pObject->m_RefCount++;
-    SetNull();
-    m_pObject = ref.m_pObject;
-  }
-
-  bool IsNull() const { return !m_pObject; }
-  bool NotNull() const { return !IsNull(); }
-
-  const ObjClass* GetObject() const { return m_pObject; }
-  ObjClass* GetModify() {
-    if (!m_pObject) {
-      m_pObject = new CountedObj;
-      m_pObject->m_RefCount = 1;
-    } else if (m_pObject->m_RefCount > 1) {
-      m_pObject->m_RefCount--;
-      CountedObj* pOldObject = m_pObject;
-      m_pObject = new CountedObj(*pOldObject);
-      m_pObject->m_RefCount = 1;
-    }
-    return m_pObject;
-  }
-
-  void SetNull() {
-    if (!m_pObject) {
-      return;
-    }
-    m_pObject->m_RefCount--;
-    if (m_pObject->m_RefCount <= 0) {
-      delete m_pObject;
-    }
-    m_pObject = nullptr;
-  }
-
-  bool operator==(const Ref& ref) const { return m_pObject == ref.m_pObject; }
-
- protected:
-  CountedObj* m_pObject;
-};
-
 class IFX_Pause {
  public:
   virtual ~IFX_Pause() {}
diff --git a/core/fxge/include/fx_dib.h b/core/fxge/include/fx_dib.h
index 540996cfc1..011e6f7856 100644
--- a/core/fxge/include/fx_dib.h
+++ b/core/fxge/include/fx_dib.h
@@ -10,6 +10,7 @@
 #include <memory>
 #include <vector>
 
+#include "core/fxcrt/include/cfx_count_ref.h"
 #include "core/fxcrt/include/fx_basic.h"
 #include "core/fxcrt/include/fx_coordinates.h"
 
@@ -389,6 +390,7 @@ class CFX_DIBExtractor {
 };
 
 typedef CFX_CountRef<CFX_DIBitmap> CFX_DIBitmapRef;
+
 class CFX_FilteredDIB : public CFX_DIBSource {
  public:
   CFX_FilteredDIB();