Include chromium version number in policy templates

Add a chromium version to policy template files.

BUG=108941
TEST=python test_suite_all.py

[cschuet (SLOW), 2014-10-07 13:51:35]

cschuet@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[pastarmovj, 2014-10-07 14:54:36]

A few comments ;)

It's good as a whole just style stuff.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writer_configuration.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writer_configuration.py:55: if('version' in
defines):
nit: No brackets and a space after if. 

Note: nit means small change no major blocker.
Note2: You can see all the style requirements in the respective language style
guides.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adm_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adm_writer.py:212:
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
nit: Line too long. Split it. 80chars max. No need for triple ''' on a single
line literals.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adm_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adm_writer_unittest.py:94: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'}, 'adm', 'en')
nit: Line too long.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adm_writer_unittest.py:95: expected_output
= '''; chromium version: 39.0.0.0\n''' + self.ConstructOutput(
Ditto.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adml_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adml_writer.py:165:
self.AddComment(self._doc.documentElement, self.config['build'] + ''' version:
''' + self._GetChromiumVersionString())
Line too long.
You don't need triple ''' for one line string literals.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adml_writer.py:166: #
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
Why is this line here? If you don't need it remove it.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adml_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adml_writer_unittest.py:80: def
testEmptyVersion(self):
Actually I think a better name for this test will be testVersionAnnotation or
some such.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/admx_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/admx_writer.py:353:
self.AddComment(self._doc.documentElement, self.config['build'] + ''' version:
''' + self._GetChromiumVersionString())
Line too long.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/admx_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/admx_writer_unittest.py:41: 'build':
'test',
Why is this change done?

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/doc_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/doc_writer.py:594:
self.AddComment(self._main_div, self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
Line too long and no need for triple quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/doc_writer_unittest.py:42: 'build': 'test',
Again here. I guess the explanation is the same as on the previous instance.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/doc_writer_unittest.py:107: def
testSkeletonVersion(self):
I'd keep the name of this test consistent over all file types.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/ios_plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/ios_plist_writer.py:101:
self.AddComment(self._root_dict, ''' ''' + self.config['build'] + ''' version:
''' + self._GetChromiumVersionString() + ''' ''')
Line too long; triple quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/ios_plist_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/ios_plist_writer_unittest.py:60: _defines =
{ '_chromium': '1', 'mac_bundle_id': 'com.example.Test' }
I'd simplify that and make the test always take the define you need - e.g. add
it to the _defines list.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/json_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/json_writer.py:43: self._out.append('''//
''' + comment)
No need for triple quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/json_writer.py:76:
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
line too long, quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/json_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/json_writer_unittest.py:79: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'}, 'json',
'en')
Line too long.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/plist_strings_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_strings_writer.py:26:
self._out.append('''<!-- ''' + comment + ''' -->''' )
triple quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_strings_writer.py:70:
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
line length, quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/plist_strings_writer_unittest.py
(right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_strings_writer_unittest.py:62:
{'_chromium': '1', 'mac_bundle_id': 'com.example.Test', 'version': '39.0.0.0'},
line too long.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_writer.py:124:
self.AddComment(self._plist, self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
line too long, quotes.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/reg_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/reg_writer.py:110: prefix.append('''; ''' +
self.config['build'] + ''' version: ''' + self._GetChromiumVersionString())
same.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/reg_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/reg_writer_unittest.py:59: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version': '39.0.0.0', }, 'reg',
'en')
same.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/template_writer.py:110: Returns: The
Chromium version string or an empty string if it has not been set.
too long.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/template_writer.py:112: if('version' in
self.config):
space after if, no braces.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/template_writer.py:205: '''Appends the
comment to the internal buffer.
something is fishy with those lines - too many whitespace?

[cschuet1, 2014-10-07 15:42:44]

cschuet@google.com changed reviewers:
+ cschuet@google.com

[cschuet1, 2014-10-07 15:42:47]

addressed all you concerns

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writer_configuration.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writer_configuration.py:55: if('version' in
defines):
On 2014/10/07 14:54:35, pastarmovj wrote:
> nit: No brackets and a space after if. 
> 
> Note: nit means small change no major blocker.
> Note2: You can see all the style requirements in the respective language style
> guides.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adm_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adm_writer.py:212:
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 14:54:35, pastarmovj wrote:
> nit: Line too long. Split it. 80chars max. No need for triple ''' on a single
> line literals.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adm_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adm_writer_unittest.py:94: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'}, 'adm', 'en')
On 2014/10/07 14:54:35, pastarmovj wrote:
> nit: Line too long.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adm_writer_unittest.py:95: expected_output
= '''; chromium version: 39.0.0.0\n''' + self.ConstructOutput(
On 2014/10/07 14:54:35, pastarmovj wrote:
> Ditto.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adml_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adml_writer.py:165:
self.AddComment(self._doc.documentElement, self.config['build'] + ''' version:
''' + self._GetChromiumVersionString())
On 2014/10/07 14:54:35, pastarmovj wrote:
> Line too long.
> You don't need triple ''' for one line string literals.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adml_writer.py:166: #
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 14:54:35, pastarmovj wrote:
> Why is this line here? If you don't need it remove it.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/adml_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/adml_writer_unittest.py:80: def
testEmptyVersion(self):
On 2014/10/07 14:54:35, pastarmovj wrote:
> Actually I think a better name for this test will be testVersionAnnotation or
> some such.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/admx_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/admx_writer.py:353:
self.AddComment(self._doc.documentElement, self.config['build'] + ''' version:
''' + self._GetChromiumVersionString())
On 2014/10/07 14:54:35, pastarmovj wrote:
> Line too long.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/admx_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/admx_writer_unittest.py:41: 'build':
'test',
On 2014/10/07 14:54:35, pastarmovj wrote:
> Why is this change done?
_GetChromiumVersionString() in TemplateWriter needs this define in order to
build the ChromiumVersionString.

test renamed to test_product

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/doc_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/doc_writer.py:594:
self.AddComment(self._main_div, self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 14:54:35, pastarmovj wrote:
> Line too long and no need for triple quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/doc_writer_unittest.py:42: 'build': 'test',
On 2014/10/07 14:54:35, pastarmovj wrote:
> Again here. I guess the explanation is the same as on the previous instance.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/doc_writer_unittest.py:107: def
testSkeletonVersion(self):
On 2014/10/07 14:54:35, pastarmovj wrote:
> I'd keep the name of this test consistent over all file types.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/ios_plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/ios_plist_writer.py:101:
self.AddComment(self._root_dict, ''' ''' + self.config['build'] + ''' version:
''' + self._GetChromiumVersionString() + ''' ''')
On 2014/10/07 14:54:35, pastarmovj wrote:
> Line too long; triple quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/ios_plist_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/ios_plist_writer_unittest.py:60: _defines =
{ '_chromium': '1', 'mac_bundle_id': 'com.example.Test' }
On 2014/10/07 14:54:35, pastarmovj wrote:
> I'd simplify that and make the test always take the define you need - e.g. add
> it to the _defines list.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/json_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/json_writer.py:43: self._out.append('''//
''' + comment)
On 2014/10/07 14:54:35, pastarmovj wrote:
> No need for triple quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/json_writer.py:76:
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 14:54:35, pastarmovj wrote:
> line too long, quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/json_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/json_writer_unittest.py:79: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'}, 'json',
'en')
On 2014/10/07 14:54:36, pastarmovj wrote:
> Line too long.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/plist_strings_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_strings_writer.py:26:
self._out.append('''<!-- ''' + comment + ''' -->''' )
On 2014/10/07 14:54:36, pastarmovj wrote:
> triple quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_strings_writer.py:70:
self.WriteComment(self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 14:54:36, pastarmovj wrote:
> line length, quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/plist_strings_writer_unittest.py
(right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_strings_writer_unittest.py:62:
{'_chromium': '1', 'mac_bundle_id': 'com.example.Test', 'version': '39.0.0.0'},
On 2014/10/07 14:54:36, pastarmovj wrote:
> line too long.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/plist_writer.py:124:
self.AddComment(self._plist, self.config['build'] + ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 14:54:36, pastarmovj wrote:
> line too long, quotes.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/reg_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/reg_writer.py:110: prefix.append('''; ''' +
self.config['build'] + ''' version: ''' + self._GetChromiumVersionString())
On 2014/10/07 14:54:36, pastarmovj wrote:
> same.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/reg_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/reg_writer_unittest.py:59: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version': '39.0.0.0', }, 'reg',
'en')
On 2014/10/07 14:54:36, pastarmovj wrote:
> same.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/template_writer.py:110: Returns: The
Chromium version string or an empty string if it has not been set.
On 2014/10/07 14:54:36, pastarmovj wrote:
> too long.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/template_writer.py:112: if('version' in
self.config):
On 2014/10/07 14:54:36, pastarmovj wrote:
> space after if, no braces.

Done.

https://codereview.chromium.org/631223003/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/template_writer.py:205: '''Appends the
comment to the internal buffer.
On 2014/10/07 14:54:36, pastarmovj wrote:
> something is fishy with those lines - too many whitespace?

Done.

[pastarmovj, 2014-10-07 15:47:17]

almost there :)

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/adm_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/adm_writer_unittest.py:94: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'},
Move all params to the new line.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/adm_writer_unittest.py:96: expected_output
= '; chromium version: 39.0.0.0\n' +\
A space after +.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/adml_writer.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/adml_writer.py:165:
self.AddComment(self._doc.documentElement, self.config['build'] +
End the line with a \

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/admx_writer.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/admx_writer.py:353:
self.AddComment(self._doc.documentElement, self.config['build'] +\
space after +

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/admx_writer.py:354: ''' version: ''' +
self._GetChromiumVersionString())
triple ''' still here.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer.py:594:
self.AddComment(self._main_div, self.config['build'] + ' version: ' +\
space after + (you'll need to split on the previous plus i think).

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/json_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/json_writer_unittest.py:79: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'},
Move them all to the next line.

[cschuet1, 2014-10-07 16:00:24]

try again :)

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/adm_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/adm_writer_unittest.py:94: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'},
On 2014/10/07 15:47:16, pastarmovj wrote:
> Move all params to the new line.

Done.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/adm_writer_unittest.py:96: expected_output
= '; chromium version: 39.0.0.0\n' +\
On 2014/10/07 15:47:16, pastarmovj wrote:
> A space after +.

Done.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/adml_writer.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/adml_writer.py:165:
self.AddComment(self._doc.documentElement, self.config['build'] +
On 2014/10/07 15:47:16, pastarmovj wrote:
> End the line with a \

Done.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/admx_writer.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/admx_writer.py:353:
self.AddComment(self._doc.documentElement, self.config['build'] +\
On 2014/10/07 15:47:16, pastarmovj wrote:
> space after +

Done.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/admx_writer.py:354: ''' version: ''' +
self._GetChromiumVersionString())
On 2014/10/07 15:47:16, pastarmovj wrote:
> triple ''' still here.

Done.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer.py:594:
self.AddComment(self._main_div, self.config['build'] + ' version: ' +\
On 2014/10/07 15:47:16, pastarmovj wrote:
> space after + (you'll need to split on the previous plus i think).

Done.

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
File grit/format/policy_templates/writers/json_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/20001/grit/format/policy_templ...
grit/format/policy_templates/writers/json_writer_unittest.py:79: output =
self.GetOutput(grd, 'fr', {'_chromium': '1', 'version':'39.0.0.0'},
On 2014/10/07 15:47:16, pastarmovj wrote:
> Move them all to the next line.

Done.

[pastarmovj, 2014-10-10 07:50:38]

pastarmovj@chromium.org changed reviewers:
+ joaodasilva@chromium.org

[pastarmovj, 2014-10-10 07:50:39]

Hey Joao do you mind having a look too before we land this.

[Joao da Silva, 2014-10-13 10:30:58]

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/ios_plist_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/ios_plist_writer_unittest.py:58:
defines=None):
This doesn't seem to be used?

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/plist_strings_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/plist_strings_writer.py:26:
self._out.append('<!-- ' + comment + ' -->' )
Note that this file isn't XML. See the apple docs at

https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/Loadin...

for the specification; basically comments are written like this:

/* comment line */

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/plist_writer.py:125:
self._GetChromiumVersionString())
Can you add a unittest for this writer too?

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/reg_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/reg_writer.py:111:
self._GetChromiumVersionString())
Add a WriteComment to override the default impl in the parent class, for
consistency

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:6: import os
This isn't used

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:110: Returns: The
Chromium version string or an empty string if it has not
Note that python functions return None by default, so this won't return an empty
string if the version is not set

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:206: '''Appends the
comment to the internal buffer.
Remove trailing whitespaces from this line.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:208: comment: The
comment to be added.
From this one too

[cschuet (SLOW), 2014-10-14 18:55:21]

addressed all issues

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/ios_plist_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/ios_plist_writer_unittest.py:58:
defines=None):
On 2014/10/13 10:30:58, Joao da Silva wrote:
> This doesn't seem to be used?

Done. These were the remains of a more elaborate test mechanism.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/plist_strings_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/plist_strings_writer.py:26:
self._out.append('<!-- ' + comment + ' -->' )
On 2014/10/13 10:30:58, Joao da Silva wrote:
> Note that this file isn't XML. See the apple docs at
> 
>
https://developer.apple.com/library/ios/documentation/Cocoa/Conceptual/Loadin...
> 
> for the specification; basically comments are written like this:
> 
> /* comment line */

Done. Good catch!

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/plist_writer.py:125:
self._GetChromiumVersionString())
On 2014/10/13 10:30:58, Joao da Silva wrote:
> Can you add a unittest for this writer too?

Done.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/reg_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/reg_writer.py:111:
self._GetChromiumVersionString())
On 2014/10/13 10:30:58, Joao da Silva wrote:
> Add a WriteComment to override the default impl in the parent class, for
> consistency

Done.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:6: import os
On 2014/10/13 10:30:58, Joao da Silva wrote:
> This isn't used

Done.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:110: Returns: The
Chromium version string or an empty string if it has not
On 2014/10/13 10:30:58, Joao da Silva wrote:
> Note that python functions return None by default, so this won't return an
empty
> string if the version is not set
This was the way I originally implemented this. But I switched to returning None
for clarity. Updated the documetation comment.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:206: '''Appends the
comment to the internal buffer.
On 2014/10/13 10:30:58, Joao da Silva wrote:
> Remove trailing whitespaces from this line.

Done.

https://codereview.chromium.org/631223003/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/template_writer.py:208: comment: The
comment to be added.
On 2014/10/13 10:30:58, Joao da Silva wrote:
> From this one too

Done.

[Joao da Silva, 2014-10-14 19:05:21]

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
File grit/format/policy_templates/writers/plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer.py:125:
self._GetChromiumVersionString())
Maybe you forgot to add the unittest for this writer during the last upload?

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
File grit/format/policy_templates/writers/reg_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/reg_writer_unittest.py:62: 'Windows
Registry Editor Version 5.00\r\n')
The .reg file format isn't very well specified. These 2 links have some
pointers:

http://support.microsoft.com/kb/310516
http://en.wikipedia.org/wiki/Windows_Registry#.REG_files

Both mention that a .reg file is validated by the contents of the first line,
which must be this magic string. So any comments should come after, to make sure
every windows version will happily consume the file.

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/template_writer.py:109: Returns: The
Chromium version string or an empty string if it has not
or None if it has not been set

[cschuet (SLOW), 2014-10-14 21:04:54]

fixed the reg-file writer and added the forgotten unit test

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
File grit/format/policy_templates/writers/plist_writer.py (right):

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer.py:125:
self._GetChromiumVersionString())
On 2014/10/14 19:05:21, Joao da Silva wrote:
> Maybe you forgot to add the unittest for this writer during the last upload?

Done. Right forgot to add the unit test. Thanks.

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
File grit/format/policy_templates/writers/reg_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/reg_writer_unittest.py:62: 'Windows
Registry Editor Version 5.00\r\n')
On 2014/10/14 19:05:21, Joao da Silva wrote:
> The .reg file format isn't very well specified. These 2 links have some
> pointers:
> 
> http://support.microsoft.com/kb/310516
> http://en.wikipedia.org/wiki/Windows_Registry#.REG_files
> 
> Both mention that a .reg file is validated by the contents of the first line,
> which must be this magic string. So any comments should come after, to make
sure
> every windows version will happily consume the file.

Done.

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/reg_writer_unittest.py:62: 'Windows
Registry Editor Version 5.00\r\n')
On 2014/10/14 19:05:21, Joao da Silva wrote:
> The .reg file format isn't very well specified. These 2 links have some
> pointers:
> 
> http://support.microsoft.com/kb/310516
> http://en.wikipedia.org/wiki/Windows_Registry#.REG_files
> 
> Both mention that a .reg file is validated by the contents of the first line,
> which must be this magic string. So any comments should come after, to make
sure
> every windows version will happily consume the file.

Done.

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
File grit/format/policy_templates/writers/template_writer.py (right):

https://codereview.chromium.org/631223003/diff/140001/grit/format/policy_temp...
grit/format/policy_templates/writers/template_writer.py:109: Returns: The
Chromium version string or an empty string if it has not
On 2014/10/14 19:05:21, Joao da Silva wrote:
> or None if it has not been set

Done.

[Joao da Silva, 2014-10-15 10:46:46]

lgtm

Seems like plist_writer_unittest.py is still missing :-)

[cschuet (SLOW), 2014-10-15 12:42:33]

finally added the missing file. thanks @Joao da Silva

[Joao da Silva, 2014-10-15 13:04:37]

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
File grit/format/policy_templates/writers/plist_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:55: def
_GetExpectedOutputsWithVersion(self, product_name, bundle_id, policies,
version):
break lines longer than 80 columns

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:90: 
one newline is enough

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:97: 'messages':
{},
keep the indentation that this had before

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:128: 'Chromium',
'com.example.Test', '<array/>', 'chromium version: 39.0.0.0')
seems like this line is longer than 80 columns too

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:132: 
remove these newlines

[cschuet (SLOW), 2014-10-15 15:23:14]

ok, thanks. I should be paying more attention to the formatting...

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
File grit/format/policy_templates/writers/plist_writer_unittest.py (right):

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:55: def
_GetExpectedOutputsWithVersion(self, product_name, bundle_id, policies,
version):
On 2014/10/15 13:04:37, Joao da Silva wrote:
> break lines longer than 80 columns

Done.

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:90: 
On 2014/10/15 13:04:37, Joao da Silva wrote:
> one newline is enough

Done.

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:97: 'messages':
{},
On 2014/10/15 13:04:37, Joao da Silva wrote:
> keep the indentation that this had before

Done.

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:128: 'Chromium',
'com.example.Test', '<array/>', 'chromium version: 39.0.0.0')
On 2014/10/15 13:04:37, Joao da Silva wrote:
> seems like this line is longer than 80 columns too

Done.

https://codereview.chromium.org/631223003/diff/180001/grit/format/policy_temp...
grit/format/policy_templates/writers/plist_writer_unittest.py:132: 
On 2014/10/15 13:04:37, Joao da Silva wrote:
> remove these newlines

Done.

[Joao da Silva, 2014-10-15 15:28:36]

lgtm!

Thanks for bearing with me. Yep, formatting is important (even if not always
consistent and up to the bar in existing code).

This is ready to land from my side. You can try to commit with gcl but it will
probably fail because you're not a committer of this project. If it doesn't work
then I or Julian can get this landed for you.

[Joao da Silva, 2014-10-16 12:03:26]

This has been landed as https://code.google.com/p/grit-i18n/source/detail?r=178.

Christoph, you can now close this CL manually (the CQ does this for you for
chromium CLs, but not for others). The next step is to update the chromium
src/DEPS file to make it use grit revision 178 or later.