Add version information to the policy templates

Add version information to the policy templates

Add for each policy the version number of Chrome when is was introduced.
Update platform-based policy filtering to use the new template format (no
new logic introduced.) Update the HTML documentation writer to use the new
version numbers.

BUG=62500
TEST=PolicyTemplateGeneratorUnittest.testPolicyFiltering,DocWriterUnittest.testAddPolicyDetails,DocWriterUnittest.testAddPolicySection

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

[gfeher, 2010-11-09 18:15:36]

Please review this CL!

[gfeher, 2010-11-09 20:27:25]

Now with your correct address. Jean-Luc, please review this when you'll have
time!

[jeanluc, 2010-11-10 05:11:50]

I've entered my comments but am getting an error when trying to
publish.  Can you check them there?

Jean-Luc

On Tue, Nov 9, 2010 at 12:27 PM,  <gfeher@chromium.org> wrote:
> Now with your correct address. Jean-Luc, please review this when you'll have
> time!
>
> http://codereview.chromium.org/4704006/show
>

[gfeher, 2010-11-10 08:45:54]

Thank you. Unfortunately I can't see any of them. This seems to be bug
http://crbug.com/55888
The work-around is to not to add comments to the JSON file. Please comment it
separately in a message.

Gábor

[jeanluc, 2010-11-10 15:51:37]

That must be it.  I could not see the side by side json file either.
Here are the two comments about the .json file:

-------
+# Products and versions:
+#   Each policy has the list of products and version numbers where it is
+#   supported under the key 'supported_on'. Each item of this list has the
+#   form of 'product:since_version-until_version', which means that support
+#   for the policy in 'product' was introduced in 'since_version' and removed

It's not just product but product[.platform].  I would explain that.

I would explain also where * can be used and what it means.

It would also be good documenting what accepted version values are.
The unit tests show single digit values yet one of the entries of the
type 0.2.3.12.
----
-	'name': 'ExtensionInstallForcelist',
-	'type': 'list',
-	'annotations': {
-          'platforms': ['linux', 'mac', 'win'],
-          'products': ['chrome'],
+        'name': 'ExtensionInstallForcelist',
+        'supported_on': ['chrome.*:9-'],
supported_on appears after type for the other entries.  I'd be consistent.
+        'type': 'list',
+        'annotations': {
----

Jean-Luc

On Wed, Nov 10, 2010 at 12:45 AM,  <gfeher@chromium.org> wrote:
> Thank you. Unfortunately I can't see any of them. This seems to be bug
> http://crbug.com/55888
> The work-around is to not to add comments to the JSON file. Please comment
> it
> separately in a message.
>
> Gábor
>
> http://codereview.chromium.org/4704006/
>

[jeanluc1, 2010-11-10 15:52:14]

Comments on the .json file that can't be done directly for some reason...
------
+# Products and versions:
+#   Each policy has the list of products and version numbers where it is
+#   supported under the key 'supported_on'. Each item of this list has the
+#   form of 'product:since_version-until_version', which means that support
+#   for the policy in 'product' was introduced in 'since_version' and removed

It's not just product but product[.platform].  I would explain that.

I would explain also where * can be used and what it means.

It would also be good documenting what accepted version values are.
The unit tests show single digit values yet one of the entries of the
type 0.2.3.12.
----
-       'name': 'ExtensionInstallForcelist',
-       'type': 'list',
-       'annotations': {
-          'platforms': ['linux', 'mac', 'win'],
-          'products': ['chrome'],
+        'name': 'ExtensionInstallForcelist',
+        'supported_on': ['chrome.*:9-'],
supported_on appears after type for the other entries.  I'd be consistent.
+        'type': 'list',
+        'annotations': {
----

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/doc_writer.py (left):

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:457: 'chrome_os':
self.config['os_name'],
Is this change on purpose?

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/doc_writer.py (right):

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:305: product,
platform = product_platform.split('.')
You have code that parses the pattern in two different files.  I would suggest
creating one that explodes the pattern into (product, platform, since-version,
until-version).

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:321:
text.append('%s %s' % (since_version, since))
This assumes that "since version" happens before the actual version in all
languages.  I don't know that there's not a language where it would be something
like: "Since 8 version".  Better to use placeholders and substitute.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/template_writer.py (right):

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:6: import re
I don't think that's used.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:52: if
product_platform == 'chrome_frame':
I'm not sure I would make an exception just for chrome_frame.  Why not just have
chrome_frame.win in the data files?

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:58: # e.g.:
product_platform = 'chrome_os'
In this case, platform will not be set and we'll never return true.  That does
not look right.

It would be good to give examples of possible matches and the expected results.

[gfeher, 2010-11-11 18:59:04]

Please take another look.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/doc_writer.py (left):

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:457: 'chrome_os':
self.config['os_name'],
On 2010/11/10 15:52:15, jeanluc1 wrote:
> Is this change on purpose?

It was but I have changed my mind to avoid a special case. 
Chrome OS will be listed in the documentation as "Chrome OS (Chrome OS)",
similarly to "Chrome (Windows)".

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/doc_writer.py (right):

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:305: product,
platform = product_platform.split('.')
On 2010/11/10 15:52:15, jeanluc1 wrote:
> You have code that parses the pattern in two different files.  I would suggest
> creating one that explodes the pattern into (product, platform, since-version,
> until-version).

Done.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:321:
text.append('%s %s' % (since_version, since))
On 2010/11/10 15:52:15, jeanluc1 wrote:
> This assumes that "since version" happens before the actual version in all
> languages.  I don't know that there's not a language where it would be
something
> like: "Since 8 version".  Better to use placeholders and substitute.

Done.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/template_writer.py (right):

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:6: import re
On 2010/11/10 15:52:15, jeanluc1 wrote:
> I don't think that's used.

Done.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:52: if
product_platform == 'chrome_frame':
On 2010/11/10 15:52:15, jeanluc1 wrote:
> I'm not sure I would make an exception just for chrome_frame.  Why not just
have
> chrome_frame.win in the data files?

Currently self.platforms is a list of platforms, but I am planning to change it
to a similar list to the 'supported_on' lists in policy_templates.json. So this
exception is only temporary, this is why I'd like to keep it.

http://codereview.chromium.org/4704006/diff/11001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:58: # e.g.:
product_platform = 'chrome_os'
On 2010/11/10 15:52:15, jeanluc1 wrote:
> In this case, platform will not be set and we'll never return true.  That does
> not look right.
> 
> It would be good to give examples of possible matches and the expected
results.

I've refactored this code and it became much more cleaner.

[jeanluc1, 2010-11-12 01:44:48]

http://codereview.chromium.org/4704006/diff/37001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/4704006/diff/37001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:660: since version <ph
name="SINCE_VERSION">$6<ex>8</ex></ph>
Why not use $1?

http://codereview.chromium.org/4704006/diff/37001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:664: until version <ph
name="UNTIL_VERSION">$6<ex>10</ex></ph>
Same here.

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/policy_template_generator.py
(right):

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:89:
product_part, version_part = supported_on_item.split(':')
I would name product_part to be product_platform_part

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:105:
platforms = [platform]
What happens if someone enters 'chrome:8-'?  Seems like this will cause an
exception.   Is that what we want?  Or is the omission of the .platform part the
same as .*?

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:113: return
result
One more note:  having a compact representation is nice but you could instead
require the dictionary to be entered in the .json file.

[gfeher, 2010-11-12 17:58:00]

http://codereview.chromium.org/4704006/diff/37001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/4704006/diff/37001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:660: since version <ph
name="SINCE_VERSION">$6<ex>8</ex></ph>
On 2010/11/12 01:44:48, jeanluc1 wrote:
> Why not use $1?

$1 gets always replaced by the product name in template_formatter.py. See the
end of policy_templates.json for the list of the rest:
http://www.google.com/codesearch/p?hl=en#hfE6470xZHk/chrome/app/policy/policy...
The URLs should be cleaned up from there, though.

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/policy_template_generator.py
(right):

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:89:
product_part, version_part = supported_on_item.split(':')
On 2010/11/12 01:44:48, jeanluc1 wrote:
> I would name product_part to be product_platform_part

Done.

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:105:
platforms = [platform]
On 2010/11/12 01:44:48, jeanluc1 wrote:
> What happens if someone enters 'chrome:8-'?  Seems like this will cause an
> exception.   Is that what we want?  Or is the omission of the .platform part
the
> same as .*?

I'd prefer to throw an exception so that the format does not allow doing one
thing in slightly different ways.

http://codereview.chromium.org/4704006/diff/37001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:113: return
result
On 2010/11/12 01:44:48, jeanluc1 wrote:
> One more note:  having a compact representation is nice but you could instead
> require the dictionary to be entered in the .json file.

I like the compact one better because the size of the JSON dictionary would be
larger by a big factor.

[gfeher, 2010-11-16 17:25:37]

Please take another look.

[jeanluc1, 2010-11-16 18:09:45]

LGTM