Integrate user strings into the JSON policy template file

Integrate user strings into the JSON policy template file

The new concept is the following: policy_templates.json contains all the data necessary to generate templates: policy names, English captions, descriptions, etc. This gets translated the following GRIT gatherer: policy_json.py. The text of the translated JSON file is then picked up, parsed, and passed to the writers by template_formatter.py.

BUG=64898
TEST=PolicyJsonUnittest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71421

[gfeher, 2011-01-10 20:49:30]

Please review.
Jói: please look at the interfacing with GRIT
Jakon: please look at the rest

[Jói, 2011-01-10 21:16:50]

I may be misunderstanding some things, please see this first round of comments.
I didn't look at any of the files under the policy_templates folder.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
File tools/grit/grit/gather/policy_json.py (right):

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:82:
self.skeleton_.append(self.uberclique.MakeClique(msg))
Don't you get into problems with JSON escaping when you output the file? Seems
you would at least need to override the Escape method from RegexpGatherer.

I don't mind which approach you take to this, but did you think about using an
existing JSON writer implementation and not using the
translatable/non-translatable skeleton idiom that RegexpGatherer uses (instead
you would just keep a list of messages and other metadata and then output it as
a JSON file)?

Do you use the .json file that calls to the Translate function will generate at
all? Perhaps this is the case since you have all those other writer classes in
the policy_templates folder that get invoked as output formats via <output>
nodes, but if you don't use it, then maybe you should provide an overridden
Translate method that just returns the empty string, to save some time, and
again if you don't use the output .json, there is no need for you to keep all
those non-translatable chunks.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:217: self._AddNontranslateableChunk("     
'%s': {\n" % name)
use two space indent

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:230: # Although we use the RegexpGatherer
base class, we do not use the
This is confusing, since the inheritance implies an is-a relationship.

It would probably be better to split out a common base class from RegexpGatherer
(and have RegexpGatherer and your class inherit from it), named
SkeletonGatherer, that leaves the Parse() method unimplemented but takes the
rest of the stuff that's not really regular expression stuff and moves it to the
base.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:234: if (self.have_parsed_):
looks like you are forgetting to set self.have_parsed_ below

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/node/struct...
File tools/grit/grit/node/structure.py (right):

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/node/struct...
tools/grit/grit/node/structure.py:18: import grit.gather.json_loader
can remove this import now

[Jakob Kummerow, 2011-01-11 11:11:24]

the policy_templates/ folder LGTM.

nits, as discussed offline:
• you didn't update the copyright header ("2010" -> "2011"), but that's not
officially required anyway.
• not directly related to this CL: we should clean up the #!/usr/bin/python2.4
header lines some time. They're both outdated and inconsistent: in some files
they're present, in others not.

[gfeher, 2011-01-11 20:09:11]

I've addressed a subset of comments.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
File tools/grit/grit/gather/policy_json.py (right):

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:82:
self.skeleton_.append(self.uberclique.MakeClique(msg))
On 2011/01/10 21:16:50, Jói wrote:
> Don't you get into problems with JSON escaping when you output the file? Seems
> you would at least need to override the Escape method from RegexpGatherer.
> 
The output seems fine. I've diffed all the outputted templates against their
versions generated on trunk. However, I will override Escape once we had agreed
on the general approach.

> I don't mind which approach you take to this, but did you think about using an
> existing JSON writer implementation and not using the
> translatable/non-translatable skeleton idiom that RegexpGatherer uses (instead
> you would just keep a list of messages and other metadata and then output it
as
> a JSON file)?
> 
If you mean policy_templates/json_writer.py, then that is a different thing. In
short, it outputs a very-very stripped down version of policy_templates.json.
Otherwise please clarify.

> Do you use the .json file that calls to the Translate function will generate
at
> all? Perhaps this is the case since you have all those other writer classes in
> the policy_templates folder that get invoked as output formats via <output>
> nodes, but if you don't use it, then maybe you should provide an overridden
> Translate method that just returns the empty string, to save some time, and
> again if you don't use the output .json, there is no need for you to keep all
> those non-translatable chunks.

Yes, I do use the output of Translate. It goes through template_formatter.py and
goes to the writers. So it is not the structure node that writes the output
files, but the output nodes. I wanted to utilize the structure node to create
these files (as you had recommended earlier), but it seemed like that there is
no such possibility in open source GRIT.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:217: self._AddNontranslateableChunk("     
'%s': {\n" % name)
On 2011/01/10 21:16:50, Jói wrote:
> use two space indent

Done.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:230: # Although we use the RegexpGatherer
base class, we do not use the
On 2011/01/10 21:16:50, Jói wrote:
> This is confusing, since the inheritance implies an is-a relationship.

I agree.

> It would probably be better to split out a common base class from
RegexpGatherer
> (and have RegexpGatherer and your class inherit from it), named
> SkeletonGatherer, that leaves the Parse() method unimplemented but takes the
> rest of the stuff that's not really regular expression stuff and moves it to
the
> base.

Would it also make sense to reimplement that stuff here? (Please see my e-mail.)

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:234: if (self.have_parsed_):
On 2011/01/10 21:16:50, Jói wrote:
> looks like you are forgetting to set self.have_parsed_ below

Done. This was copied from muppet_strings.py, should I fix that as well?

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/node/struct...
File tools/grit/grit/node/structure.py (right):

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/node/struct...
tools/grit/grit/node/structure.py:18: import grit.gather.json_loader
On 2011/01/10 21:16:50, Jói wrote:
> can remove this import now

Done.

[Jói, 2011-01-11 20:24:57]

Overall approach seems fine to me, see some responses to your comments below.

Cheers,
Jói

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
File tools/grit/grit/gather/policy_json.py (right):

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:82:
self.skeleton_.append(self.uberclique.MakeClique(msg))
On 2011/01/11 20:09:11, gfeher wrote:
> On 2011/01/10 21:16:50, Jói wrote:
> > Don't you get into problems with JSON escaping when you output the file?
Seems
> > you would at least need to override the Escape method from RegexpGatherer.
> > 
> The output seems fine. I've diffed all the outputted templates against their
> versions generated on trunk. However, I will override Escape once we had
agreed
> on the general approach.
> 
> > I don't mind which approach you take to this, but did you think about using
an
> > existing JSON writer implementation and not using the
> > translatable/non-translatable skeleton idiom that RegexpGatherer uses
(instead
> > you would just keep a list of messages and other metadata and then output it
> as
> > a JSON file)?
> > 
> If you mean policy_templates/json_writer.py, then that is a different thing.
In
> short, it outputs a very-very stripped down version of policy_templates.json.
> Otherwise please clarify.
> 
> > Do you use the .json file that calls to the Translate function will generate
> at
> > all? Perhaps this is the case since you have all those other writer classes
in
> > the policy_templates folder that get invoked as output formats via <output>
> > nodes, but if you don't use it, then maybe you should provide an overridden
> > Translate method that just returns the empty string, to save some time, and
> > again if you don't use the output .json, there is no need for you to keep
all
> > those non-translatable chunks.
> 
> Yes, I do use the output of Translate. It goes through template_formatter.py
and
> goes to the writers. So it is not the structure node that writes the output
> files, but the output nodes. I wanted to utilize the structure node to create
> these files (as you had recommended earlier), but it seemed like that there is
> no such possibility in open source GRIT.

OK.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:230: # Although we use the RegexpGatherer
base class, we do not use the
On 2011/01/11 20:09:11, gfeher wrote:
> On 2011/01/10 21:16:50, Jói wrote:
> > This is confusing, since the inheritance implies an is-a relationship.
> 
> I agree.
> 
> > It would probably be better to split out a common base class from
> RegexpGatherer
> > (and have RegexpGatherer and your class inherit from it), named
> > SkeletonGatherer, that leaves the Parse() method unimplemented but takes the
> > rest of the stuff that's not really regular expression stuff and moves it to
> the
> > base.
> 
> Would it also make sense to reimplement that stuff here? (Please see my
e-mail.)

I would recommend extracting the base class as discussed.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:234: if (self.have_parsed_):
On 2011/01/11 20:09:11, gfeher wrote:
> On 2011/01/10 21:16:50, Jói wrote:
> > looks like you are forgetting to set self.have_parsed_ below
> 
> Done. This was copied from muppet_strings.py, should I fix that as well?

Yes please.

[gfeher, 2011-01-12 12:18:32]

Please take another look.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
File tools/grit/grit/gather/policy_json.py (right):

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:82:
self.skeleton_.append(self.uberclique.MakeClique(msg))
I probably won't need to override Escape.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:230: # Although we use the RegexpGatherer
base class, we do not use the
On 2011/01/11 20:24:57, Jói wrote:
> On 2011/01/11 20:09:11, gfeher wrote:
> > On 2011/01/10 21:16:50, Jói wrote:
> > > This is confusing, since the inheritance implies an is-a relationship.
> > 
> > I agree.
> > 
> > > It would probably be better to split out a common base class from
> > RegexpGatherer
> > > (and have RegexpGatherer and your class inherit from it), named
> > > SkeletonGatherer, that leaves the Parse() method unimplemented but takes
the
> > > rest of the stuff that's not really regular expression stuff and moves it
to
> > the
> > > base.
> > 
> > Would it also make sense to reimplement that stuff here? (Please see my
> e-mail.)
> 
> I would recommend extracting the base class as discussed.

Done.

http://codereview.chromium.org/6134006/diff/14001/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:234: if (self.have_parsed_):
On 2011/01/11 20:24:57, Jói wrote:
> On 2011/01/11 20:09:11, gfeher wrote:
> > On 2011/01/10 21:16:50, Jói wrote:
> > > looks like you are forgetting to set self.have_parsed_ below
> > 
> > Done. This was copied from muppet_strings.py, should I fix that as well?
> 
> Yes please.

Done.

[Jói, 2011-01-12 14:45:59]

Thanks for the cleanup.

Two things:
a) I'm assuming all [ grit unit ] unit tests pass after the refactoring.
b) Don't you still need to override Escape() in policy_json.py ?

Cheers,
Jói

http://codereview.chromium.org/6134006/diff/33001/tools/grit/grit/gather/skel...
File tools/grit/grit/gather/skeleton_gatherer.py (right):

http://codereview.chromium.org/6134006/diff/33001/tools/grit/grit/gather/skel...
tools/grit/grit/gather/skeleton_gatherer.py:25: # List of parts of the document.
Translateable parts are clique.MessageClique
+80

[gfeher, 2011-01-12 18:27:23]

> a) I'm assuming all [ grit unit ] unit tests pass after the refactoring.
Ooops, good that you ask! There were three failures already, but this CL changes
nothing. I've filed http://crbug.com/69405 to track this. 

Note: I've removed the template generator's tests from test_suite_all.py because
now they are invoked from a presubmit script.

> b) Don't you still need to override Escape() in policy_json.py ?
I've done double and triple checks: I've found&fixed one escaping problem that
happened when parsing the JSON file, before GRIT has any say. Otherwise, I have
no such problems. If you tell me what kind of escaping-problems you are having
in mind, I might be able to prove why my current code is free from those.

http://codereview.chromium.org/6134006/diff/33001/tools/grit/grit/gather/skel...
File tools/grit/grit/gather/skeleton_gatherer.py (right):

http://codereview.chromium.org/6134006/diff/33001/tools/grit/grit/gather/skel...
tools/grit/grit/gather/skeleton_gatherer.py:25: # List of parts of the document.
Translateable parts are clique.MessageClique
On 2011/01/12 14:45:59, Jói wrote:
> +80

Done.

[Jói, 2011-01-12 18:49:58]

> If you tell me what kind of escaping-problems you are having
> in mind, I might be able to prove why my current code is free from those.

Presumably, if one of the messages had a quote in it, that would cause
the output .json file you create to have an extra quote and thus be
unparseable?  See e.g.
http://bytes.com/topic/javascript/answers/478176-escaping-special-characters-...

Your current data may not have this problem, I'm just suggesting that
you handle escaping to avoid such problems in the future.

Cheers,
Jói



On Wed, Jan 12, 2011 at 1:27 PM,  <gfeher@chromium.org> wrote:
>> a) I'm assuming all [ grit unit ] unit tests pass after the refactoring.
>
> Ooops, good that you ask! There were three failures already, but this CL
> changes
> nothing. I've filed http://crbug.com/69405 to track this.
>
> Note: I've removed the template generator's tests from test_suite_all.py
> because
> now they are invoked from a presubmit script.
>
>> b) Don't you still need to override Escape() in policy_json.py ?
>
> I've done double and triple checks: I've found&fixed one escaping problem
> that
> happened when parsing the JSON file, before GRIT has any say. Otherwise, I
> have
> no such problems. If you tell me what kind of escaping-problems you are
> having
> in mind, I might be able to prove why my current code is free from those.
>
>
>
>
>
http://codereview.chromium.org/6134006/diff/33001/tools/grit/grit/gather/skel...
> File tools/grit/grit/gather/skeleton_gatherer.py (right):
>
>
http://codereview.chromium.org/6134006/diff/33001/tools/grit/grit/gather/skel...
> tools/grit/grit/gather/skeleton_gatherer.py:25: # List of parts of the
> document. Translateable parts are clique.MessageClique
> On 2011/01/12 14:45:59, Jói wrote:
>>
>> +80
>
> Done.
>
> http://codereview.chromium.org/6134006/
>

[gfeher, 2011-01-13 12:15:30]

I've fixed escaping and added tests. Please take a look.

[Jói, 2011-01-13 18:54:31]

LGTM

http://codereview.chromium.org/6134006/diff/84003/tools/grit/grit/gather/poli...
File tools/grit/grit/gather/policy_json.py (right):

http://codereview.chromium.org/6134006/diff/84003/tools/grit/grit/gather/poli...
tools/grit/grit/gather/policy_json.py:254: 
just one blank line between members