Use CFX_RetainPtr to ref count CFX_ByteStringchromium/2696

This moves the reference counts from an ad-hoc mechanism
to being automatically tracked.

Also:
Make StringData::Create() always return non-null.
Add better ctors for StringData itself.
Consolidate copies into StringData methods.
Simplify the concat() code.
Rename/reorder some parameter names to be simpler.

Review URL: https://codereview.chromium.org/1810823002

3 files changed, 391 insertions, 470 deletions

diff --git a/core/fxcrt/fx_basic_bstring.cpp b/core/fxcrt/fx_basic_bstring.cpp
index ab9299694e..e54da84f7e 100644
--- a/core/fxcrt/fx_basic_bstring.cpp
+++ b/core/fxcrt/fx_basic_bstring.cpp
@@ -43,18 +43,16 @@ static int _Buffer_itoa(char* buf, int i, uint32_t flags) {
   }
   return len;
 }
+
 CFX_ByteString CFX_ByteString::FormatInteger(int i, uint32_t flags) {
   char buf[32];
   return CFX_ByteStringC(buf, _Buffer_itoa(buf, i, flags));
 }
 
 // static
-CFX_ByteString::StringData* CFX_ByteString::StringData::Create(int nLen) {
-  // |nLen| is currently declared as in |int|. TODO(palmer): It should be
-  // a |size_t|, or at least unsigned.
-  if (nLen == 0 || nLen < 0) {
-    return NULL;
-  }
+CFX_ByteString::StringData* CFX_ByteString::StringData::Create(
+    FX_STRSIZE nLen) {
+  FXSYS_assert(nLen > 0);
 
   // Fixed portion of header plus a NUL char not included in m_nAllocLength.
   // sizeof(FX_CHAR) is always 1, used for consistency with CFX_Widestring.
@@ -74,296 +72,301 @@ CFX_ByteString::StringData* CFX_ByteString::StringData::Create(int nLen) {
   void* pData = FX_Alloc(uint8_t, totalSize);
   return new (pData) StringData(nLen, usableSize);
 }
-CFX_ByteString::~CFX_ByteString() {
-  if (m_pData) {
-    m_pData->Release();
-  }
+
+CFX_ByteString::StringData* CFX_ByteString::StringData::Create(
+    const StringData& other) {
+  StringData* result = Create(other.m_nDataLength);
+  result->CopyContents(other);
+  return result;
 }
-CFX_ByteString::CFX_ByteString(const FX_CHAR* lpsz, FX_STRSIZE nLen) {
-  if (nLen < 0) {
-    nLen = lpsz ? FXSYS_strlen(lpsz) : 0;
-  }
-  if (nLen) {
-    m_pData = StringData::Create(nLen);
-    if (m_pData) {
-      FXSYS_memcpy(m_pData->m_String, lpsz, nLen);
-    }
-  } else {
-    m_pData = NULL;
-  }
+
+CFX_ByteString::StringData* CFX_ByteString::StringData::Create(
+    const FX_CHAR* pStr,
+    FX_STRSIZE nLen) {
+  StringData* result = Create(nLen);
+  result->CopyContents(pStr, nLen);
+  return result;
+}
+
+CFX_ByteString::StringData::StringData(FX_STRSIZE dataLen, FX_STRSIZE allocLen)
+    : m_nRefs(0), m_nDataLength(dataLen), m_nAllocLength(allocLen) {
+  FXSYS_assert(dataLen >= 0);
+  FXSYS_assert(dataLen <= allocLen);
+  m_String[dataLen] = 0;
+}
+
+void CFX_ByteString::StringData::CopyContents(const StringData& other) {
+  FXSYS_assert(other.m_nDataLength <= m_nAllocLength);
+  FXSYS_memcpy(m_String, other.m_String, other.m_nDataLength + 1);
+}
+
+void CFX_ByteString::StringData::CopyContents(const FX_CHAR* pStr,
+                                              FX_STRSIZE nLen) {
+  FXSYS_assert(nLen >= 0 && nLen <= m_nAllocLength);
+  FXSYS_memcpy(m_String, pStr, nLen);
+  m_String[nLen] = 0;
 }
-CFX_ByteString::CFX_ByteString(const uint8_t* lpsz, FX_STRSIZE nLen) {
+
+void CFX_ByteString::StringData::CopyContentsAt(FX_STRSIZE offset,
+                                                const FX_CHAR* pStr,
+                                                FX_STRSIZE nLen) {
+  FXSYS_assert(offset >= 0 && nLen >= 0 && offset + nLen <= m_nAllocLength);
+  FXSYS_memcpy(m_String + offset, pStr, nLen);
+  m_String[offset + nLen] = 0;
+}
+
+CFX_ByteString::CFX_ByteString(const FX_CHAR* pStr, FX_STRSIZE nLen) {
+  if (nLen < 0)
+    nLen = pStr ? FXSYS_strlen(pStr) : 0;
+
+  if (nLen)
+    m_pData.Reset(StringData::Create(pStr, nLen));
+}
+
+CFX_ByteString::CFX_ByteString(const uint8_t* pStr, FX_STRSIZE nLen) {
   if (nLen > 0) {
-    m_pData = StringData::Create(nLen);
-    if (m_pData) {
-      FXSYS_memcpy(m_pData->m_String, lpsz, nLen);
-    }
-  } else {
-    m_pData = NULL;
+    m_pData.Reset(
+        StringData::Create(reinterpret_cast<const FX_CHAR*>(pStr), nLen));
   }
 }
+
 CFX_ByteString::CFX_ByteString(char ch) {
-  m_pData = StringData::Create(1);
-  if (m_pData) {
-    m_pData->m_String[0] = ch;
-  }
-}
-CFX_ByteString::CFX_ByteString(const CFX_ByteString& stringSrc) {
-  if (!stringSrc.m_pData) {
-    m_pData = NULL;
-    return;
-  }
-  if (stringSrc.m_pData->m_nRefs >= 0) {
-    m_pData = stringSrc.m_pData;
-    m_pData->Retain();
-  } else {
-    m_pData = NULL;
-    *this = stringSrc;
-  }
+  m_pData.Reset(StringData::Create(1));
+  m_pData->m_String[0] = ch;
 }
+
 CFX_ByteString::CFX_ByteString(const CFX_ByteStringC& stringSrc) {
-  if (stringSrc.IsEmpty()) {
-    m_pData = NULL;
-    return;
+  if (!stringSrc.IsEmpty()) {
+    m_pData.Reset(
+        StringData::Create(stringSrc.GetCStr(), stringSrc.GetLength()));
   }
-  m_pData = NULL;
-  *this = stringSrc;
 }
+
 CFX_ByteString::CFX_ByteString(const CFX_ByteStringC& str1,
                                const CFX_ByteStringC& str2) {
-  m_pData = NULL;
   int nNewLen = str1.GetLength() + str2.GetLength();
-  if (nNewLen == 0) {
+  if (nNewLen == 0)
     return;
-  }
-  m_pData = StringData::Create(nNewLen);
-  if (m_pData) {
-    FXSYS_memcpy(m_pData->m_String, str1.GetCStr(), str1.GetLength());
-    FXSYS_memcpy(m_pData->m_String + str1.GetLength(), str2.GetCStr(),
-                 str2.GetLength());
-  }
+
+  m_pData.Reset(StringData::Create(nNewLen));
+  m_pData->CopyContents(str1.GetCStr(), str1.GetLength());
+  m_pData->CopyContentsAt(str1.GetLength(), str2.GetCStr(), str2.GetLength());
 }
-const CFX_ByteString& CFX_ByteString::operator=(const FX_CHAR* lpsz) {
-  if (!lpsz || lpsz[0] == 0) {
-    Empty();
-  } else {
-    AssignCopy(FXSYS_strlen(lpsz), lpsz);
-  }
+
+CFX_ByteString::~CFX_ByteString() {}
+
+const CFX_ByteString& CFX_ByteString::operator=(const FX_CHAR* pStr) {
+  if (!pStr || !pStr[0])
+    clear();
+  else
+    AssignCopy(pStr, FXSYS_strlen(pStr));
+
   return *this;
 }
+
 const CFX_ByteString& CFX_ByteString::operator=(const CFX_ByteStringC& str) {
-  if (str.IsEmpty()) {
-    Empty();
-  } else {
-    AssignCopy(str.GetLength(), str.GetCStr());
-  }
+  if (str.IsEmpty())
+    clear();
+  else
+    AssignCopy(str.GetCStr(), str.GetLength());
+
   return *this;
 }
+
 const CFX_ByteString& CFX_ByteString::operator=(
     const CFX_ByteString& stringSrc) {
-  if (m_pData == stringSrc.m_pData) {
-    return *this;
-  }
-  if (stringSrc.IsEmpty()) {
-    Empty();
-  } else if ((m_pData && m_pData->m_nRefs < 0) ||
-             (stringSrc.m_pData && stringSrc.m_pData->m_nRefs < 0)) {
-    AssignCopy(stringSrc.m_pData->m_nDataLength, stringSrc.m_pData->m_String);
-  } else {
-    Empty();
+  if (m_pData != stringSrc.m_pData)
     m_pData = stringSrc.m_pData;
-    if (m_pData) {
-      m_pData->Retain();
-    }
-  }
+
   return *this;
 }
+
 const CFX_ByteString& CFX_ByteString::operator=(const CFX_BinaryBuf& buf) {
   Load(buf.GetBuffer(), buf.GetSize());
   return *this;
 }
+
 void CFX_ByteString::Load(const uint8_t* buf, FX_STRSIZE len) {
-  Empty();
-  if (len) {
-    m_pData = StringData::Create(len);
-    if (m_pData) {
-      FXSYS_memcpy(m_pData->m_String, buf, len);
-    }
-  } else {
-    m_pData = NULL;
+  if (!len) {
+    clear();
+    return;
   }
+
+  m_pData.Reset(StringData::Create(reinterpret_cast<const FX_CHAR*>(buf), len));
 }
-const CFX_ByteString& CFX_ByteString::operator+=(const FX_CHAR* lpsz) {
-  if (lpsz) {
-    ConcatInPlace(FXSYS_strlen(lpsz), lpsz);
-  }
+
+const CFX_ByteString& CFX_ByteString::operator+=(const FX_CHAR* pStr) {
+  if (pStr)
+    Concat(pStr, FXSYS_strlen(pStr));
+
   return *this;
 }
+
 const CFX_ByteString& CFX_ByteString::operator+=(char ch) {
-  ConcatInPlace(1, &ch);
+  Concat(&ch, 1);
   return *this;
 }
+
 const CFX_ByteString& CFX_ByteString::operator+=(const CFX_ByteString& str) {
-  if (!str.m_pData) {
-    return *this;
-  }
-  ConcatInPlace(str.m_pData->m_nDataLength, str.m_pData->m_String);
+  if (str.m_pData)
+    Concat(str.m_pData->m_String, str.m_pData->m_nDataLength);
+
   return *this;
 }
+
 const CFX_ByteString& CFX_ByteString::operator+=(const CFX_ByteStringC& str) {
-  if (str.IsEmpty()) {
-    return *this;
-  }
-  ConcatInPlace(str.GetLength(), str.GetCStr());
+  if (!str.IsEmpty())
+    Concat(str.GetCStr(), str.GetLength());
+
   return *this;
 }
+
 bool CFX_ByteString::Equal(const char* ptr) const {
-  if (!m_pData) {
+  if (!m_pData)
     return !ptr || ptr[0] == '\0';
-  }
-  if (!ptr) {
+
+  if (!ptr)
     return m_pData->m_nDataLength == 0;
-  }
+
   return FXSYS_strlen(ptr) == m_pData->m_nDataLength &&
          FXSYS_memcmp(ptr, m_pData->m_String, m_pData->m_nDataLength) == 0;
 }
+
 bool CFX_ByteString::Equal(const CFX_ByteStringC& str) const {
-  if (!m_pData) {
+  if (!m_pData)
     return str.IsEmpty();
-  }
+
   return m_pData->m_nDataLength == str.GetLength() &&
          FXSYS_memcmp(m_pData->m_String, str.GetCStr(), str.GetLength()) == 0;
 }
+
 bool CFX_ByteString::Equal(const CFX_ByteString& other) const {
-  if (IsEmpty()) {
+  if (IsEmpty())
     return other.IsEmpty();
-  }
-  if (other.IsEmpty()) {
+
+  if (other.IsEmpty())
     return false;
-  }
+
   return other.m_pData->m_nDataLength == m_pData->m_nDataLength &&
          FXSYS_memcmp(other.m_pData->m_String, m_pData->m_String,
                       m_pData->m_nDataLength) == 0;
 }
-void CFX_ByteString::Empty() {
-  if (m_pData) {
-    m_pData->Release();
-    m_pData = NULL;
-  }
-}
+
 bool CFX_ByteString::EqualNoCase(const CFX_ByteStringC& str) const {
-  if (!m_pData) {
+  if (!m_pData)
     return str.IsEmpty();
-  }
+
   FX_STRSIZE len = str.GetLength();
-  if (m_pData->m_nDataLength != len) {
+  if (m_pData->m_nDataLength != len)
     return false;
-  }
+
   const uint8_t* pThis = (const uint8_t*)m_pData->m_String;
   const uint8_t* pThat = str.GetPtr();
   for (FX_STRSIZE i = 0; i < len; i++) {
     if ((*pThis) != (*pThat)) {
       uint8_t bThis = *pThis;
-      if (bThis >= 'A' && bThis <= 'Z') {
+      if (bThis >= 'A' && bThis <= 'Z')
         bThis += 'a' - 'A';
-      }
+
       uint8_t bThat = *pThat;
-      if (bThat >= 'A' && bThat <= 'Z') {
+      if (bThat >= 'A' && bThat <= 'Z')
         bThat += 'a' - 'A';
-      }
-      if (bThis != bThat) {
+
+      if (bThis != bThat)
         return false;
-      }
     }
     pThis++;
     pThat++;
   }
   return true;
 }
-void CFX_ByteString::AssignCopy(FX_STRSIZE nSrcLen,
-                                const FX_CHAR* lpszSrcData) {
+
+void CFX_ByteString::AssignCopy(const FX_CHAR* pSrcData, FX_STRSIZE nSrcLen) {
   AllocBeforeWrite(nSrcLen);
-  FXSYS_memcpy(m_pData->m_String, lpszSrcData, nSrcLen);
+  m_pData->CopyContents(pSrcData, nSrcLen);
   m_pData->m_nDataLength = nSrcLen;
-  m_pData->m_String[nSrcLen] = 0;
 }
+
 void CFX_ByteString::CopyBeforeWrite() {
-  if (!m_pData || m_pData->m_nRefs <= 1) {
+  if (!m_pData || m_pData->CanOperateInPlace(m_pData->m_nDataLength))
+    return;
+
+  if (!m_pData->m_nDataLength) {
+    clear();
     return;
   }
-  StringData* pData = m_pData;
-  m_pData->Release();
-  FX_STRSIZE nDataLength = pData->m_nDataLength;
-  m_pData = StringData::Create(nDataLength);
-  if (m_pData) {
-    FXSYS_memcpy(m_pData->m_String, pData->m_String, nDataLength + 1);
-  }
+
+  CFX_RetainPtr<StringData> pData(StringData::Create(*m_pData));
+  m_pData.Swap(pData);
 }
+
 void CFX_ByteString::AllocBeforeWrite(FX_STRSIZE nLen) {
-  if (m_pData && m_pData->m_nRefs <= 1 && m_pData->m_nAllocLength >= nLen) {
+  if (m_pData && m_pData->CanOperateInPlace(nLen))
+    return;
+
+  if (!nLen) {
+    clear();
     return;
   }
-  Empty();
-  m_pData = StringData::Create(nLen);
+
+  m_pData.Reset(StringData::Create(nLen));
 }
+
 void CFX_ByteString::ReleaseBuffer(FX_STRSIZE nNewLength) {
-  if (!m_pData) {
+  if (!m_pData)
     return;
-  }
-  CopyBeforeWrite();
-  if (nNewLength == -1) {
-    nNewLength = FXSYS_strlen((const FX_CHAR*)m_pData->m_String);
-  }
+
+  if (nNewLength == -1)
+    nNewLength = FXSYS_strlen(m_pData->m_String);
+
   if (nNewLength == 0) {
-    Empty();
+    clear();
     return;
   }
+
   FXSYS_assert(nNewLength <= m_pData->m_nAllocLength);
+  CopyBeforeWrite();
   m_pData->m_nDataLength = nNewLength;
   m_pData->m_String[nNewLength] = 0;
 }
+
 void CFX_ByteString::Reserve(FX_STRSIZE len) {
   GetBuffer(len);
   ReleaseBuffer(GetLength());
 }
+
 FX_CHAR* CFX_ByteString::GetBuffer(FX_STRSIZE nMinBufLength) {
-  if (!m_pData && nMinBufLength == 0) {
-    return NULL;
-  }
-  if (m_pData && m_pData->m_nRefs <= 1 &&
-      m_pData->m_nAllocLength >= nMinBufLength) {
-    return m_pData->m_String;
-  }
   if (!m_pData) {
-    m_pData = StringData::Create(nMinBufLength);
-    if (!m_pData) {
-      return NULL;
-    }
+    if (nMinBufLength == 0)
+      return nullptr;
+
+    m_pData.Reset(StringData::Create(nMinBufLength));
     m_pData->m_nDataLength = 0;
     m_pData->m_String[0] = 0;
     return m_pData->m_String;
   }
-  StringData* pOldData = m_pData;
-  FX_STRSIZE nOldLen = pOldData->m_nDataLength;
-  if (nMinBufLength < nOldLen) {
-    nMinBufLength = nOldLen;
-  }
-  m_pData = StringData::Create(nMinBufLength);
-  if (!m_pData) {
-    return NULL;
-  }
-  FXSYS_memcpy(m_pData->m_String, pOldData->m_String, (nOldLen + 1));
-  m_pData->m_nDataLength = nOldLen;
-  pOldData->Release();
+
+  if (m_pData->CanOperateInPlace(nMinBufLength))
+    return m_pData->m_String;
+
+  nMinBufLength = std::max(nMinBufLength, m_pData->m_nDataLength);
+  if (nMinBufLength == 0)
+    return nullptr;
+
+  CFX_RetainPtr<StringData> pNewData(StringData::Create(nMinBufLength));
+  pNewData->CopyContents(*m_pData);
+  pNewData->m_nDataLength = m_pData->m_nDataLength;
+  m_pData.Swap(pNewData);
   return m_pData->m_String;
 }
+
 FX_STRSIZE CFX_ByteString::Delete(FX_STRSIZE nIndex, FX_STRSIZE nCount) {
-  if (!m_pData) {
+  if (!m_pData)
     return 0;
-  }
-  if (nIndex < 0) {
+
+  if (nIndex < 0)
     nIndex = 0;
-  }
+
   FX_STRSIZE nOldLength = m_pData->m_nDataLength;
   if (nCount > 0 && nIndex < nOldLength) {
     FX_STRSIZE mLength = nIndex + nCount;
@@ -379,91 +382,66 @@ FX_STRSIZE CFX_ByteString::Delete(FX_STRSIZE nIndex, FX_STRSIZE nCount) {
   }
   return m_pData->m_nDataLength;
 }
-void CFX_ByteString::ConcatInPlace(FX_STRSIZE nSrcLen,
-                                   const FX_CHAR* lpszSrcData) {
-  if (nSrcLen == 0 || !lpszSrcData) {
+
+void CFX_ByteString::Concat(const FX_CHAR* pSrcData, FX_STRSIZE nSrcLen) {
+  if (!pSrcData || nSrcLen <= 0)
     return;
-  }
+
   if (!m_pData) {
-    m_pData = StringData::Create(nSrcLen);
-    if (!m_pData) {
-      return;
-    }
-    FXSYS_memcpy(m_pData->m_String, lpszSrcData, nSrcLen);
+    m_pData.Reset(StringData::Create(pSrcData, nSrcLen));
     return;
   }
-  if (m_pData->m_nRefs > 1 ||
-      m_pData->m_nDataLength + nSrcLen > m_pData->m_nAllocLength) {
-    ConcatCopy(m_pData->m_nDataLength, m_pData->m_String, nSrcLen, lpszSrcData);
-  } else {
-    FXSYS_memcpy(m_pData->m_String + m_pData->m_nDataLength, lpszSrcData,
-                 nSrcLen);
+
+  if (m_pData->CanOperateInPlace(m_pData->m_nDataLength + nSrcLen)) {
+    m_pData->CopyContentsAt(m_pData->m_nDataLength, pSrcData, nSrcLen);
     m_pData->m_nDataLength += nSrcLen;
-    m_pData->m_String[m_pData->m_nDataLength] = 0;
-  }
-}
-void CFX_ByteString::ConcatCopy(FX_STRSIZE nSrc1Len,
-                                const FX_CHAR* lpszSrc1Data,
-                                FX_STRSIZE nSrc2Len,
-                                const FX_CHAR* lpszSrc2Data) {
-  int nNewLen = nSrc1Len + nSrc2Len;
-  if (nNewLen <= 0) {
     return;
   }
-  // Don't release until done copying, might be one of the arguments.
-  StringData* pOldData = m_pData;
-  m_pData = StringData::Create(nNewLen);
-  if (m_pData) {
-    memcpy(m_pData->m_String, lpszSrc1Data, nSrc1Len);
-    memcpy(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len);
-  }
-  pOldData->Release();
+
+  CFX_RetainPtr<StringData> pNewData(
+      StringData::Create(m_pData->m_nDataLength + nSrcLen));
+  pNewData->CopyContents(*m_pData);
+  pNewData->CopyContentsAt(m_pData->m_nDataLength, pSrcData, nSrcLen);
+  m_pData.Swap(pNewData);
 }
+
 CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE nFirst) const {
-  if (!m_pData) {
-    return CFX_ByteString();
-  }
   return Mid(nFirst, m_pData->m_nDataLength - nFirst);
 }
+
 CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE nFirst, FX_STRSIZE nCount) const {
-  if (nFirst < 0) {
-    nFirst = 0;
-  }
-  if (nCount < 0) {
-    nCount = 0;
-  }
-  if (nFirst + nCount > m_pData->m_nDataLength) {
-    nCount = m_pData->m_nDataLength - nFirst;
-  }
-  if (nFirst > m_pData->m_nDataLength) {
-    nCount = 0;
-  }
-  if (nFirst == 0 && nFirst + nCount == m_pData->m_nDataLength) {
+  if (!m_pData)
+    return CFX_ByteString();
+
+  nFirst = std::min(std::max(nFirst, 0), m_pData->m_nDataLength);
+  nCount = std::min(std::max(nCount, 0), m_pData->m_nDataLength - nFirst);
+  if (nCount == 0)
+    return CFX_ByteString();
+
+  if (nFirst == 0 && nFirst + nCount == m_pData->m_nDataLength)
     return *this;
-  }
+
   CFX_ByteString dest;
   AllocCopy(dest, nCount, nFirst);
   return dest;
 }
+
 void CFX_ByteString::AllocCopy(CFX_ByteString& dest,
                                FX_STRSIZE nCopyLen,
                                FX_STRSIZE nCopyIndex) const {
-  // |FX_STRSIZE| is currently typedef'd as in |int|. TODO(palmer): It
-  // should be a |size_t|, or at least unsigned.
-  if (nCopyLen == 0 || nCopyLen < 0) {
+  if (nCopyLen <= 0)
     return;
-  }
-  ASSERT(!dest.m_pData);
-  dest.m_pData = StringData::Create(nCopyLen);
-  if (dest.m_pData) {
-    FXSYS_memcpy(dest.m_pData->m_String, m_pData->m_String + nCopyIndex,
-                 nCopyLen);
-  }
+
+  CFX_RetainPtr<StringData> pNewData(
+      StringData::Create(m_pData->m_String + nCopyIndex, nCopyLen));
+  dest.m_pData.Swap(pNewData);
 }
+
 #define FORCE_ANSI 0x10000
 #define FORCE_UNICODE 0x20000
 #define FORCE_INT64 0x40000
-void CFX_ByteString::FormatV(const FX_CHAR* lpszFormat, va_list argList) {
+
+void CFX_ByteString::FormatV(const FX_CHAR* pFormat, va_list argList) {
   va_list argListSave;
 #if defined(__ARMCC_VERSION) ||                                              \
     (!defined(_MSC_VER) && (_FX_CPU_ == _FX_X64_ || _FX_CPU_ == _FX_IA64_ || \
@@ -474,71 +452,71 @@ void CFX_ByteString::FormatV(const FX_CHAR* lpszFormat, va_list argList) {
   argListSave = argList;
 #endif
   int nMaxLen = 0;
-  for (const FX_CHAR* lpsz = lpszFormat; *lpsz != 0; lpsz++) {
-    if (*lpsz != '%' || *(lpsz = lpsz + 1) == '%') {
-      nMaxLen += FXSYS_strlen(lpsz);
+  for (const FX_CHAR* pStr = pFormat; *pStr != 0; pStr++) {
+    if (*pStr != '%' || *(pStr = pStr + 1) == '%') {
+      nMaxLen += FXSYS_strlen(pStr);
       continue;
     }
     int nItemLen = 0;
     int nWidth = 0;
-    for (; *lpsz != 0; lpsz++) {
-      if (*lpsz == '#') {
+    for (; *pStr != 0; pStr++) {
+      if (*pStr == '#') {
         nMaxLen += 2;
-      } else if (*lpsz == '*') {
+      } else if (*pStr == '*') {
         nWidth = va_arg(argList, int);
-      } else if (*lpsz != '-' && *lpsz != '+' && *lpsz != '0' && *lpsz != ' ') {
+      } else if (*pStr != '-' && *pStr != '+' && *pStr != '0' && *pStr != ' ') {
         break;
       }
     }
     if (nWidth == 0) {
-      nWidth = FXSYS_atoi(lpsz);
-      while (std::isdigit(*lpsz))
-        lpsz++;
+      nWidth = FXSYS_atoi(pStr);
+      while (std::isdigit(*pStr))
+        pStr++;
     }
     if (nWidth < 0 || nWidth > 128 * 1024) {
-      lpszFormat = "Bad width";
+      pFormat = "Bad width";
       nMaxLen = 10;
       break;
     }
     int nPrecision = 0;
-    if (*lpsz == '.') {
-      lpsz++;
-      if (*lpsz == '*') {
+    if (*pStr == '.') {
+      pStr++;
+      if (*pStr == '*') {
         nPrecision = va_arg(argList, int);
-        lpsz++;
+        pStr++;
       } else {
-        nPrecision = FXSYS_atoi(lpsz);
-        while (std::isdigit(*lpsz))
-          lpsz++;
+        nPrecision = FXSYS_atoi(pStr);
+        while (std::isdigit(*pStr))
+          pStr++;
       }
     }
     if (nPrecision < 0 || nPrecision > 128 * 1024) {
-      lpszFormat = "Bad precision";
+      pFormat = "Bad precision";
       nMaxLen = 14;
       break;
     }
     int nModifier = 0;
-    if (FXSYS_strncmp(lpsz, "I64", 3) == 0) {
-      lpsz += 3;
+    if (FXSYS_strncmp(pStr, "I64", 3) == 0) {
+      pStr += 3;
       nModifier = FORCE_INT64;
     } else {
-      switch (*lpsz) {
+      switch (*pStr) {
         case 'h':
           nModifier = FORCE_ANSI;
-          lpsz++;
+          pStr++;
           break;
         case 'l':
           nModifier = FORCE_UNICODE;
-          lpsz++;
+          pStr++;
           break;
         case 'F':
         case 'N':
         case 'L':
-          lpsz++;
+          pStr++;
           break;
       }
     }
-    switch (*lpsz | nModifier) {
+    switch (*pStr | nModifier) {
       case 'c':
       case 'C':
         nItemLen = 2;
@@ -609,7 +587,7 @@ void CFX_ByteString::FormatV(const FX_CHAR* lpszFormat, va_list argList) {
         nItemLen = nWidth;
       }
     } else {
-      switch (*lpsz) {
+      switch (*pStr) {
         case 'd':
         case 'i':
         case 'u':
@@ -668,105 +646,94 @@ void CFX_ByteString::FormatV(const FX_CHAR* lpszFormat, va_list argList) {
   GetBuffer(nMaxLen);
   if (m_pData) {
     memset(m_pData->m_String, 0, nMaxLen);
-    FXSYS_vsnprintf(m_pData->m_String, nMaxLen - 1, lpszFormat, argListSave);
+    FXSYS_vsnprintf(m_pData->m_String, nMaxLen - 1, pFormat, argListSave);
     ReleaseBuffer();
   }
   va_end(argListSave);
 }
-void CFX_ByteString::Format(const FX_CHAR* lpszFormat, ...) {
+
+void CFX_ByteString::Format(const FX_CHAR* pFormat, ...) {
   va_list argList;
-  va_start(argList, lpszFormat);
-  FormatV(lpszFormat, argList);
+  va_start(argList, pFormat);
+  FormatV(pFormat, argList);
   va_end(argList);
 }
+
 FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE nIndex, FX_CHAR ch) {
-  CopyBeforeWrite();
-  if (nIndex < 0) {
-    nIndex = 0;
-  }
   FX_STRSIZE nNewLength = m_pData ? m_pData->m_nDataLength : 0;
-  if (nIndex > nNewLength) {
-    nIndex = nNewLength;
-  }
+  nIndex = std::max(nIndex, 0);
+  nIndex = std::min(nIndex, nNewLength);
   nNewLength++;
+
+  CopyBeforeWrite();
   if (!m_pData || m_pData->m_nAllocLength < nNewLength) {
-    StringData* pOldData = m_pData;
-    const FX_CHAR* pstr = m_pData->m_String;
-    m_pData = StringData::Create(nNewLength);
-    if (!m_pData) {
-      return 0;
-    }
-    if (pOldData) {
-      FXSYS_memmove(m_pData->m_String, pstr, (pOldData->m_nDataLength + 1));
-      pOldData->Release();
-    } else {
-      m_pData->m_String[0] = 0;
-    }
+    CFX_RetainPtr<StringData> pNewData(StringData::Create(nNewLength));
+    pNewData->CopyContents(*m_pData);
+    m_pData.Swap(pNewData);
   }
+
   FXSYS_memmove(m_pData->m_String + nIndex + 1, m_pData->m_String + nIndex,
-                (nNewLength - nIndex));
+                nNewLength - nIndex);
   m_pData->m_String[nIndex] = ch;
   m_pData->m_nDataLength = nNewLength;
   return nNewLength;
 }
+
 CFX_ByteString CFX_ByteString::Right(FX_STRSIZE nCount) const {
-  if (!m_pData) {
+  if (!m_pData)
     return CFX_ByteString();
-  }
-  if (nCount < 0) {
-    nCount = 0;
-  }
-  if (nCount >= m_pData->m_nDataLength) {
+
+  nCount = std::max(nCount, 0);
+  if (nCount >= m_pData->m_nDataLength)
     return *this;
-  }
+
   CFX_ByteString dest;
   AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount);
   return dest;
 }
+
 CFX_ByteString CFX_ByteString::Left(FX_STRSIZE nCount) const {
-  if (!m_pData) {
+  if (!m_pData)
     return CFX_ByteString();
-  }
-  if (nCount < 0) {
-    nCount = 0;
-  }
-  if (nCount >= m_pData->m_nDataLength) {
+
+  nCount = std::max(nCount, 0);
+  if (nCount >= m_pData->m_nDataLength)
     return *this;
-  }
+
   CFX_ByteString dest;
   AllocCopy(dest, nCount, 0);
   return dest;
 }
+
 FX_STRSIZE CFX_ByteString::Find(FX_CHAR ch, FX_STRSIZE nStart) const {
-  if (!m_pData) {
+  if (!m_pData)
     return -1;
-  }
-  FX_STRSIZE nLength = m_pData->m_nDataLength;
-  if (nStart >= nLength) {
+
+  if (nStart >= m_pData->m_nDataLength)
     return -1;
-  }
-  const FX_CHAR* lpsz = FXSYS_strchr(m_pData->m_String + nStart, ch);
-  return lpsz ? (int)(lpsz - m_pData->m_String) : -1;
+
+  const FX_CHAR* pStr = FXSYS_strchr(m_pData->m_String + nStart, ch);
+  return pStr ? (int)(pStr - m_pData->m_String) : -1;
 }
+
 FX_STRSIZE CFX_ByteString::ReverseFind(FX_CHAR ch) const {
-  if (!m_pData) {
+  if (!m_pData)
     return -1;
-  }
+
   FX_STRSIZE nLength = m_pData->m_nDataLength;
-  while (nLength) {
-    if (m_pData->m_String[nLength - 1] == ch) {
-      return nLength - 1;
-    }
-    nLength--;
+  while (nLength--) {
+    if (m_pData->m_String[nLength] == ch)
+      return nLength;
   }
   return -1;
 }
+
 const FX_CHAR* FX_strstr(const FX_CHAR* str1,
                          int len1,
                          const FX_CHAR* str2,
                          int len2) {
   if (len2 > len1 || len2 == 0) {
-    return NULL;
+    return nullptr;
   }
   const FX_CHAR* end_ptr = str1 + len1 - len2;
   while (str1 <= end_ptr) {
@@ -782,42 +749,46 @@ const FX_CHAR* FX_strstr(const FX_CHAR* str1,
     }
     str1++;
   }
-  return NULL;
+  return nullptr;
 }
-FX_STRSIZE CFX_ByteString::Find(const CFX_ByteStringC& lpszSub,
+
+FX_STRSIZE CFX_ByteString::Find(const CFX_ByteStringC& pSub,
                                 FX_STRSIZE nStart) const {
-  if (!m_pData) {
+  if (!m_pData)
     return -1;
-  }
+
   FX_STRSIZE nLength = m_pData->m_nDataLength;
-  if (nStart > nLength) {
+  if (nStart > nLength)
     return -1;
-  }
-  const FX_CHAR* lpsz =
+
+  const FX_CHAR* pStr =
       FX_strstr(m_pData->m_String + nStart, m_pData->m_nDataLength - nStart,
-                lpszSub.GetCStr(), lpszSub.GetLength());
-  return lpsz ? (int)(lpsz - m_pData->m_String) : -1;
+                pSub.GetCStr(), pSub.GetLength());
+  return pStr ? (int)(pStr - m_pData->m_String) : -1;
 }
+
 void CFX_ByteString::MakeLower() {
-  if (!m_pData) {
+  if (!m_pData)
     return;
-  }
+
   CopyBeforeWrite();
-  if (GetLength() < 1) {
+  if (GetLength() < 1)
     return;
-  }
+
   FXSYS_strlwr(m_pData->m_String);
 }
+
 void CFX_ByteString::MakeUpper() {
-  if (!m_pData) {
+  if (!m_pData)
     return;
-  }
+
   CopyBeforeWrite();
-  if (GetLength() < 1) {
+  if (GetLength() < 1)
     return;
-  }
+
   FXSYS_strupr(m_pData->m_String);
 }
+
 FX_STRSIZE CFX_ByteString::Remove(FX_CHAR chRemove) {
   if (!m_pData) {
     return 0;
@@ -841,57 +812,54 @@ FX_STRSIZE CFX_ByteString::Remove(FX_CHAR chRemove) {
   m_pData->m_nDataLength -= nCount;
   return nCount;
 }
-FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& lpszOld,
-                                   const CFX_ByteStringC& lpszNew) {
-  if (!m_pData) {
-    return 0;
-  }
-  if (lpszOld.IsEmpty()) {
+
+FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& pOld,
+                                   const CFX_ByteStringC& pNew) {
+  if (!m_pData || pOld.IsEmpty())
     return 0;
-  }
-  FX_STRSIZE nSourceLen = lpszOld.GetLength();
-  FX_STRSIZE nReplacementLen = lpszNew.GetLength();
+
+  FX_STRSIZE nSourceLen = pOld.GetLength();
+  FX_STRSIZE nReplacementLen = pNew.GetLength();
   FX_STRSIZE nCount = 0;
   const FX_CHAR* pStart = m_pData->m_String;
   FX_CHAR* pEnd = m_pData->m_String + m_pData->m_nDataLength;
   while (1) {
     const FX_CHAR* pTarget = FX_strstr(pStart, (FX_STRSIZE)(pEnd - pStart),
-                                       lpszOld.GetCStr(), nSourceLen);
-    if (!pTarget) {
+                                       pOld.GetCStr(), nSourceLen);
+    if (!pTarget)
       break;
-    }
+
     nCount++;
     pStart = pTarget + nSourceLen;
   }
-  if (nCount == 0) {
+  if (nCount == 0)
     return 0;
-  }
+
   FX_STRSIZE nNewLength =
       m_pData->m_nDataLength + (nReplacementLen - nSourceLen) * nCount;
+
   if (nNewLength == 0) {
-    Empty();
+    clear();
     return nCount;
   }
-  StringData* pNewData = StringData::Create(nNewLength);
-  if (!pNewData) {
-    return 0;
-  }
+
+  CFX_RetainPtr<StringData> pNewData(StringData::Create(nNewLength));
   pStart = m_pData->m_String;
   FX_CHAR* pDest = pNewData->m_String;
   for (FX_STRSIZE i = 0; i < nCount; i++) {
     const FX_CHAR* pTarget = FX_strstr(pStart, (FX_STRSIZE)(pEnd - pStart),
-                                       lpszOld.GetCStr(), nSourceLen);
+                                       pOld.GetCStr(), nSourceLen);
     FXSYS_memcpy(pDest, pStart, pTarget - pStart);
     pDest += pTarget - pStart;
-    FXSYS_memcpy(pDest, lpszNew.GetCStr(), lpszNew.GetLength());
-    pDest += lpszNew.GetLength();
+    FXSYS_memcpy(pDest, pNew.GetCStr(), pNew.GetLength());
+    pDest += pNew.GetLength();
     pStart = pTarget + nSourceLen;
   }
   FXSYS_memcpy(pDest, pStart, pEnd - pStart);
-  m_pData->Release();
-  m_pData = pNewData;
+  m_pData.Swap(pNewData);
   return nCount;
 }
+
 void CFX_ByteString::SetAt(FX_STRSIZE nIndex, FX_CHAR ch) {
   if (!m_pData) {
     return;
@@ -901,6 +869,7 @@ void CFX_ByteString::SetAt(FX_STRSIZE nIndex, FX_CHAR ch) {
   CopyBeforeWrite();
   m_pData->m_String[nIndex] = ch;
 }
+
 CFX_WideString CFX_ByteString::UTF8Decode() const {
   CFX_UTF8Decoder decoder;
   for (FX_STRSIZE i = 0; i < GetLength(); i++) {
@@ -944,8 +913,8 @@ int CFX_ByteString::Compare(const CFX_ByteStringC& str) const {
   }
   return 0;
 }
-void CFX_ByteString::TrimRight(const CFX_ByteStringC& lpszTargets) {
-  if (!m_pData || lpszTargets.IsEmpty()) {
+void CFX_ByteString::TrimRight(const CFX_ByteStringC& pTargets) {
+  if (!m_pData || pTargets.IsEmpty()) {
     return;
   }
   CopyBeforeWrite();
@@ -955,11 +924,11 @@ void CFX_ByteString::TrimRight(const CFX_ByteStringC& lpszTargets) {
   }
   while (pos) {
     FX_STRSIZE i = 0;
-    while (i < lpszTargets.GetLength() &&
-           lpszTargets[i] != m_pData->m_String[pos - 1]) {
+    while (i < pTargets.GetLength() &&
+           pTargets[i] != m_pData->m_String[pos - 1]) {
       i++;
     }
-    if (i == lpszTargets.GetLength()) {
+    if (i == pTargets.GetLength()) {
       break;
     }
     pos--;
@@ -975,11 +944,11 @@ void CFX_ByteString::TrimRight(FX_CHAR chTarget) {
 void CFX_ByteString::TrimRight() {
   TrimRight("\x09\x0a\x0b\x0c\x0d\x20");
 }
-void CFX_ByteString::TrimLeft(const CFX_ByteStringC& lpszTargets) {
+void CFX_ByteString::TrimLeft(const CFX_ByteStringC& pTargets) {
   if (!m_pData) {
     return;
   }
-  if (lpszTargets.IsEmpty()) {
+  if (pTargets.IsEmpty()) {
     return;
   }
   CopyBeforeWrite();
@@ -990,11 +959,10 @@ void CFX_ByteString::TrimLeft(const CFX_ByteStringC& lpszTargets) {
   FX_STRSIZE pos = 0;
   while (pos < len) {
     FX_STRSIZE i = 0;
-    while (i < lpszTargets.GetLength() &&
-           lpszTargets[i] != m_pData->m_String[pos]) {
+    while (i < pTargets.GetLength() && pTargets[i] != m_pData->m_String[pos]) {
       i++;
     }
-    if (i == lpszTargets.GetLength()) {
+    if (i == pTargets.GetLength()) {
       break;
     }
     pos++;
diff --git a/core/fxcrt/fx_basic_bstring_unittest.cpp b/core/fxcrt/fx_basic_bstring_unittest.cpp
index 834a6e444d..6e360448c7 100644
--- a/core/fxcrt/fx_basic_bstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_bstring_unittest.cpp
@@ -288,28 +288,28 @@ TEST(fxcrt, ByteStringCNull) {
   EXPECT_NE(null_string, non_null_string);
 }
 
-TEST(fxcrt, ByteStringConcatInPlace) {
+TEST(fxcrt, ByteStringConcat) {
   CFX_ByteString fred;
-  fred.ConcatInPlace(4, "FRED");
+  fred.Concat("FRED", 4);
   EXPECT_EQ("FRED", fred);
 
-  fred.ConcatInPlace(2, "DY");
+  fred.Concat("DY", 2);
   EXPECT_EQ("FREDDY", fred);
 
   fred.Delete(3, 3);
   EXPECT_EQ("FRE", fred);
 
-  fred.ConcatInPlace(1, "D");
+  fred.Concat("D", 1);
   EXPECT_EQ("FRED", fred);
 
   CFX_ByteString copy = fred;
-  fred.ConcatInPlace(2, "DY");
+  fred.Concat("DY", 2);
   EXPECT_EQ("FREDDY", fred);
   EXPECT_EQ("FRED", copy);
 
   // Test invalid arguments.
   copy = fred;
-  fred.ConcatInPlace(-6, "freddy");
+  fred.Concat("freddy", -6);
   CFX_ByteString not_aliased("xxxxxx");
   EXPECT_EQ("FREDDY", fred);
   EXPECT_EQ("xxxxxx", not_aliased);
diff --git a/core/fxcrt/include/fx_string.h b/core/fxcrt/include/fx_string.h
index 0aa34171c2..f73a1345b9 100644
--- a/core/fxcrt/include/fx_string.h
+++ b/core/fxcrt/include/fx_string.h
@@ -10,6 +10,7 @@
 #include <stdint.h>  // For intptr_t.
 #include <algorithm>
 
+#include "core/fxcrt/include/cfx_retain_ptr.h"
 #include "core/fxcrt/include/fx_memory.h"
 #include "core/fxcrt/include/fx_system.h"
 
@@ -92,11 +93,9 @@ class CFX_ByteStringC {
   uint32_t GetID(FX_STRSIZE start_pos = 0) const;
 
   const uint8_t* GetPtr() const { return m_Ptr; }
-
   const FX_CHAR* GetCStr() const { return (const FX_CHAR*)m_Ptr; }
 
   FX_STRSIZE GetLength() const { return m_Length; }
-
   bool IsEmpty() const { return m_Length == 0; }
 
   uint8_t GetAt(FX_STRSIZE index) const { return m_Ptr[index]; }
@@ -144,16 +143,9 @@ class CFX_ByteString {
  public:
   typedef FX_CHAR value_type;
 
-  CFX_ByteString() : m_pData(nullptr) {}
-
-  // Copy constructor.
-  CFX_ByteString(const CFX_ByteString& str);
-
-  // Move constructor.
-  inline CFX_ByteString(CFX_ByteString&& other) {
-    m_pData = other.m_pData;
-    other.m_pData = nullptr;
-  }
+  CFX_ByteString() {}
+  CFX_ByteString(const CFX_ByteString& other) : m_pData(other.m_pData) {}
+  CFX_ByteString(CFX_ByteString&& other) { m_pData.Swap(other.m_pData); }
 
   CFX_ByteString(char ch);
   CFX_ByteString(const FX_CHAR* ptr)
@@ -167,8 +159,11 @@ class CFX_ByteString {
 
   ~CFX_ByteString();
 
-  static CFX_ByteString FromUnicode(const FX_WCHAR* ptr, FX_STRSIZE len = -1);
+  // Deprecated -- use clear().
+  void Empty() { m_pData.Reset(); }
+  void clear() { m_pData.Reset(); }
 
+  static CFX_ByteString FromUnicode(const FX_WCHAR* ptr, FX_STRSIZE len = -1);
   static CFX_ByteString FromUnicode(const CFX_WideString& str);
 
   // Explicit conversion to C-style string.
@@ -190,7 +185,6 @@ class CFX_ByteString {
   }
 
   FX_STRSIZE GetLength() const { return m_pData ? m_pData->m_nDataLength : 0; }
-
   bool IsEmpty() const { return !GetLength(); }
 
   int Compare(const CFX_ByteStringC& str) const;
@@ -217,24 +211,16 @@ class CFX_ByteString {
     return result < 0 || (result == 0 && GetLength() < str.GetLength());
   }
 
-  void Empty();
-
   const CFX_ByteString& operator=(const FX_CHAR* str);
-
   const CFX_ByteString& operator=(const CFX_ByteStringC& bstrc);
-
   const CFX_ByteString& operator=(const CFX_ByteString& stringSrc);
-
   const CFX_ByteString& operator=(const CFX_BinaryBuf& buf);
 
   void Load(const uint8_t* str, FX_STRSIZE len);
 
   const CFX_ByteString& operator+=(FX_CHAR ch);
-
   const CFX_ByteString& operator+=(const FX_CHAR* str);
-
   const CFX_ByteString& operator+=(const CFX_ByteString& str);
-
   const CFX_ByteString& operator+=(const CFX_ByteStringC& bstrc);
 
   uint8_t GetAt(FX_STRSIZE nIndex) const {
@@ -246,49 +232,34 @@ class CFX_ByteString {
   }
 
   void SetAt(FX_STRSIZE nIndex, FX_CHAR ch);
-
   FX_STRSIZE Insert(FX_STRSIZE index, FX_CHAR ch);
-
   FX_STRSIZE Delete(FX_STRSIZE index, FX_STRSIZE count = 1);
 
   void Format(const FX_CHAR* lpszFormat, ...);
-
   void FormatV(const FX_CHAR* lpszFormat, va_list argList);
 
   void Reserve(FX_STRSIZE len);
-
   FX_CHAR* GetBuffer(FX_STRSIZE len);
-
   void ReleaseBuffer(FX_STRSIZE len = -1);
 
   CFX_ByteString Mid(FX_STRSIZE first) const;
-
   CFX_ByteString Mid(FX_STRSIZE first, FX_STRSIZE count) const;
-
   CFX_ByteString Left(FX_STRSIZE count) const;
-
   CFX_ByteString Right(FX_STRSIZE count) const;
 
   FX_STRSIZE Find(const CFX_ByteStringC& lpszSub, FX_STRSIZE start = 0) const;
-
   FX_STRSIZE Find(FX_CHAR ch, FX_STRSIZE start = 0) const;
-
   FX_STRSIZE ReverseFind(FX_CHAR ch) const;
 
   void MakeLower();
-
   void MakeUpper();
 
   void TrimRight();
-
   void TrimRight(FX_CHAR chTarget);
-
   void TrimRight(const CFX_ByteStringC& lpszTargets);
 
   void TrimLeft();
-
   void TrimLeft(FX_CHAR chTarget);
-
   void TrimLeft(const CFX_ByteStringC& lpszTargets);
 
   FX_STRSIZE Replace(const CFX_ByteStringC& lpszOld,
@@ -308,52 +279,62 @@ class CFX_ByteString {
   static CFX_ByteString FormatFloat(FX_FLOAT f, int precision = 0);
 
  protected:
-  // To ensure ref counts do not overflow, consider the worst possible case:
-  // the entire address space contains nothing but pointers to this object.
-  // Since the count increments with each new pointer, the largest value is
-  // the number of pointers that can fit into the address space. The size of
-  // the address space itself is a good upper bound on it; we need not go
-  // larger.
   class StringData {
    public:
-    static StringData* Create(int nLen);
+    static StringData* Create(FX_STRSIZE nLen);
+    static StringData* Create(const StringData& other);
+    static StringData* Create(const FX_CHAR* pStr, FX_STRSIZE nLen);
+
     void Retain() { ++m_nRefs; }
     void Release() {
       if (--m_nRefs <= 0)
         FX_Free(this);
     }
 
+    bool CanOperateInPlace(FX_STRSIZE nTotalLen) const {
+      return m_nRefs <= 1 && nTotalLen <= m_nAllocLength;
+    }
+
+    void CopyContents(const StringData& other);
+    void CopyContents(const FX_CHAR* pStr, FX_STRSIZE nLen);
+    void CopyContentsAt(FX_STRSIZE offset,
+                        const FX_CHAR* pStr,
+                        FX_STRSIZE nLen);
+
+    // To ensure ref counts do not overflow, consider the worst possible case:
+    // the entire address space contains nothing but pointers to this object.
+    // Since the count increments with each new pointer, the largest value is
+    // the number of pointers that can fit into the address space. The size of
+    // the address space itself is a good upper bound on it.
     intptr_t m_nRefs;  // Would prefer ssize_t, but no windows support.
+
+    // |FX_STRSIZE| is currently typedef'd as |int|.
+    // TODO(palmer): It should be a |size_t|, or at least unsigned.
+    // These lengths do not include the terminating NUL, but the underlying
+    // buffer is sized to be capable of holding it.
     FX_STRSIZE m_nDataLength;
     FX_STRSIZE m_nAllocLength;
+
+    // Not really 1, variable size.
     FX_CHAR m_String[1];
 
    private:
-    StringData(FX_STRSIZE dataLen, FX_STRSIZE allocLen)
-        : m_nRefs(1), m_nDataLength(dataLen), m_nAllocLength(allocLen) {
-      FXSYS_assert(dataLen >= 0);
-      FXSYS_assert(allocLen >= 0);
-      FXSYS_assert(dataLen <= allocLen);
-      m_String[dataLen] = 0;
-    }
+    StringData(FX_STRSIZE dataLen, FX_STRSIZE allocLen);
     ~StringData() = delete;
   };
 
+  void CopyBeforeWrite();
+  void AllocBeforeWrite(FX_STRSIZE nLen);
   void AllocCopy(CFX_ByteString& dest,
                  FX_STRSIZE nCopyLen,
                  FX_STRSIZE nCopyIndex) const;
-  void AssignCopy(FX_STRSIZE nSrcLen, const FX_CHAR* lpszSrcData);
-  void ConcatCopy(FX_STRSIZE nSrc1Len,
-                  const FX_CHAR* lpszSrc1Data,
-                  FX_STRSIZE nSrc2Len,
-                  const FX_CHAR* lpszSrc2Data);
-  void ConcatInPlace(FX_STRSIZE nSrcLen, const FX_CHAR* lpszSrcData);
-  void CopyBeforeWrite();
-  void AllocBeforeWrite(FX_STRSIZE nLen);
+  void AssignCopy(const FX_CHAR* pSrcData, FX_STRSIZE nSrcLen);
+  void Concat(const FX_CHAR* lpszSrcData, FX_STRSIZE nSrcLen);
 
-  StringData* m_pData;
-  friend class fxcrt_ByteStringConcatInPlace_Test;
+  CFX_RetainPtr<StringData> m_pData;
+  friend class fxcrt_ByteStringConcat_Test;
 };
+
 inline CFX_ByteStringC::CFX_ByteStringC(const CFX_ByteString& src) {
   m_Ptr = (const uint8_t*)src;
   m_Length = src.GetLength();
@@ -421,6 +402,7 @@ inline CFX_ByteString operator+(const CFX_ByteStringC& str1,
                                 const CFX_ByteString& str2) {
   return CFX_ByteString(str1, str2);
 }
+
 class CFX_WideStringC {
  public:
   typedef FX_WCHAR value_type;
@@ -478,7 +460,6 @@ class CFX_WideStringC {
   const FX_WCHAR* GetPtr() const { return m_Ptr; }
 
   FX_STRSIZE GetLength() const { return m_Length; }
-
   bool IsEmpty() const { return m_Length == 0; }
 
   FX_WCHAR GetAt(FX_STRSIZE index) const { return m_Ptr[index]; }
@@ -530,6 +511,7 @@ class CFX_WideStringC {
  private:
   void* operator new(size_t) throw() { return NULL; }
 };
+
 inline bool operator==(const wchar_t* lhs, const CFX_WideStringC& rhs) {
   return rhs == lhs;
 }
@@ -550,7 +532,7 @@ class CFX_WideString {
   CFX_WideString(const CFX_WideString& str);
 
   // Move constructor.
-  inline CFX_WideString(CFX_WideString&& other) {
+  CFX_WideString(CFX_WideString&& other) {
     m_pData = other.m_pData;
     other.m_pData = nullptr;
   }
@@ -559,22 +541,18 @@ class CFX_WideString {
       : CFX_WideString(ptr, ptr ? FXSYS_wcslen(ptr) : 0) {}
 
   CFX_WideString(const FX_WCHAR* ptr, FX_STRSIZE len);
-
   CFX_WideString(FX_WCHAR ch);
 
   CFX_WideString(const CFX_WideStringC& str);
-
   CFX_WideString(const CFX_WideStringC& str1, const CFX_WideStringC& str2);
 
   ~CFX_WideString();
 
   static CFX_WideString FromLocal(const CFX_ByteString& str);
-
   static CFX_WideString FromCodePage(const CFX_ByteString& str,
                                      uint16_t codepage);
 
   static CFX_WideString FromUTF8(const char* str, FX_STRSIZE len);
-
   static CFX_WideString FromUTF16LE(const unsigned short* str, FX_STRSIZE len);
 
   static FX_STRSIZE WStringLength(const unsigned short* str);
@@ -588,21 +566,15 @@ class CFX_WideString {
   void Empty();
 
   bool IsEmpty() const { return !GetLength(); }
-
   FX_STRSIZE GetLength() const { return m_pData ? m_pData->m_nDataLength : 0; }
 
   const CFX_WideString& operator=(const FX_WCHAR* str);
-
   const CFX_WideString& operator=(const CFX_WideString& stringSrc);
-
   const CFX_WideString& operator=(const CFX_WideStringC& stringSrc);
 
   const CFX_WideString& operator+=(const FX_WCHAR* str);
-
   const CFX_WideString& operator+=(FX_WCHAR ch);
-
   const CFX_WideString& operator+=(const CFX_WideString& str);
-
   const CFX_WideString& operator+=(const CFX_WideStringC& str);
 
   bool operator==(const wchar_t* ptr) const { return Equal(ptr); }
@@ -632,9 +604,7 @@ class CFX_WideString {
   void SetAt(FX_STRSIZE nIndex, FX_WCHAR ch);
 
   int Compare(const FX_WCHAR* str) const;
-
   int Compare(const CFX_WideString& str) const;
-
   int CompareNoCase(const FX_WCHAR* str) const;
 
   bool Equal(const wchar_t* ptr) const;
@@ -642,57 +612,40 @@ class CFX_WideString {
   bool Equal(const CFX_WideString& other) const;
 
   CFX_WideString Mid(FX_STRSIZE first) const;
-
   CFX_WideString Mid(FX_STRSIZE first, FX_STRSIZE count) const;
-
   CFX_WideString Left(FX_STRSIZE count) const;
-
   CFX_WideString Right(FX_STRSIZE count) const;
 
   FX_STRSIZE Insert(FX_STRSIZE index, FX_WCHAR ch);
-
   FX_STRSIZE Delete(FX_STRSIZE index, FX_STRSIZE count = 1);
 
   void Format(const FX_WCHAR* lpszFormat, ...);
-
   void FormatV(const FX_WCHAR* lpszFormat, va_list argList);
 
   void MakeLower();
-
   void MakeUpper();
 
   void TrimRight();
-
   void TrimRight(FX_WCHAR chTarget);
-
   void TrimRight(const FX_WCHAR* lpszTargets);
 
   void TrimLeft();
-
   void TrimLeft(FX_WCHAR chTarget);
-
   void TrimLeft(const FX_WCHAR* lpszTargets);
 
   void Reserve(FX_STRSIZE len);
-
   FX_WCHAR* GetBuffer(FX_STRSIZE len);
-
   void ReleaseBuffer(FX_STRSIZE len = -1);
 
   int GetInteger() const;
-
   FX_FLOAT GetFloat() const;
 
   FX_STRSIZE Find(const FX_WCHAR* lpszSub, FX_STRSIZE start = 0) const;
-
   FX_STRSIZE Find(FX_WCHAR ch, FX_STRSIZE start = 0) const;
-
   FX_STRSIZE Replace(const FX_WCHAR* lpszOld, const FX_WCHAR* lpszNew);
-
   FX_STRSIZE Remove(FX_WCHAR ch);
 
   CFX_ByteString UTF8Encode() const;
-
   CFX_ByteString UTF16LE_Encode() const;
 
  protected: