[Template Expressions] changing ${ leader to $i18n{

ui/base/template_expressions.cc

Index: ui/base/template_expressions.cc
diff --git a/ui/base/template_expressions.cc b/ui/base/template_expressions.cc
index 1d63e0c34eac113f386f5a80d5817faa092a5270..2a466324c6e6975c0f7a1d14d61c15dafbcfbdce 100644
--- a/ui/base/template_expressions.cc
+++ b/ui/base/template_expressions.cc
@@ -12,25 +12,26 @@ namespace ui {
 
 std::string ReplaceTemplateExpressions(
     base::StringPiece format_string,
-    const std::map<base::StringPiece, std::string>& substitutions) {
+    const TemplateReplacements& substitutions) {
+  const char kLeader[] = "$i18n{";
+  const size_t kLeaderSize = arraysize(kLeader) - 1;

[Dan Beam, 2016/01/22 00:51:44]

wait, why "- 1"?  should we just go back to strlen() instead if that's the right
number without the "- 1"?

[Dan Beam, 2016/01/22 00:51:44]

if these are invariant, should they be static or in an anonymous namespace (or
both)?

[dschuyler, 2016/01/22 01:58:58]

arraysize allows for initializing the value statically, which is nice.  The -1
is for the null terminator in the character array (double quotes always add a
null terminator in the array).

[dschuyler, 2016/01/22 01:58:58]

Done.

   std::string formatted;
   const size_t kValueLengthGuess = 16;
   formatted.reserve(format_string.length() +
                     substitutions.size() * kValueLengthGuess);
   base::StringPiece::const_iterator i = format_string.begin();
   while (i < format_string.end()) {
-    if (*i == '$' && i + 2 < format_string.end() && i[1] == '{' &&
-        i[2] != '}') {
-      size_t key_start = i + strlen("${") - format_string.begin();
+    if (base::StringPiece(i).starts_with(kLeader)) {
+      size_t key_start = i + kLeaderSize - format_string.begin();
       size_t key_length = format_string.find('}', key_start);
       if (key_length == base::StringPiece::npos)
         NOTREACHED() << "TemplateExpression missing ending brace '}'";
       key_length -= key_start;
-      base::StringPiece key(format_string.begin() + key_start, key_length);
+      std::string key(format_string.begin() + key_start, key_length);

[Dan Beam, 2016/01/22 00:51:44]

why are you changing the type of |key|?

[Dan Beam, 2016/01/22 00:51:44]

DCHECK(!key.empty());

[dschuyler, 2016/01/22 01:58:58]

In a near-future CL this code will be run in a background thread where keys are
not made from StringPiece.  StringPiece holds pointers into memory owned by
something else.  That's generally safe within a single thread and less so across
threads.  This is prep for multi-threaded use.  

[dschuyler, 2016/01/22 01:58:58]

Done.

       const auto& replacement = substitutions.find(key);
       if (replacement != substitutions.end()) {
         formatted.append(replacement->second);
-        i += strlen("${") + key_length + strlen("}");
+        i += kLeaderSize + key_length + strlen("}");
         continue;
       } else {
         NOTREACHED() << "TemplateExpression key not found: " << key;