Fix placeholder syntax for Chrome extensions. (issue 1425693007 by jamiewalch@chromium.org)

[jamie...@chromium.org]

Reviewers: kelvinp
CL: https://codereview.chromium.org/1425693007/

Message:
ptal

Description:
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

Base URL: https://chromium.googlesource.com/external/grit-i18n.git@master

Affected files (+19, -2 lines):
M grit/format/chrome_messages_json.py


Index: grit/format/chrome_messages_json.py
diff --git a/grit/format/chrome_messages_json.py
b/grit/format/chrome_messages_json.py
index
be934ab1175924657a79796dbf0def4c2464ec5f..7cd0e28b98d9bb6dfa24355c7a901c42531deb4e
100644
--- a/grit/format/chrome_messages_json.py
+++ b/grit/format/chrome_messages_json.py
@@ -19,8 +19,11 @@ def Format(root, lang='en', output_dir='.'):

encoder = JSONEncoder();
format = (' "%s": {\n'
- ' "message": %s\n'
+ ' "message": %s\n%s'
' }')
+ placeholder_format = (' "%i": {\n'
+ ' "content": "$%i"\n'
+ ' }')
first = True
for child in root.ActiveDescendants():
if isinstance(child, message.MessageNode):
@@ -31,9 +34,23 @@ def Format(root, lang='en', output_dir='.'):
loc_message = encoder.encode(child.ws_at_start +
child.Translate(lang) +
child.ws_at_end)

+ # Replace $n place-holders with $n$ and add an
appropriate "placeholders"
+ # entry.
+ placeholders = ''
+ for i in range(1, 10):
+ if loc_message.find('$%d' % i) == -1:
+ break
+ loc_message = loc_message.replace('$%d' % i, '$%d$' % i)
+ if placeholders:
+ placeholders += ',\n'
+ placeholders += placeholder_format % (i, i)
+
if not first:
yield ',\n'
first = False
- yield format % (id, loc_message)
+
+ if placeholders:
+ placeholders = ',\n "placeholders": {\n%s\n }' % placeholders
+ yield format % (id, loc_message, placeholders)

yield '\n}\n'

[kel...@chromium.org]

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

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages_json.py#newcode1
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_json.py#newcode24
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_json.py#newcode40
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.

https://codereview.chromium.org/1425693007/

[kel...@chromium.org]

lgtm once the last comment is addressed.

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

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages_json.py#newcode1
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_json.py#newcode24
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_json.py#newcode40
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.

https://codereview.chromium.org/1425693007/

[jamie...@chromium.org]

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

https://codereview.chromium.org/1425693007/diff/1/grit/format/chrome_messages_json.py#newcode1
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_json.py#newcode24
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_json.py#newcode40
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.

https://codereview.chromium.org/1425693007/

[tha...@chromium.org]

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)

https://codereview.chromium.org/1425693007/

[jamie...@chromium.org]

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

https://codereview.chromium.org/1425693007/

[tha...@chromium.org]

landed in grit-i18n r202

https://codereview.chromium.org/1425693007/