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

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009
Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}

Committed: https://crrev.com/119857aa5b443f81ddbc8929ba510dd178b3ee69
Cr-Commit-Position: refs/heads/master@{#372239}

[dschuyler, 2016-01-16 00:38:16]

dschuyler@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-01-19 19:01:43]

I'm not a good reviewer for this. you should find more specific owners who're
familiar with the code.

[dschuyler, 2016-01-19 19:18:28]

dschuyler@chromium.org changed reviewers:
+ estade@chromium.org
- jam@chromium.org

[dschuyler, 2016-01-19 19:18:29]

Changing reviewer from jam@ to estade@.

[Evan Stade, 2016-01-19 22:04:46]

why estade and not dbeam? Looking at the bug, dbeam has more context on this
change, so if the only reason is to try to spread out the review load I think
you should instead just stick with Dan.

[dschuyler, 2016-01-19 22:10:44]

dschuyler@chromium.org changed reviewers:
+ dbeam@chromium.org
- estade@chromium.org

[dschuyler, 2016-01-19 22:10:44]

As suggested, moving reviewer from estade@ to dbeam@.

[Dan Beam, 2016-01-20 22:34:38]

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.cc:16: const char* leader = "$i18n{";
const char kLeader[] = "$i18n{";
const size_t kLeaderSize = arraysize(kLeader);

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.cc:24: size_t key_start = i + strlen(leader) -
format_string.begin();
can we cache this strlen()?

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.cc:33: i += strlen(leader) + key_length +
strlen("}");
and re-use the cached strlen here as well?

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expressions.h
File ui/base/template_expressions.h (right):

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.h:20: typedef std::map<const std::string,
std::string> ReplaceTemplateMap;
nit: I18nTemplateMap? TemplateReplacements? TemplateData?

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
File ui/base/template_expressions_unittest.cc (right):

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions_unittest.cc:17: EXPECT_EQ("blank",
ReplaceTemplateExpressions("$i18n{}", substitutions));
i still think this should never happen

[dschuyler, 2016-01-21 18:43:53]

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.cc:16: const char* leader = "$i18n{";
On 2016/01/20 22:34:38, Dan Beam wrote:
> const char kLeader[] = "$i18n{";
> const size_t kLeaderSize = arraysize(kLeader);

Done.

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.cc:24: size_t key_start = i + strlen(leader) -
format_string.begin();
On 2016/01/20 22:34:38, Dan Beam wrote:
> can we cache this strlen()?

Done.

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.cc:33: i += strlen(leader) + key_length +
strlen("}");
On 2016/01/20 22:34:38, Dan Beam wrote:
> and re-use the cached strlen here as well?

Done.

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expressions.h
File ui/base/template_expressions.h (right):

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions.h:20: typedef std::map<const std::string,
std::string> ReplaceTemplateMap;
On 2016/01/20 22:34:38, Dan Beam wrote:
> nit: I18nTemplateMap? TemplateReplacements? TemplateData?

Done.

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
File ui/base/template_expressions_unittest.cc (right):

https://codereview.chromium.org/1585923011/diff/1/ui/base/template_expression...
ui/base/template_expressions_unittest.cc:17: EXPECT_EQ("blank",
ReplaceTemplateExpressions("$i18n{}", substitutions));
On 2016/01/20 22:34:38, Dan Beam wrote:
> i still think this should never happen

I think that is reasonable - that it would/should never happen. 
At the moment the empty string is a valid string, so I want to test that case.
This is also undoing that other change where ${} was special and not translated.
Now $i18n{} is not special, it's just matching the empty string.


Acknowledged.

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

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:17: const size_t kLeaderSize =
arraysize(kLeader) - 1;
if these are invariant, should they be static or in an anonymous namespace (or
both)?

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:17: const size_t kLeaderSize =
arraysize(kLeader) - 1;
wait, why "- 1"?  should we just go back to strlen() instead if that's the right
number without the "- 1"?

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:30: std::string key(format_string.begin() +
key_start, key_length);
why are you changing the type of |key|?

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:30: std::string key(format_string.begin() +
key_start, key_length);
DCHECK(!key.empty());

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

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:17: const size_t kLeaderSize =
arraysize(kLeader) - 1;
On 2016/01/22 00:51:44, Dan Beam wrote:
> if these are invariant, should they be static or in an anonymous namespace (or
> both)?

Done.

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:17: const size_t kLeaderSize =
arraysize(kLeader) - 1;
On 2016/01/22 00:51:44, Dan Beam wrote:
> wait, why "- 1"?  should we just go back to strlen() instead if that's the
right
> number without the "- 1"?

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).

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:30: std::string key(format_string.begin() +
key_start, key_length);
On 2016/01/22 00:51:44, Dan Beam wrote:
> DCHECK(!key.empty());

Done.

https://codereview.chromium.org/1585923011/diff/20001/ui/base/template_expres...
ui/base/template_expressions.cc:30: std::string key(format_string.begin() +
key_start, key_length);
On 2016/01/22 00:51:44, Dan Beam wrote:
> why are you changing the type of |key|?

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.

[Dan Beam, 2016-01-23 03:31:55]

lgtm

https://codereview.chromium.org/1585923011/diff/40001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/1585923011/diff/40001/ui/base/template_expres...
ui/base/template_expressions.cc:15: const size_t kTailSize = arraysize(kTail) -
1;
nit: don't indent inside of namespaces

[dschuyler, 2016-01-25 22:19:11]

Just the nit change.

https://codereview.chromium.org/1585923011/diff/40001/ui/base/template_expres...
File ui/base/template_expressions.cc (right):

https://codereview.chromium.org/1585923011/diff/40001/ui/base/template_expres...
ui/base/template_expressions.cc:15: const size_t kTailSize = arraysize(kTail) -
1;
On 2016/01/23 03:31:55, Dan Beam wrote:
> nit: don't indent inside of namespaces

Done.

[dschuyler, 2016-01-26 00:32:34]

dschuyler@chromium.org changed reviewers:
+ thakis@chromium.org

[dschuyler, 2016-01-26 00:32:35]

@thakis, May I get  owner  lgtm on the files in /ui/base
  /ui/base/template_expressions.cc
  /ui/base/template_expressions.h
  /ui/base/template_expressions_unittest.cc

[Nico, 2016-01-26 01:14:09]

yuck, lgtm

[dschuyler, 2016-01-26 18:00:39]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-01-26 18:00:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1585923011/#ps60001
(title: "review nit")

[commit-bot: I haz the power, 2016-01-26 18:03:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1585923011/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1585923011/60001

[commit-bot: I haz the power, 2016-01-26 18:08:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-26 18:10:01]

Description was changed from

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009
==========

to

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}
==========

[commit-bot: I haz the power, 2016-01-26 18:10:02]

Patchset 4 (id:??) landed as
https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}

[Dan Beam, 2016-01-27 20:33:29]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1647583003/ by dbeam@chromium.org.

The reason for reverting is: broke incognito NTP and chrome://apps
https://code.google.com/p/chromium/issues/detail?id=581621.

[Dan Beam, 2016-01-27 23:42:12]

Description was changed from

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}
==========

to

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}
==========

[Dan Beam, 2016-01-27 23:43:33]

update: this didn't break chrome://apps, but dschuyler found why it broke
incognito (a new templated .css file was added around the same time this landed)

tl;dr - https://codereview.chromium.org/1646593003/ is the fix

we're gonna re-use this issue to re-land

[dschuyler, 2016-01-27 23:49:10]

This patch has the fix for the incognito window.
(An additional file needed updating).

[dschuyler, 2016-01-28 01:54:37]

I went looking for what else may have been missed.
This patch fixed the css_checker_test.py, which still
had ${foo} replacements.

[Dan Beam, 2016-01-28 22:11:02]

https://codereview.chromium.org/1585923011/diff/120001/ui/webui/resources/css...
File ui/webui/resources/css/text_defaults.css (right):

https://codereview.chromium.org/1585923011/diff/120001/ui/webui/resources/css...
ui/webui/resources/css/text_defaults.css:16: * Otherwise its $i18n{placeholders}
won't be expanded. */
fix this
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[dschuyler, 2016-01-28 22:16:15]

https://codereview.chromium.org/1585923011/diff/120001/ui/webui/resources/css...
File ui/webui/resources/css/text_defaults.css (right):

https://codereview.chromium.org/1585923011/diff/120001/ui/webui/resources/css...
ui/webui/resources/css/text_defaults.css:16: * Otherwise its $i18n{placeholders}
won't be expanded. */
On 2016/01/28 22:11:01, Dan Beam wrote:
> fix this
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

Done.

[Dan Beam, 2016-01-28 23:06:53]

lgtm

[dschuyler, 2016-01-29 01:14:56]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-01-29 01:14:56]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1585923011/#ps140001
(title: "fix for $i18n comment")

[commit-bot: I haz the power, 2016-01-29 01:15:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1585923011/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1585923011/140001

[commit-bot: I haz the power, 2016-01-29 01:22:29]

Description was changed from

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}
==========

to

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}
==========

[commit-bot: I haz the power, 2016-01-29 01:22:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-29 01:23:17]

Description was changed from

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}
==========

to

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

This CL changes the Template Expressions from using
a ${ to identify the start of an expression, to using
$i18n{.  The intention is to reduce (eliminate) false
positives when searching for expressions that need
replacement.  This CL also prepares for the replacement
work to be done in a background thread and used on a
broader set of files.

BUG=506009

Committed: https://crrev.com/5aa7fd5f14e49c65ebba87e4248424d622b92930
Cr-Commit-Position: refs/heads/master@{#371537}

Committed: https://crrev.com/119857aa5b443f81ddbc8929ba510dd178b3ee69
Cr-Commit-Position: refs/heads/master@{#372239}
==========

[commit-bot: I haz the power, 2016-01-29 01:23:19]

Patchset 8 (id:??) landed as
https://crrev.com/119857aa5b443f81ddbc8929ba510dd178b3ee69
Cr-Commit-Position: refs/heads/master@{#372239}

[pinguinad86, 2016-06-06 15:28:09]

pinguinad86@gmail.com changed reviewers:
+ pinguinad86@gmail.com

[pinguinad86, 2016-06-06 15:28:10]

lgtm

[pinguinad86, 2016-06-06 15:28:15]

lgtm

[pinguinad86, 2016-06-06 15:28:15]

lgtm