Improved sorting for policy templates

Mac templates: sort policies alphabetically
Linux .json examples: sort policies alphabetically
Windows .reg examples: group all policies with type!="list"

BUG=61733
TEST=manual inspection of generated policy template files; grit.format.policy_templates.writers.template_writer_unittest

Review URL: http://codereview.chromium.org/6137002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@71022 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 198 insertions, 97 deletions

diff --git a/tools/grit/grit/format/policy_templates/PRESUBMIT.py b/tools/grit/grit/format/policy_templates/PRESUBMIT.py
index d949a0e..2822968 100644
--- a/tools/grit/grit/format/policy_templates/PRESUBMIT.py
+++ b/tools/grit/grit/format/policy_templates/PRESUBMIT.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -11,7 +11,8 @@ UNIT_TESTS = [
   'writers.json_writer_unittest',
   'writers.plist_strings_writer_unittest',
   'writers.plist_writer_unittest',
-  'writers.reg_writer_unittest'
+  'writers.reg_writer_unittest',
+  'writers.template_writer_unittest'
 ]
 
 def CheckChangeOnUpload(input_api, output_api):
diff --git a/tools/grit/grit/format/policy_templates/policy_template_generator.py b/tools/grit/grit/format/policy_templates/policy_template_generator.py
index 3b2447e..cce0116 100644
--- a/tools/grit/grit/format/policy_templates/policy_template_generator.py
+++ b/tools/grit/grit/format/policy_templates/policy_template_generator.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -6,22 +6,6 @@
 import copy
 
 
-def GetPolicySortingKey(policy):
-  '''Extracts a sorting key from a policy. These keys can be used for
-  list.sort() methods to sort policies.
-  See PolicyTemplateGenerator._SortPolicies for usage.
-  '''
-  is_group = policy['type'] == 'group'
-  if is_group:
-    # Groups are sorted by caption.
-    str_key = policy['caption']
-  else:
-    # Regular policies are sorted by name.
-    str_key = policy['name']
-  # Groups come before regular policies.
-  return (not is_group, str_key)
-
-
 class PolicyTemplateGenerator:
   '''Generates template text for a particular platform.
 
@@ -49,18 +33,6 @@ class PolicyTemplateGenerator:
     # Localized messages to be inserted to the policy_definitions structure:
     self._messages = messages
     self._ProcessPolicyList(self._policy_definitions)
-    self._SortPolicies(self._policy_definitions)
-
-  def _SortPolicies(self, policy_list):
-    '''Sorts a list of policies in-place alphabetically. The order is the
-    following: first groups alphabetically by caption, then other policies
-    alphabetically by name. The order of policies inside groups is unchanged.
-
-    Args:
-      policy_list: The list of policies to sort. Sub-lists in groups will not
-        be sorted.
-    '''
-    policy_list.sort(key=GetPolicySortingKey)
 
   def _ProcessSupportedOn(self, supported_on):
     '''Parses and converts the string items of the list of supported platforms
diff --git a/tools/grit/grit/format/policy_templates/policy_template_generator_unittest.py b/tools/grit/grit/format/policy_templates/policy_template_generator_unittest.py
index 5a73cf72..d51c0a5 100644
--- a/tools/grit/grit/format/policy_templates/policy_template_generator_unittest.py
+++ b/tools/grit/grit/format/policy_templates/policy_template_generator_unittest.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -257,7 +257,7 @@ class PolicyTemplateGeneratorUnittest(unittest.TestCase):
           'supported_on': ['chrome.ddd:8-']
         },
       ]
-    },{
+    }, {
       'name': 'Group2',
       'type': 'group',
       'policies': [
@@ -267,7 +267,7 @@ class PolicyTemplateGeneratorUnittest(unittest.TestCase):
           'supported_on': ['chrome.eee:8-']
         },
       ]
-    },{
+    }, {
       'name': 'SinglePolicy',
       'type': 'int',
       'supported_on': ['chrome.eee:8-']
@@ -306,52 +306,6 @@ class PolicyTemplateGeneratorUnittest(unittest.TestCase):
         local_mock_writer.result_list,
         ['begin_Group1', 'Group1Policy1', 'Group1Policy2', 'end_group'])
 
-  def testSorting(self):
-    # Tests that policies are sorted correctly.
-    policy_defs = [
-      {'name': 'zp', 'type': 'string', 'caption': 'a1', 'supported_on': []},
-      {
-        'type': 'group',
-        'caption': 'z_group1_caption',
-        'name': 'group1',
-        'policies': [
-          {'name': 'z0', 'type': 'string', 'supported_on': []},
-          {'name': 'a0', 'type': 'string', 'supported_on': []}
-        ]
-      },
-      {
-        'type': 'group',
-        'caption': 'b_group2_caption',
-        'name': 'group2',
-        'policies': [{'name': 'q', 'type': 'string', 'supported_on': []}],
-      },
-      {'name': 'ap', 'type': 'string', 'caption': 'a2', 'supported_on': []}
-    ]
-    sorted_policy_defs = [
-      {
-        'type': 'group',
-        'caption': 'b_group2_caption',
-        'name': 'group2',
-        'policies': [{'name': 'q', 'type': 'string', 'supported_on': []}],
-      },
-      {
-        'type': 'group',
-        'caption': 'z_group1_caption',
-        'name': 'group1',
-        'policies': [
-          {'name': 'z0', 'type': 'string', 'supported_on': []},
-          {'name': 'a0', 'type': 'string', 'supported_on': []}
-        ]
-      },
-      {'name': 'ap', 'type': 'string', 'caption': 'a2', 'supported_on': []},
-      {'name': 'zp', 'type': 'string', 'caption': 'a1', 'supported_on': []},
-    ]
-    ptg = policy_template_generator.PolicyTemplateGenerator([], [])
-    ptg._SortPolicies(policy_defs)
-    self.assertEquals(
-        policy_defs,
-        sorted_policy_defs)
-
   def testSortingInvoked(self):
     # Tests that policy-sorting happens before passing policies to the writer.
     policy_defs = [
diff --git a/tools/grit/grit/format/policy_templates/writers/json_writer.py b/tools/grit/grit/format/policy_templates/writers/json_writer.py
index 776e4f5..8a6e35f 100644
--- a/tools/grit/grit/format/policy_templates/writers/json_writer.py
+++ b/tools/grit/grit/format/policy_templates/writers/json_writer.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -22,6 +22,9 @@ class JsonWriter(template_writer.TemplateWriter):
   files.
   '''
 
+  def PreprocessPolicies(self, policy_list):
+    return self.FlattenGroupsAndSortPolicies(policy_list)
+
   def WritePolicy(self, policy):
     example_value = policy['annotations']['example_value']
     if policy['type'] == 'string':
diff --git a/tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py b/tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py
index 965c900..4f699f2 100644
--- a/tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py
+++ b/tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -31,12 +31,15 @@ class PListStringsWriter(template_writer.TemplateWriter):
       desc: The text of the description to add.
     '''
     caption = caption.replace('"', '\\"')
-    caption = caption.replace('\n','\\n')
+    caption = caption.replace('\n', '\\n')
     desc = desc.replace('"', '\\"')
-    desc = desc.replace('\n','\\n')
+    desc = desc.replace('\n', '\\n')
     self._out.append('%s.pfm_title = \"%s\";' % (item_name, caption))
     self._out.append('%s.pfm_description = \"%s\";' % (item_name, desc))
 
+  def PreprocessPolicies(self, policy_list):
+    return self.FlattenGroupsAndSortPolicies(policy_list)
+
   def WritePolicy(self, policy):
     '''Add strings to the stringtable corresponding a given policy.
 
diff --git a/tools/grit/grit/format/policy_templates/writers/plist_writer.py b/tools/grit/grit/format/policy_templates/writers/plist_writer.py
index 87e9db9..a0068ac 100644
--- a/tools/grit/grit/format/policy_templates/writers/plist_writer.py
+++ b/tools/grit/grit/format/policy_templates/writers/plist_writer.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -71,6 +71,9 @@ class PListWriter(xml_formatted_writer.XMLFormattedWriter):
     array = self._AddKeyValuePair(parent, 'pfm_targets', 'array')
     self.AddElement(array, 'string', {}, 'user-managed')
 
+  def PreprocessPolicies(self, policy_list):
+    return self.FlattenGroupsAndSortPolicies(policy_list)
+
   def WritePolicy(self, policy):
     policy_name = policy['name']
     policy_type = policy['type']
@@ -122,7 +125,7 @@ class PListWriter(xml_formatted_writer.XMLFormattedWriter):
     # Get all the XML content in a one-line string.
     xml = self._doc.toxml()
     # Determine where the line breaks will be. (They will only be between tags.)
-    lines = xml[1:len(xml)-1].split('><')
+    lines = xml[1:len(xml) - 1].split('><')
     indent = ''
     res = ''
     # Determine indent for each line.
@@ -134,7 +137,7 @@ class PListWriter(xml_formatted_writer.XMLFormattedWriter):
         indent = indent[2:]
       lines[i] = indent + '<' + line + '>'
       if (line[0] not in ['/', '?', '!'] and '</' not in line and
-          line[len(line)-1] != '/'):
+          line[len(line) - 1] != '/'):
         # If the current line starts with an opening tag and does not conatin a
         # closing tag, increase indent after the line is printed.
         indent += '  '
diff --git a/tools/grit/grit/format/policy_templates/writers/reg_writer.py b/tools/grit/grit/format/policy_templates/writers/reg_writer.py
index 5d03fbd..92a39c9 100644
--- a/tools/grit/grit/format/policy_templates/writers/reg_writer.py
+++ b/tools/grit/grit/format/policy_templates/writers/reg_writer.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -36,6 +36,19 @@ class RegWriter(template_writer.TemplateWriter):
       self._out.append('[%s]' % key)
       self._last_key = key
 
+  def PreprocessPolicies(self, policy_list):
+    return self.FlattenGroupsAndSortPolicies(policy_list,
+                                             self.GetPolicySortingKey)
+
+  def GetPolicySortingKey(self, policy):
+    '''Extracts a sorting key from a policy. These keys can be used for
+    list.sort() methods to sort policies.
+    See TemplateWriter.SortPoliciesGroupsFirst for usage.
+    '''
+    is_list = policy['type'] == 'list'
+    # Lists come after regular policies.
+    return (is_list, policy['name'])
+
   def WritePolicy(self, policy):
     example_value = policy['annotations']['example_value']
 
diff --git a/tools/grit/grit/format/policy_templates/writers/reg_writer_unittest.py b/tools/grit/grit/format/policy_templates/writers/reg_writer_unittest.py
index 9cb9db7..9cd614b 100644
--- a/tools/grit/grit/format/policy_templates/writers/reg_writer_unittest.py
+++ b/tools/grit/grit/format/policy_templates/writers/reg_writer_unittest.py
@@ -1,5 +1,5 @@
-#!/usr/bin/python2.4
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+#!/usr/bin/python2.6
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -20,7 +20,7 @@ from grit.format.policy_templates.writers import writer_unittest_common
 class RegWriterUnittest(writer_unittest_common.WriterUnittestCommon):
   '''Unit tests for RegWriter.'''
 
-  NEWLINE='\r\n'
+  NEWLINE = '\r\n'
 
   def CompareOutputs(self, output, expected_output):
     '''Compares the output of the reg_writer with its expected output.
@@ -43,7 +43,7 @@ class RegWriterUnittest(writer_unittest_common.WriterUnittestCommon):
         '  "policy_definitions": [],'
         '  "placeholders": [],'
         '}', '<messages></messages>')
-    output = self.GetOutput(grd, 'fr', {'_chromium': '1',}, 'reg', 'en')
+    output = self.GetOutput(grd, 'fr', {'_chromium': '1', }, 'reg', 'en')
     expected_output = 'Windows Registry Editor Version 5.00'
     self.CompareOutputs(output, expected_output)
 
@@ -233,12 +233,12 @@ class RegWriterUnittest(writer_unittest_common.WriterUnittestCommon):
     expected_output = self.NEWLINE.join([
         'Windows Registry Editor Version 5.00',
         '',
+        '[HKEY_LOCAL_MACHINE\\Software\\Policies\\Chromium]',
+        '"Policy2"="c"',
+        '',
         '[HKEY_LOCAL_MACHINE\\Software\\Policies\\Chromium\\Policy1]',
         '"1"="a"',
-        '"2"="b"',
-        '',
-        '[HKEY_LOCAL_MACHINE\\Software\\Policies\\Chromium]',
-        '"Policy2"="c"'])
+        '"2"="b"'])
     self.CompareOutputs(output, expected_output)
 
 
diff --git a/tools/grit/grit/format/policy_templates/writers/template_writer.py b/tools/grit/grit/format/policy_templates/writers/template_writer.py
index 8b742d5..01c2ef4 100644
--- a/tools/grit/grit/format/policy_templates/writers/template_writer.py
+++ b/tools/grit/grit/format/policy_templates/writers/template_writer.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 The Chromium Authors. All rights reserved.
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -89,6 +89,7 @@ class TemplateWriter(object):
       Generated output for the passed template definition.
     '''
     self.Init()
+    template = self.PreprocessPolicies(template)
     self.BeginTemplate()
     for policy in template:
       if policy['type'] == 'group':
@@ -105,6 +106,22 @@ class TemplateWriter(object):
     self.EndTemplate()
     return self.GetTemplateText()
 
+  def PreprocessPolicies(self, policy_list):
+    '''Preprocesses a list of policies according to a given writer's needs.
+    Preprocessing steps include sorting policies and stripping unneeded
+    information such as groups (for writers that ignore them).
+    Subclasses are encouraged to override this method, overriding
+    implementations may call one of the provided specialized implementations.
+    The default behaviour is to use SortPoliciesGroupsFirst().
+
+    Args:
+      policy_list: A list containing the policies to sort.
+
+    Returns:
+      The sorted policy list.
+    '''
+    return self.SortPoliciesGroupsFirst(policy_list)
+
   def WritePolicy(self, policy):
     '''Appends the template text corresponding to a policy into the
     internal buffer.
@@ -148,3 +165,55 @@ class TemplateWriter(object):
       The generated template from the the internal buffer as a string.
     '''
     raise NotImplementedError()
+
+  def SortPoliciesGroupsFirst(self, policy_list):
+    '''Sorts a list of policies alphabetically. The order is the
+    following: first groups alphabetically by caption, then other policies
+    alphabetically by name. The order of policies inside groups is unchanged.
+
+    Args:
+      policy_list: The list of policies to sort. Sub-lists in groups will not
+        be sorted.
+    '''
+    policy_list.sort(key=self.GetPolicySortingKeyGroupsFirst)
+    return policy_list
+
+  def FlattenGroupsAndSortPolicies(self, policy_list, sorting_key=None):
+    '''Sorts a list of policies according to |sorting_key|, defaulting
+    to alphabetical sorting if no key is given. If |policy_list| contains
+    policies with type="group", it is flattened first, i.e. any groups' contents
+    are inserted into the list as first-class elements and the groups are then
+    removed.
+    '''
+    new_list = []
+    for policy in policy_list:
+      if policy['type'] == 'group':
+        for grouped_policy in policy['policies']:
+          new_list.append(grouped_policy)
+      else:
+        new_list.append(policy)
+    if sorting_key == None:
+      sorting_key = self.GetPolicySortingKeyName
+    new_list.sort(key=sorting_key)
+    return new_list
+
+  def GetPolicySortingKeyName(self, policy):
+    return policy['name']
+
+  def GetPolicySortingKeyGroupsFirst(self, policy):
+    '''Extracts a sorting key from a policy. These keys can be used for
+    list.sort() methods to sort policies.
+    See TemplateWriter.SortPolicies for usage.
+    '''
+    is_group = policy['type'] == 'group'
+    if is_group:
+      # Groups are sorted by caption.
+      str_key = policy['caption']
+    else:
+      # Regular policies are sorted by name.
+      str_key = policy['name']
+    # Groups come before regular policies.
+    return (not is_group, str_key)
+
+
+
diff --git a/tools/grit/grit/format/policy_templates/writers/template_writer_unittest.py b/tools/grit/grit/format/policy_templates/writers/template_writer_unittest.py
new file mode 100644
index 0000000..60ea55a
--- /dev/null
+++ b/tools/grit/grit/format/policy_templates/writers/template_writer_unittest.py
@@ -0,0 +1,83 @@
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+'''Unit tests for grit.format.policy_templates.writers.template_writer'''
+
+import os
+import sys
+if __name__ == '__main__':
+  sys.path.append(os.path.join(os.path.dirname(sys.argv[0]), '../../../..'))
+
+import unittest
+
+from grit.format.policy_templates.writers import template_writer
+
+
+POLICY_DEFS = [
+  {'name': 'zp', 'type': 'string', 'caption': 'a1', 'supported_on': []},
+  {
+    'type': 'group',
+    'caption': 'z_group1_caption',
+    'name': 'group1',
+    'policies': [
+      {'name': 'z0', 'type': 'string', 'supported_on': []},
+      {'name': 'a0', 'type': 'string', 'supported_on': []}
+    ]
+  },
+  {
+    'type': 'group',
+    'caption': 'b_group2_caption',
+    'name': 'group2',
+    'policies': [{'name': 'q', 'type': 'string', 'supported_on': []}],
+  },
+  {'name': 'ap', 'type': 'string', 'caption': 'a2', 'supported_on': []}
+]
+
+
+GROUP_FIRST_SORTED_POLICY_DEFS = [
+  {
+    'type': 'group',
+    'caption': 'b_group2_caption',
+    'name': 'group2',
+    'policies': [{'name': 'q', 'type': 'string', 'supported_on': []}],
+  },
+  {
+    'type': 'group',
+    'caption': 'z_group1_caption',
+    'name': 'group1',
+    'policies': [
+      {'name': 'z0', 'type': 'string', 'supported_on': []},
+      {'name': 'a0', 'type': 'string', 'supported_on': []}
+    ]
+  },
+  {'name': 'ap', 'type': 'string', 'caption': 'a2', 'supported_on': []},
+  {'name': 'zp', 'type': 'string', 'caption': 'a1', 'supported_on': []},
+]
+
+
+IGNORE_GROUPS_SORTED_POLICY_DEFS = [
+  {'name': 'a0', 'type': 'string', 'supported_on': []},
+  {'name': 'ap', 'type': 'string', 'caption': 'a2', 'supported_on': []},
+  {'name': 'q', 'type': 'string', 'supported_on': []},
+  {'name': 'z0', 'type': 'string', 'supported_on': []},
+  {'name': 'zp', 'type': 'string', 'caption': 'a1', 'supported_on': []},
+]
+
+
+class TemplateWriterUnittests(unittest.TestCase):
+  '''Unit tests for templater_writer.py.'''
+
+  def testSortingGroupsFirst(self):
+    tw = template_writer.TemplateWriter(None, None, None)
+    sorted_list = tw.SortPoliciesGroupsFirst(POLICY_DEFS)
+    self.assertEqual(sorted_list, GROUP_FIRST_SORTED_POLICY_DEFS)
+
+  def testSortingIgnoreGroups(self):
+    tw = template_writer.TemplateWriter(None, None, None)
+    sorted_list = tw.FlattenGroupsAndSortPolicies(POLICY_DEFS)
+    self.assertEqual(sorted_list, IGNORE_GROUPS_SORTED_POLICY_DEFS)
+
+
+if __name__ == '__main__':
+  unittest.main()