Fix placeholder syntax for Chrome extensions.

Fix placeholder syntax for Chrome extensions.

This changes the output format for placeholders in Chrome extensions from $n (which is ambiguous in some circumstances) to $var$. For simplicity (and to keep the file size small), |var| is always |n|; in other words, we use placeholders named $1$, $2$, etc., regardless of the name given in the GRD file.

BUG=551100
r202

[Jamie, 2015-11-04 03:10:27]

Description was changed from

==========
Fix placeholder syntax for Chrome extensions.

BUG=551100
==========

to

==========
Fix placeholder syntax for Chrome extensions.

This changes the output format for placeholders in Chrome extensions from $n
(which is ambiguous in some circumstances) to $var$. For simplicity (and to keep
the file size small), |var| is always |n|; in other words, we use placeholders
named $1$, $2$, etc., regardless of the name given in the GRD file.

BUG=551100
==========

[Jamie, 2015-11-04 03:13:46]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-11-04 03:13:46]

ptal

[kelvinp, 2015-11-04 19:02:10]

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
File grit/format/chrome_messages_json.py (right):

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:1: #!/usr/bin/env python
Will this affect other extensions in Chrome?

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:24: placeholder_format = ('    "%i": {\n'
According to the documentation, the placeholder is optional.  I wonder if there
is much value in generating placeholder that only have

1: {
  content: "$1
}

There may be more value in parsing the <ph> tag and fill in the example value.

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:40: for i in range(1, 10):
Is 10 the maximum number of placeholders?
Nevermind, found the information on
https://developer.chrome.com/extensions/i18n#method-getMessage
There can only be 9 substitution strings.

[kelvinp, 2015-11-04 19:09:04]

lgtm once the last comment is addressed.

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
File grit/format/chrome_messages_json.py (right):

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:1: #!/usr/bin/env python
On 2015/11/04 19:02:10, kelvinp wrote:
> Will this affect other extensions in Chrome?

Spoke to Jamie, looks like the UT passes so the other parties shouldn't e
affected.

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:24: placeholder_format = ('    "%i": {\n'
On 2015/11/04 19:02:10, kelvinp wrote:
> According to the documentation, the placeholder is optional.  I wonder if
there
> is much value in generating placeholder that only have
> 
> 1: {
>   content: "$1
> }
> 
> There may be more value in parsing the <ph> tag and fill in the example value.

Spoke with Jamie offline.  We want to minimize text in an output file.

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:40: for i in range(1, 10):
On 2015/11/04 19:02:10, kelvinp wrote:
> Is 10 the maximum number of placeholders?
> Nevermind, found the information on
> https://developer.chrome.com/extensions/i18n#method-getMessage
> There can only be 9 substitution strings.
May be good to include the link about there can only be 9 parameters in the
comment.

[Jamie, 2015-11-04 19:13:55]

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
File grit/format/chrome_messages_json.py (right):

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:1: #!/usr/bin/env python
On 2015/11/04 19:02:10, kelvinp wrote:
> Will this affect other extensions in Chrome?

Yes. It will affect them all, but the net effect should be zero--we're just
substituting one syntax for placeholder declaration for another.

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:24: placeholder_format = ('    "%i": {\n'
On 2015/11/04 19:02:10, kelvinp wrote:
> According to the documentation, the placeholder is optional.  I wonder if
there
> is much value in generating placeholder that only have
> 
> 1: {
>   content: "$1
> }

It's only optional insofar as you don't need it for messages with no
placeholders that use the $name$ syntax. Since we are using that syntax, we need
it.

> 
> There may be more value in parsing the <ph> tag and fill in the example value.

I considered that, but since the messages.json files are output files (ie, not
intended to be human-readable), there's not a lot of value in copying that
information across. It would also make the code more complex.

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages...
grit/format/chrome_messages_json.py:40: for i in range(1, 10):
On 2015/11/04 19:02:10, kelvinp wrote:
> Is 10 the maximum number of placeholders?
> Nevermind, found the information on
> https://developer.chrome.com/extensions/i18n#method-getMessage
> There can only be 9 substitution strings.

Acknowledged.

[Nico, 2015-11-04 21:04:07]

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

[Nico, 2015-11-04 21:04:07]

Is it possible to write a test for this? (run `python grit.py unit` to run the
existing tests, or you can use the usual python unittest way to run individual
tests too iirc)

[Jamie, 2015-11-04 21:24:21]

On 2015/11/04 21:04:07, Nico wrote:
> Is it possible to write a test for this? (run `python grit.py unit` to run the
> existing tests, or you can use the usual python unittest way to run individual
> tests too iirc)

Done (and it found a couple of formatting errors, thanks!)

[Nico, 2015-11-04 21:31:08]

landed in grit-i18n r202

[Nico, 2015-11-04 21:31:16]

Description was changed from

==========
Fix placeholder syntax for Chrome extensions.

This changes the output format for placeholders in Chrome extensions from $n
(which is ambiguous in some circumstances) to $var$. For simplicity (and to keep
the file size small), |var| is always |n|; in other words, we use placeholders
named $1$, $2$, etc., regardless of the name given in the GRD file.

BUG=551100
==========

to

==========
Fix placeholder syntax for Chrome extensions.

This changes the output format for placeholders in Chrome extensions from $n
(which is ambiguous in some circumstances) to $var$. For simplicity (and to keep
the file size small), |var| is always |n|; in other words, we use placeholders
named $1$, $2$, etc., regardless of the name given in the GRD file.

BUG=551100
r202
==========