Change ReplaceStringPlaceholders() to allow only up to 9 placeholders.

Initially ReplaceStringPlaceholder() was written to allow not more than
9 placeholders, which means it would only parse a single digit after $.
For example $11 was treated as a placeholder followed by a digit. The
behavior of that function was changed in crrev.com/88190. After that
change it allows more than 9 placeholders, but no longer supports
placeholders followed by a digit. Documentation for that function
in the header wasn't updated as part of that change, so it still
states that there can be no more than 9 placeholders.

ReplaceStringPlaceholder() is never used with more than 9 place
holder. Extension localization API and Chrome's internal localization
library both limit number of placeholders, see
I18NCustomBindings::GetL10nMessage() and l10n_util::GetStringFUTF16().
So it's safe to revert to the old behavior.

Also fixed the code to handle invalid placeholders explicitly and
added a unittest for that case.

BUG=168720

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

Cr-Commit-Position: refs/heads/master@{#387534}

base/strings/string_util.cc

@@ -888,6 +888,7 @@ OutStringType DoReplaceStringPlaceholders(
     const std::vector<OutStringType>& subst,
     std::vector<size_t>* offsets) {
   size_t substitutions = subst.size();
+  DCHECK_LT(substitutions, 10U);
 
   size_t sub_length = 0;
   for (const auto& cur : subst)
@@ -901,22 +902,18 @@ OutStringType DoReplaceStringPlaceholders(
     if ('$' == *i) {
       if (i + 1 != format_string.end()) {
         ++i;
-        DCHECK('$' == *i || '1' <= *i) << "Invalid placeholder: " << *i;
         if ('$' == *i) {
           while (i != format_string.end() && '$' == *i) {
             formatted.push_back('$');
             ++i;
           }
           --i;
         } else {
-          uintptr_t index = 0;
-          while (i != format_string.end() && '0' <= *i && *i <= '9') {
-            index *= 10;
-            index += *i - '0';
-            ++i;
+          if (*i < '1' || *i > '9') {
+            DLOG(ERROR) << "Invalid placeholder: $" << *i;
+            continue;
           }
-          --i;
-          index -= 1;
+          uintptr_t index = *i - '1';
           if (offsets) {
             ReplacementOffset r_offset(index,
                 static_cast<int>(formatted.size()));

base/strings/string_util.h

@@ -437,7 +437,7 @@ BASE_EXPORT std::string JoinString(const std::vector<std::string>& parts,
 BASE_EXPORT string16 JoinString(const std::vector<string16>& parts,
                                 StringPiece16 separator);
 
-// Replace $1-$2-$3..$9 in the format string with |a|-|b|-|c|..|i| respectively.
+// Replace $1-$2-$3..$9 in the format string with values from |subst|.
 // Additionally, any number of consecutive '$' characters is replaced by that
 // number less one. Eg $$->$, $$$->$$, etc. The offsets parameter here can be
 // NULL. This only allows you to use up to nine replacements.

base/strings/string_util_unittest.cc

@@ -820,9 +820,9 @@ TEST(StringUtilTest, ReplaceStringPlaceholdersTooFew) {
 
   string16 formatted =
       ReplaceStringPlaceholders(
-          ASCIIToUTF16("$1a,$2b,$3c,$4d,$5e,$6f,$1g,$2h,$3i"), subst, NULL);
+          ASCIIToUTF16("$1a,$2b,$3c,$4d,$5e,$6f,$1g,$2h,$3i"), subst, nullptr);
 
-  EXPECT_EQ(formatted, ASCIIToUTF16("9aa,8bb,7cc,d,e,f,9ag,8bh,7ci"));
+  EXPECT_EQ(ASCIIToUTF16("9aa,8bb,7cc,d,e,f,9ag,8bh,7ci"), formatted);
 }
 
 TEST(StringUtilTest, ReplaceStringPlaceholders) {
@@ -839,35 +839,25 @@ TEST(StringUtilTest, ReplaceStringPlaceholders) {
 
   string16 formatted =
       ReplaceStringPlaceholders(
-          ASCIIToUTF16("$1a,$2b,$3c,$4d,$5e,$6f,$7g,$8h,$9i"), subst, NULL);
+          ASCIIToUTF16("$1a,$2b,$3c,$4d,$5e,$6f,$7g,$8h,$9i"), subst, nullptr);
 
-  EXPECT_EQ(formatted, ASCIIToUTF16("9aa,8bb,7cc,6dd,5ee,4ff,3gg,2hh,1ii"));
+  EXPECT_EQ(ASCIIToUTF16("9aa,8bb,7cc,6dd,5ee,4ff,3gg,2hh,1ii"), formatted);
 }
 
-TEST(StringUtilTest, ReplaceStringPlaceholdersMoreThan9Replacements) {
+TEST(StringUtilTest, ReplaceStringPlaceholdersOneDigit) {
   std::vector<string16> subst;
-  subst.push_back(ASCIIToUTF16("9a"));
-  subst.push_back(ASCIIToUTF16("8b"));
-  subst.push_back(ASCIIToUTF16("7c"));
-  subst.push_back(ASCIIToUTF16("6d"));
-  subst.push_back(ASCIIToUTF16("5e"));
-  subst.push_back(ASCIIToUTF16("4f"));
-  subst.push_back(ASCIIToUTF16("3g"));
-  subst.push_back(ASCIIToUTF16("2h"));
-  subst.push_back(ASCIIToUTF16("1i"));
-  subst.push_back(ASCIIToUTF16("0j"));
-  subst.push_back(ASCIIToUTF16("-1k"));
-  subst.push_back(ASCIIToUTF16("-2l"));
-  subst.push_back(ASCIIToUTF16("-3m"));
-  subst.push_back(ASCIIToUTF16("-4n"));
-
+  subst.push_back(ASCIIToUTF16("1a"));
   string16 formatted =
-      ReplaceStringPlaceholders(
-          ASCIIToUTF16("$1a,$2b,$3c,$4d,$5e,$6f,$7g,$8h,$9i,"
-                       "$10j,$11k,$12l,$13m,$14n,$1"), subst, NULL);
+      ReplaceStringPlaceholders(ASCIIToUTF16(" $16 "), subst, nullptr);
+  EXPECT_EQ(ASCIIToUTF16(" 1a6 "), formatted);
+}
 
-  EXPECT_EQ(formatted, ASCIIToUTF16("9aa,8bb,7cc,6dd,5ee,4ff,3gg,2hh,"
-                                    "1ii,0jj,-1kk,-2ll,-3mm,-4nn,9a"));
+TEST(StringUtilTest, ReplaceStringPlaceholdersInvalidPlaceholder) {
+  std::vector<string16> subst;
+  subst.push_back(ASCIIToUTF16("1a"));
+  string16 formatted =
+      ReplaceStringPlaceholders(ASCIIToUTF16("+$-+$A+$1+"), subst, nullptr);
+  EXPECT_EQ(ASCIIToUTF16("+++1a+"), formatted);
 }
 
 TEST(StringUtilTest, StdStringReplaceStringPlaceholders) {
@@ -884,17 +874,17 @@ TEST(StringUtilTest, StdStringReplaceStringPlaceholders) {
 
   std::string formatted =
       ReplaceStringPlaceholders(
-          "$1a,$2b,$3c,$4d,$5e,$6f,$7g,$8h,$9i", subst, NULL);
+          "$1a,$2b,$3c,$4d,$5e,$6f,$7g,$8h,$9i", subst, nullptr);
 
-  EXPECT_EQ(formatted, "9aa,8bb,7cc,6dd,5ee,4ff,3gg,2hh,1ii");
+  EXPECT_EQ("9aa,8bb,7cc,6dd,5ee,4ff,3gg,2hh,1ii", formatted);
 }
 
 TEST(StringUtilTest, ReplaceStringPlaceholdersConsecutiveDollarSigns) {
   std::vector<std::string> subst;
   subst.push_back("a");
   subst.push_back("b");
   subst.push_back("c");
-  EXPECT_EQ(ReplaceStringPlaceholders("$$1 $$$2 $$$$3", subst, NULL),
+  EXPECT_EQ(ReplaceStringPlaceholders("$$1 $$$2 $$$$3", subst, nullptr),
             "$1 $$2 $$$3");
 }