Docserver manifest follow up (rewrite)

chrome/common/extensions/docs/server2/features_model.py

Index: chrome/common/extensions/docs/server2/features_model.py
diff --git a/chrome/common/extensions/docs/server2/features_model.py b/chrome/common/extensions/docs/server2/features_model.py
new file mode 100644
index 0000000000000000000000000000000000000000..95ff077df96ecdc037454a318a430521782bcaef
--- /dev/null
+++ b/chrome/common/extensions/docs/server2/features_model.py
@@ -0,0 +1,104 @@
+# Copyright 2013 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.
+
+from copy import deepcopy
+
+class FeaturesModel(object):
+  '''Load and manipulate the contents of a features file. FeaturesModel is
+  immutable.
+  '''
+  def __init__(self, features={}):
+    self._features = deepcopy(features)
+
+  def LoadFeaturesJson(self, features_json):

[not at google - send to devlin, 2013/07/31 00:36:39]

make this a static method like Parse on FeaturesModel which returns a new
FeaturesModel. You'll only ever want to call this method on construction.

[jshumway, 2013/07/31 18:10:28]

Done.

+    '''Take the raw json of a features file and clean it up. This involves
+    reducing any keys with lists as values to sinlge dicts, converting
+    'extension_types' to a list that contains 'app', 'extension', or both,
+    adding a 'name' field, and ignoring features that have a 'whitelist'
+    attribute.
+    '''
+    features = deepcopy(self._features)
+
+    for name, value in features_json.iteritems():
+      if isinstance(value, list):
+        temp = None
+        for subvalue in value:
+          if not 'whitelist' in subvalue:
+            temp = subvalue
+            break
+        else:
+          continue
+        value = temp
+
+      if 'whitelist' in value:
+        continue
+      features[name] = { 'platform': [] }
+
+      platforms = value.pop('extension_types')
+      if platforms == 'all' or 'extension' in platforms:
+        features[name]['platform'].append('extension')
+      if platforms == 'all' or 'platform_app' in platforms:
+        features[name]['platform'].append('app')
+
+      features[name]['name'] = name
+      features[name].update(value)
+
+    return FeaturesModel(features)
+
+  def Filter(self, platform=None):
+    '''Remove all entries that are not relevant to |platform|.
+    '''
+    features = deepcopy(self._features)
+
+    for name, feature in features.items():
+      if platform and not platform in feature['platform']:
+        del features[name]

[not at google - send to devlin, 2013/07/31 00:36:39]

it would be more efficient to do this constructively rather than destructively.

filtered_features = {}
for name, feature in features.items():
  if platform is None or platform in feature['platform']:
    # You don't need to (deep)copy this because the features list is
    # immutable - therefore nothing will mutate it, and we're safe to
    # share data.
    filtered_features[name] = feature
return FeaturesModel(filtered_features)

[jshumway, 2013/07/31 18:10:28]

Done.

+
+    return FeaturesModel(features)
+
+  def MergeWith(self, other):
+    '''Do a semi-recursive merge. Any keys common to both |self| and |other|
+    will have their dictionary values merged, instead of overwritten.
+    '''
+    features = deepcopy(self._features)
+
+    for key, value in other.iteritems():
+      if key in self._features:
+        features[key].update(value)
+      else:
+        features[key] = value
+
+    return FeaturesModel(features)

[not at google - send to devlin, 2013/07/31 00:36:39]

likewise

all that really needs to be different between these is the top-level object.

[jshumway, 2013/07/31 18:10:28]

Done.

+
+  def RestructureChildren(self):

[not at google - send to devlin, 2013/07/31 00:36:39]

I think that RestructureChidlren actually sounds like it mutates the objects,
which it doesnt.

Something like WithNestedChildren might be more appropriate.

[jshumway, 2013/07/31 18:10:28]

See below.

+    '''Features whose names are of the form 'parent.child' are moved to be part
+    of the 'parent' dictionary under the key 'children'. Names are changed to
+    the 'child' section of the original name.
+    '''
+    features = deepcopy(self._features)

[not at google - send to devlin, 2013/07/31 00:36:39]

I guess deepcopy here is ok, though it is unnecessarily wasteful since the
majority of these features *aren't* nested, so we're creating a lot of
duplicates.

you could copy() here and then deepcopy as necessary (maintaining a set of
things that have already been deepcoppied) but that would be wasteful.

Lemme see how this function gets called though.

[jshumway, 2013/07/31 18:10:28]

Okay, I moved this method into manifest data source, where it has kept the name
'RestructureChildren', because it does mutate the dictionary passed to it.

+
+    def add_child(features, parent, child_name, value):
+      value['name'] = child_name
+      if not 'children' in features[parent]:
+        features[parent]['children'] = {}
+      features[parent]['children'][child_name] = value
+
+    def insert_children(features):
+      for name in features.keys():
+        if '.' in name:
+          value = features.pop(name)
+          parent, child_name = name.split('.', 1)
+          add_child(features, parent, child_name, value)
+
+      for value in features.values():
+        if 'children' in value:
+          insert_children(value['children'])
+
+    insert_children(features)
+    return features
+
+  def Get(self):
+    '''Returns a dictionary of the features file.
+    '''
+    return deepcopy(self._features)