Add a Policy template writer that generates Android resources which can be exposed through Android'…

Add a Policy template writer that generates Android resources which can be exposed through Android's App Restriction Schema.

BUG=450548

[knn, 2015-01-21 11:23:30]

knn@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2015-01-21 13:55:55]

Very nice!

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

https://codereview.chromium.org/865563002/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/android_policy_writer.py:8: import
xml.sax.saxutils as xml_escape
Is there an order to the imports? Can we sort them alphabetically by source
maybe? (In which case xml.dom would come first)

https://codereview.chromium.org/865563002/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/android_policy_writer.py:79: if '-enum' in
policy['type']:
I think we can just check whether policy['items'] exists?

https://codereview.chromium.org/865563002/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/android_policy_writer.py:88: 'IT IS
GENERATED FROM policy_templates.json.'
Add one more space to align.

[knn, 2015-01-21 14:38:02]

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

https://codereview.chromium.org/865563002/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/android_policy_writer.py:8: import
xml.sax.saxutils as xml_escape
On 2015/01/21 13:55:55, Bernhard Bauer wrote:
> Is there an order to the imports? Can we sort them alphabetically by source
> maybe? (In which case xml.dom would come first)

Done.

https://codereview.chromium.org/865563002/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/android_policy_writer.py:79: if '-enum' in
policy['type']:
On 2015/01/21 13:55:55, Bernhard Bauer wrote:
> I think we can just check whether policy['items'] exists?

Done.

https://codereview.chromium.org/865563002/diff/1/grit/format/policy_templates...
grit/format/policy_templates/writers/android_policy_writer.py:88: 'IT IS
GENERATED FROM policy_templates.json.'
On 2015/01/21 13:55:55, Bernhard Bauer wrote:
> Add one more space to align.

Done.

[Bernhard Bauer, 2015-01-21 15:48:11]

lgtm

[knn, 2015-01-21 16:55:58]

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

[knn, 2015-01-21 16:55:58]

+Nico for owners review.

[Bernhard Bauer, 2015-02-09 13:02:45]

Nico, friendly ping?

[Nico, 2015-02-09 15:28:36]

Whoops, sorry, did't see this the first time round. Please ping sooner :-(

Anyways, is it possible to test this? (`./grit.py unit` to run all tests)

Also, does the bug have to be private?

[knn, 2015-02-24 20:53:33]

On 2015/02/09 15:28:36, Nico wrote:
> Whoops, sorry, did't see this the first time round. Please ping sooner :-(
> 
> Anyways, is it possible to test this? (`./grit.py unit` to run all tests)
> 
> Also, does the bug have to be private?

Sorry for the delay Nico, Can you please have a look at the change?
I have added some unittests. Also, I did not realize I had some unsubmitted
changes suggested from here (https://codereview.chromium.org/865573002/).

[Nico, 2015-03-02 19:02:06]

lgtm if bauerb likes the tests. A bunch of style notes below.

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

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:13: '''Factory
method for creating AndroidPolicyWriter objects.
Style guide says use """ for doc strings (but ' for other strings). Also, no
newline before the closing """ in general.

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:18: 
nit: python style guide says two newlines between toplevel statements

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:34: _doc = None
Instance variables are usually set in __init__ in python (e.g. because of
http://stackoverflow.com/questions/867219/python-class-members-initialization
but also other reasons)


Also, it looks like grit names members doc_ not _doc

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:92: def
Init(self):
What's wrong with doing this in __init__?

(Looks like other places in grit also have an Init() method, but I don't
understand why.)

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

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:20: class
AndroidPolicyWriterUnittest(writer_unittest_common.WriterUnittestCommon):
nit: two newlines

[knn, 2015-03-02 19:54:35]

Hi Nico,
Some of the changes you suggested encompasses other files in the directory. For
some reason it has become inconsistent with the Style Guide.
Should I make those changes as a separate CL?

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

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:13: '''Factory
method for creating AndroidPolicyWriter objects.
On 2015/03/02 19:02:06, Nico wrote:
> Style guide says use """ for doc strings (but ' for other strings). Also, no
> newline before the closing """ in general.

Acknowledged. Will have to update other files in this directory for consistency
though.

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:18: 
On 2015/03/02 19:02:06, Nico wrote:
> nit: python style guide says two newlines between toplevel statements

Done.

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:34: _doc = None
On 2015/03/02 19:02:06, Nico wrote:
> Instance variables are usually set in __init__ in python (e.g. because of
> http://stackoverflow.com/questions/867219/python-class-members-initialization
> but also other reasons)
> 
> 
> Also, it looks like grit names members doc_ not _doc

Other files in this directory are using this convention as well. Will have to
change all of them.

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer.py:92: def
Init(self):
On 2015/03/02 19:02:06, Nico wrote:
> What's wrong with doing this in __init__?
> 
> (Looks like other places in grit also have an Init() method, but I don't
> understand why.)

Acknowledged. Will update other files as well.

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

https://codereview.chromium.org/865563002/diff/40001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:20: class
AndroidPolicyWriterUnittest(writer_unittest_common.WriterUnittestCommon):
On 2015/03/02 19:02:06, Nico wrote:
> nit: two newlines

Done.

[Bernhard Bauer, 2015-03-03 10:52:15]

Just some formatting nits for the unit tests:

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
File grit/format/policy_templates/writers/android_policy_writer_unittest.py
(right):

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:29:
'desc': 'This is a long policy caption. More than one sentence in a single line
because it is very important.\nIgnore this.'
Break this line? (Concatenate the strings without adding newlines.)

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:40: '
More than one sentence in a single line because it is very important.</string>'
And this line.

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:70:
'<item>_caption1</item>'
Can you indent these?

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer_unittest.py:58:
'doc_android_restriction_name': {'text': '_test_android_restriction_name'},
Break this line.

[knn, 2015-03-03 16:09:52]

Done!

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
File grit/format/policy_templates/writers/android_policy_writer_unittest.py
(right):

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:29:
'desc': 'This is a long policy caption. More than one sentence in a single line
because it is very important.\nIgnore this.'
On 2015/03/03 10:52:15, Bernhard Bauer wrote:
> Break this line? (Concatenate the strings without adding newlines.)

Done.

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:40: '
More than one sentence in a single line because it is very important.</string>'
On 2015/03/03 10:52:15, Bernhard Bauer wrote:
> And this line.

Done.

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/android_policy_writer_unittest.py:70:
'<item>_caption1</item>'
On 2015/03/03 10:52:15, Bernhard Bauer wrote:
> Can you indent these?

Done.

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/865563002/diff/60001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer_unittest.py:58:
'doc_android_restriction_name': {'text': '_test_android_restriction_name'},
On 2015/03/03 10:52:15, Bernhard Bauer wrote:
> Break this line.

Done.

[Bernhard Bauer, 2015-03-03 16:23:58]

LGTM!

[Nico, 2015-03-04 18:50:11]

I tried landing this, and when I ran `python grit.py unit` before committing,
the new test didn't pass:

======================================================================
FAIL: testAddPolicySectionForAndroidOnly
(grit.format.policy_templates.writers.doc_writer_unittest.DocWriterUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File
"/Volumes/MacintoshHD2/src/grit-i18n/grit/format/policy_templates/writers/doc_writer_unittest.py",
line 838, in testAddPolicySectionForAndroidOnly
    '<root>'
AssertionError: '<root><div style="margin-left: 0px"><h3><a
name="PolicyName"/>PolicyName</h3><span>PolicyCaption</span><dl><dt
style="style_dt;">_test_data_type</dt><dd>Integer</dd><dt
style="style_dt;">_test_android_restriction_name</dt><dd
style="style_.monospace;">PolicyName</dd><dt
style="style_dt;">_test_supported_on</dt><dd><ul style="style_ul;"><li>Chrome
(Android) ..33..</li></ul></dd><dt
style="style_dt;">_test_supported_features</dt><dd>_test_feature_dynamic_refresh:
_test_not_supported</dd><dt
style="style_dt;">_test_description</dt><dd><p>PolicyDesc</p></dd><dt
style="style_dt;">_test_example_value</dt><dd>123 (Android)</dd></dl><a
href="#top">_test_back_to_top</a></div></root>' != '<root><div
style="margin-left: 0px"><h3><a
name="PolicyName"/>PolicyName</h3><span>PolicyCaption</span><dl><dt
style="style_dt;">_test_data_type</dt><dd>Integer</dd><dt
style="style_dt;">_test_android_restriction_name</dt><dd
style="style_.monospace;">PolicyName</dd><dt
style="style_dt;">_test_supported_on</dt><dd><ul style="style_ul;"><li>Chrome
(Android) ..33..</li></ul></dd><dt
style="style_dt;">_test_supported_features</dt><dd>_test_feature_dynamic_refresh:
_test_not_supported</dd><dt
style="style_dt;">_test_description</dt><dd>PolicyDesc</dd><dt
style="style_dt;">_test_example_value</dt><dd>123 (Android)</dd></dl><a
href="#top">_test_back_to_top</a></div></root>'

----------------------------------------------------------------------
Ran 360 tests in 2.424s

[Nico, 2015-03-04 19:15:23]

https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer_unittest.py:856:
'<dd>PolicyDesc</dd>'
Changing this line to

            '<dd><p>PolicyDesc</dd>'

makes the test pass for me. That's also what other tests in this file do. Any
reason you don't have this <p>?

[knn, 2015-03-04 19:25:51]

On 2015/03/04 19:15:23, Nico wrote:
>
https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
> File grit/format/policy_templates/writers/doc_writer_unittest.py (right):
> 
>
https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
> grit/format/policy_templates/writers/doc_writer_unittest.py:856:
> '<dd>PolicyDesc</dd>'
> Changing this line to
> 
>             '<dd><p>PolicyDesc</dd>'
> 
> makes the test pass for me. That's also what other tests in this file do. Any
> reason you don't have this <p>?

I am unable to replicate this on computers around me. Please give me some time
to figure this out.

[knn, 2015-03-04 19:45:10]

None of tests have <p> around PolicyDesc right?
Shouldn't it fail in all of them?

https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer_unittest.py:856:
'<dd>PolicyDesc</dd>'
On 2015/03/04 19:15:23, Nico wrote:
> Changing this line to
> 
>             '<dd><p>PolicyDesc</dd>'
> 
> makes the test pass for me. That's also what other tests in this file do. Any
> reason you don't have this <p>?

None of tests have <p> around PolicyDesc right?

[Nico, 2015-03-04 19:56:07]

https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
File grit/format/policy_templates/writers/doc_writer_unittest.py (right):

https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
grit/format/policy_templates/writers/doc_writer_unittest.py:856:
'<dd>PolicyDesc</dd>'
On 2015/03/04 19:45:09, knn wrote:
> On 2015/03/04 19:15:23, Nico wrote:
> > Changing this line to
> > 
> >             '<dd><p>PolicyDesc</dd>'
> > 
> > makes the test pass for me. That's also what other tests in this file do.
Any
> > reason you don't have this <p>?
> 
> None of tests have <p> around PolicyDesc right?

all of the other PolicyDescs in <dl>s in the test expectations in
doc_writer_unittest.py are wrapped in <p>s over here. Here's an example:
https://code.google.com/p/grit-i18n/source/browse/trunk/grit/format/policy_te...

[knn, 2015-03-04 20:51:50]

On 2015/03/04 19:56:07, Nico wrote:
>
https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
> File grit/format/policy_templates/writers/doc_writer_unittest.py (right):
> 
>
https://codereview.chromium.org/865563002/diff/80001/grit/format/policy_templ...
> grit/format/policy_templates/writers/doc_writer_unittest.py:856:
> '<dd>PolicyDesc</dd>'
> On 2015/03/04 19:45:09, knn wrote:
> > On 2015/03/04 19:15:23, Nico wrote:
> > > Changing this line to
> > > 
> > >             '<dd><p>PolicyDesc</dd>'
> > > 
> > > makes the test pass for me. That's also what other tests in this file do.
> Any
> > > reason you don't have this <p>?
> > 
> > None of tests have <p> around PolicyDesc right?
> 
> all of the other PolicyDescs in <dl>s in the test expectations in
> doc_writer_unittest.py are wrapped in <p>s over here. Here's an example:
>
https://code.google.com/p/grit-i18n/source/browse/trunk/grit/format/policy_te...

Pausing this until Chromium is rolled to Grit r186.

[Nico, 2015-03-04 20:58:04]

ooooh i see, you're editing this in grit-in-your-chromium checkout, not in grit
trunk?

[knn, 2015-03-04 21:54:23]

On 2015/03/04 20:58:04, Nico wrote:
> ooooh i see, you're editing this in grit-in-your-chromium checkout, not in
grit
> trunk?

Yeah. I thought this is the norm since Code reviews happen through Chromium's
Rietveld.
I have rebased it to r186 now.

[Nico, 2015-03-04 22:14:16]

landed in grit r187