Avoid copying in TrimRight() and TrimLeft() if possible.

Make Byte and Wide code identical while at it.

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

5 files changed, 145 insertions, 18 deletions

diff --git a/core/fxcrt/fx_basic_bstring.cpp b/core/fxcrt/fx_basic_bstring.cpp
index 5b9c683c53..4a48539578 100644
--- a/core/fxcrt/fx_basic_bstring.cpp
+++ b/core/fxcrt/fx_basic_bstring.cpp
@@ -841,7 +841,6 @@ void CFX_ByteString::TrimRight(const CFX_ByteStringC& pTargets) {
   if (!m_pData || pTargets.IsEmpty()) {
     return;
   }
-  ReallocBeforeWrite(m_pData->m_nDataLength);
   FX_STRSIZE pos = GetLength();
   if (pos < 1) {
     return;
@@ -858,6 +857,7 @@ void CFX_ByteString::TrimRight(const CFX_ByteStringC& pTargets) {
     pos--;
   }
   if (pos < m_pData->m_nDataLength) {
+    ReallocBeforeWrite(m_pData->m_nDataLength);
     m_pData->m_String[pos] = 0;
     m_pData->m_nDataLength = pos;
   }
@@ -879,7 +879,6 @@ void CFX_ByteString::TrimLeft(const CFX_ByteStringC& pTargets) {
   if (len < 1)
     return;
 
-  ReallocBeforeWrite(len);
   FX_STRSIZE pos = 0;
   while (pos < len) {
     FX_STRSIZE i = 0;
@@ -892,6 +891,7 @@ void CFX_ByteString::TrimLeft(const CFX_ByteStringC& pTargets) {
     pos++;
   }
   if (pos) {
+    ReallocBeforeWrite(len);
     FX_STRSIZE nDataLength = len - pos;
     FXSYS_memmove(m_pData->m_String, m_pData->m_String + pos,
                   (nDataLength + 1) * sizeof(FX_CHAR));
diff --git a/core/fxcrt/fx_basic_bstring_unittest.cpp b/core/fxcrt/fx_basic_bstring_unittest.cpp
index 73235ddf5f..8c72b2a054 100644
--- a/core/fxcrt/fx_basic_bstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_bstring_unittest.cpp
@@ -505,6 +505,37 @@ TEST(fxcrt, ByteStringTrimRight) {
   EXPECT_EQ("", empty);
 }
 
+TEST(fxcrt, ByteStringTrimRightCopies) {
+  {
+    // With a single reference, no copy takes place.
+    CFX_ByteString fred("  FRED  ");
+    const FX_CHAR* old_buffer = fred.c_str();
+    fred.TrimRight();
+    EXPECT_EQ("  FRED", fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, we must copy.
+    CFX_ByteString fred("  FRED  ");
+    CFX_ByteString other_fred = fred;
+    const FX_CHAR* old_buffer = fred.c_str();
+    fred.TrimRight();
+    EXPECT_EQ("  FRED", fred);
+    EXPECT_EQ("  FRED  ", other_fred);
+    EXPECT_NE(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, but no modifications, no copy.
+    CFX_ByteString fred("FRED");
+    CFX_ByteString other_fred = fred;
+    const FX_CHAR* old_buffer = fred.c_str();
+    fred.TrimRight();
+    EXPECT_EQ("FRED", fred);
+    EXPECT_EQ("FRED", other_fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+}
+
 TEST(fxcrt, ByteStringTrimLeft) {
   CFX_ByteString fred("  FRED  ");
   fred.TrimLeft();
@@ -533,6 +564,37 @@ TEST(fxcrt, ByteStringTrimLeft) {
   EXPECT_EQ("", empty);
 }
 
+TEST(fxcrt, ByteStringTrimLeftCopies) {
+  {
+    // With a single reference, no copy takes place.
+    CFX_ByteString fred("  FRED  ");
+    const FX_CHAR* old_buffer = fred.c_str();
+    fred.TrimLeft();
+    EXPECT_EQ("FRED  ", fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, we must copy.
+    CFX_ByteString fred("  FRED  ");
+    CFX_ByteString other_fred = fred;
+    const FX_CHAR* old_buffer = fred.c_str();
+    fred.TrimLeft();
+    EXPECT_EQ("FRED  ", fred);
+    EXPECT_EQ("  FRED  ", other_fred);
+    EXPECT_NE(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, but no modifications, no copy.
+    CFX_ByteString fred("FRED");
+    CFX_ByteString other_fred = fred;
+    const FX_CHAR* old_buffer = fred.c_str();
+    fred.TrimLeft();
+    EXPECT_EQ("FRED", fred);
+    EXPECT_EQ("FRED", other_fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+}
+
 TEST(fxcrt, ByteStringCNotNull) {
   CFX_ByteStringC string3("abc");
   CFX_ByteStringC string6("abcdef");
diff --git a/core/fxcrt/fx_basic_wstring.cpp b/core/fxcrt/fx_basic_wstring.cpp
index b5ccaf6da5..08a9d212d4 100644
--- a/core/fxcrt/fx_basic_wstring.cpp
+++ b/core/fxcrt/fx_basic_wstring.cpp
@@ -840,7 +840,6 @@ void CFX_WideString::TrimRight(const CFX_WideStringC& pTargets) {
   if (!m_pData || pTargets.IsEmpty()) {
     return;
   }
-  ReallocBeforeWrite(m_pData->m_nDataLength);
   FX_STRSIZE pos = GetLength();
   if (pos < 1) {
     return;
@@ -852,6 +851,7 @@ void CFX_WideString::TrimRight(const CFX_WideStringC& pTargets) {
     pos--;
   }
   if (pos < m_pData->m_nDataLength) {
+    ReallocBeforeWrite(m_pData->m_nDataLength);
     m_pData->m_String[pos] = 0;
     m_pData->m_nDataLength = pos;
   }
@@ -874,18 +874,21 @@ void CFX_WideString::TrimLeft(const CFX_WideStringC& pTargets) {
   if (len < 1)
     return;
 
-  ReallocBeforeWrite(len);
-  const FX_WCHAR* lpsz = m_pData->m_String;
-  while (*lpsz != 0) {
-    if (!FXSYS_wcschr(pTargets.c_str(), *lpsz)) {
+  FX_STRSIZE pos = 0;
+  while (pos < len) {
+    FX_STRSIZE i = 0;
+    while (i < pTargets.GetLength() && pTargets[i] != m_pData->m_String[pos]) {
+      i++;
+    }
+    if (i == pTargets.GetLength()) {
       break;
     }
-    lpsz++;
+    pos++;
   }
-  if (lpsz != m_pData->m_String) {
-    int nDataLength =
-        m_pData->m_nDataLength - (FX_STRSIZE)(lpsz - m_pData->m_String);
-    FXSYS_memmove(m_pData->m_String, lpsz,
+  if (pos) {
+    ReallocBeforeWrite(len);
+    FX_STRSIZE nDataLength = len - pos;
+    FXSYS_memmove(m_pData->m_String, m_pData->m_String + pos,
                   (nDataLength + 1) * sizeof(FX_WCHAR));
     m_pData->m_nDataLength = nDataLength;
   }
diff --git a/core/fxcrt/fx_basic_wstring_unittest.cpp b/core/fxcrt/fx_basic_wstring_unittest.cpp
index cfc190f72a..bc38141a95 100644
--- a/core/fxcrt/fx_basic_wstring_unittest.cpp
+++ b/core/fxcrt/fx_basic_wstring_unittest.cpp
@@ -472,6 +472,37 @@ TEST(fxcrt, WideStringTrimRight) {
   EXPECT_EQ(L"", empty);
 }
 
+TEST(fxcrt, WideStringTrimRightCopies) {
+  {
+    // With a single reference, no copy takes place.
+    CFX_WideString fred(L"  FRED  ");
+    const FX_WCHAR* old_buffer = fred.c_str();
+    fred.TrimRight();
+    EXPECT_EQ(L"  FRED", fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, we must copy.
+    CFX_WideString fred(L"  FRED  ");
+    CFX_WideString other_fred = fred;
+    const FX_WCHAR* old_buffer = fred.c_str();
+    fred.TrimRight();
+    EXPECT_EQ(L"  FRED", fred);
+    EXPECT_EQ(L"  FRED  ", other_fred);
+    EXPECT_NE(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, but no modifications, no copy.
+    CFX_WideString fred(L"FRED");
+    CFX_WideString other_fred = fred;
+    const FX_WCHAR* old_buffer = fred.c_str();
+    fred.TrimRight();
+    EXPECT_EQ(L"FRED", fred);
+    EXPECT_EQ(L"FRED", other_fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+}
+
 TEST(fxcrt, WideStringTrimLeft) {
   CFX_WideString fred(L"  FRED  ");
   fred.TrimLeft();
@@ -500,6 +531,37 @@ TEST(fxcrt, WideStringTrimLeft) {
   EXPECT_EQ(L"", empty);
 }
 
+TEST(fxcrt, WideStringTrimLeftCopies) {
+  {
+    // With a single reference, no copy takes place.
+    CFX_WideString fred(L"  FRED  ");
+    const FX_WCHAR* old_buffer = fred.c_str();
+    fred.TrimLeft();
+    EXPECT_EQ(L"FRED  ", fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, we must copy.
+    CFX_WideString fred(L"  FRED  ");
+    CFX_WideString other_fred = fred;
+    const FX_WCHAR* old_buffer = fred.c_str();
+    fred.TrimLeft();
+    EXPECT_EQ(L"FRED  ", fred);
+    EXPECT_EQ(L"  FRED  ", other_fred);
+    EXPECT_NE(old_buffer, fred.c_str());
+  }
+  {
+    // With multiple references, but no modifications, no copy.
+    CFX_WideString fred(L"FRED");
+    CFX_WideString other_fred = fred;
+    const FX_WCHAR* old_buffer = fred.c_str();
+    fred.TrimLeft();
+    EXPECT_EQ(L"FRED", fred);
+    EXPECT_EQ(L"FRED", other_fred);
+    EXPECT_EQ(old_buffer, fred.c_str());
+  }
+}
+
 TEST(fxcrt, WideStringUTF16LE_Encode) {
   struct UTF16LEEncodeCase {
     CFX_WideString ws;
diff --git a/core/fxcrt/include/fx_string.h b/core/fxcrt/include/fx_string.h
index 0e3160a9d8..ef0dfee6fb 100644
--- a/core/fxcrt/include/fx_string.h
+++ b/core/fxcrt/include/fx_string.h
@@ -167,21 +167,21 @@ class CFX_ByteString {
   static CFX_ByteString FromUnicode(const CFX_WideString& str);
 
   // Explicit conversion to C-style string.
-  // Note: |this| must outlive the use of the result.
+  // Note: Any subsequent modification of |this| will invalidate the result.
   const FX_CHAR* c_str() const { return m_pData ? m_pData->m_String : ""; }
 
   // Implicit conversion to C-style string -- deprecated.
   operator const FX_CHAR*() const { return m_pData ? m_pData->m_String : ""; }
 
   // Explicit conversion to uint8_t*.
-  // Note: |this| must outlive the use of the result.
+  // Note: Any subsequent modification of |this| will invalidate the result.
   const uint8_t* raw_str() const {
     return m_pData ? reinterpret_cast<const uint8_t*>(m_pData->m_String)
                    : nullptr;
   }
 
   // Explicit conversion to CFX_ByteStringC.
-  // Note: |this| must outlive the use of the result.
+  // Note: Any subsequent modification of |this| will invalidate the result.
   CFX_ByteStringC AsStringC() const {
     return CFX_ByteStringC(raw_str(), GetLength());
   }
@@ -497,15 +497,15 @@ class CFX_WideString {
   static FX_STRSIZE WStringLength(const unsigned short* str);
 
   // Explicit conversion to C-style wide string.
-  // Note: |this| must outlive the use of the result.
+  // Note: Any subsequent modification of |this| will invalidate the result.
   const FX_WCHAR* c_str() const { return m_pData ? m_pData->m_String : L""; }
 
   // Implicit conversion to C-style wide string -- deprecated.
-  // Note: |this| must outlive the use of the result.
+  // Note: Any subsequent modification of |this| will invalidate the result.
   operator const FX_WCHAR*() const { return m_pData ? m_pData->m_String : L""; }
 
   // Explicit conversion to CFX_WideStringC.
-  // Note: |this| must outlive the use of the result.
+  // Note: Any subsequent modification of |this| will invalidate the result.
   CFX_WideStringC AsStringC() const {
     return CFX_WideStringC(c_str(), GetLength());
   }