Docserver manifest follow up (rewrite)

Manifest patch follow up. Adds manifest property nesting (to an arbitrary depth), fixed a bug that prevented some properties from displaying properly, removed some invalid pages, and added annotation support.

Rewrote manifest.json and the data source to make merging manifest.json with _manifest_features.json in the future painless. It is now much much easier to add properties to manifest.json! Annotations are added automatically and appropriately to designate which properties are optional/required/etc and additional annotations added in manifest.json are supported.

NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217753

[not at google - send to devlin, 2013-07-02 18:23:11]

Ok I have outstanding comments here, I have no idea how relevant they are or
whether this change is still relevant. I probably didn't finish reviewing it or
something.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-8-1
just 2-8-0

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/manifest_data_source.py:36: """ Insert all
docs in the 'subdoc' category into their 'parent' objcets,
objcets

also """ -> '''

oh damn they're all like that.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/manifest_data_source.py:114:
self._ApplyExtensionsTransformations(extensions)
how about this:

apps['required'][0]['example'] = {'value': 'Application'}

for manifest in (apps, extensions):
  manifest['optional'].sort(...)
  manifest['optional'][-1]['is_last'] = True
  self._InsertSubDocs(manifest)

return {..}

you can delete _ApplyExtension/AppsTransformations.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:75:
self._AddSelected(sidenav)
oops...? how did this happen.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/json/manifest.json:6: "value": "My
Extension"
Why this extra "value" thing? Is that something I suggested? It seems
unnecessary.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/json/manifest.json:239: "annotation":
"Optional"
I like it

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/private/manifest_properties.html
(right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:3:
{{/annotation}}
This needs to be on its own line, right? Does it happen?

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:5: "<a
href="{{documentation}}">{{name}}</a>":{{:documentation}}
closing {{:documentation}} on its own line

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:10:
{{/}}
This should be able to be:

{{#subdoc +partials.manifest_properties}}

if not, the closing {{/}} needs subdoc

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:11:
}{{:subdoc}}{{?example}} {{*@.value}}{{/}}{{/}}{{^is_last}},{{/}}
can the {{:subdoc}} here be on its own line?

{{?example}} {{*@.value}}{{/}} should be able to be

{{?example *@.value}}

or even better

{{?example *value}}

[jshumway, 2013-07-24 17:40:07]

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/app.yaml (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/app.yaml:2: version: 2-8-1
On 2013/07/02 18:23:11, kalman wrote:
> just 2-8-0

When we bump the middle version number, should we always set the last version
number to 0?

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/manifest_data_source.py:36: """ Insert all
docs in the 'subdoc' category into their 'parent' objcets,
On 2013/07/02 18:23:11, kalman wrote:
> objcets
> 
> also """ -> '''
> 
> oh damn they're all like that.

Rewrote pretty much the entire file, so it should be up to current standards in
style (''', tuples for literals not lists, etc)

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/manifest_data_source.py:114:
self._ApplyExtensionsTransformations(extensions)
On 2013/07/02 18:23:11, kalman wrote:
> how about this:
> 
> apps['required'][0]['example'] = {'value': 'Application'}
> 
> for manifest in (apps, extensions):
>   manifest['optional'].sort(...)
>   manifest['optional'][-1]['is_last'] = True
>   self._InsertSubDocs(manifest)
> 
> return {..}
> 
> you can delete _ApplyExtension/AppsTransformations.

Done.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/sidenav_data_source.py (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:75:
self._AddSelected(sidenav)
On 2013/07/02 18:23:11, kalman wrote:
> oops...? how did this happen.

Not sure, I think it was when I was updating the sidenav to use absolute URLs so
it the links wouldn't break on /platform/manifest/document.html paths. I think
this methods is back to the way it started.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/json/manifest.json:6: "value": "My
Extension"
On 2013/07/02 18:23:11, kalman wrote:
> Why this extra "value" thing? Is that something I suggested? It seems
> unnecessary.

I agree, but it is necessary.

Handlebar does not distinguish between "a value does not exist" and "a value is
false" (empty string, empty list, Boolean false, etc). This was causing a bug
with the manifest example display where lots of examples were not being
displayed because the had false values! I noticed when I was messing with
background.persistent, attempting to get 'false' to be the description and it
was never showing up.

I made example a dictionary that always contains a 'value' key. Thus
{{?example}} will either be false and indicate there is no example, or true and
that there is an example. Then {{value}} can always be used as literal json to
show the example, even if the value is false.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/json/manifest.json:239: "annotation":
"Optional"
On 2013/07/02 18:23:11, kalman wrote:
> I like it

To make manifest.json more friendly to anyone editing it, I created a function
that automatically applies annotations to the correct documents for 'level'
(optional, required, etc) indicators. So now you don't have to guess where it
should go.

The new 'annotations' key, which expects a list, is available for adding any
additional annotations though.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
File chrome/common/extensions/docs/templates/private/manifest_properties.html
(right):

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:3:
{{/annotation}}
On 2013/07/02 18:23:11, kalman wrote:
> This needs to be on its own line, right? Does it happen?

Yep, it renders correctly because this partial is expanded inside <pre> tags.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:5: "<a
href="{{documentation}}">{{name}}</a>":{{:documentation}}
On 2013/07/02 18:23:11, kalman wrote:
> closing {{:documentation}} on its own line

Because this partial is expanded inside <pre> tags, the handlebar markup has to
be in this unsightly form. A similar story for {{#subdocs}} below, which has
some strange whitespace errors when changed to the {{#subdocs +partial... /}}
form.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:10:
{{/}}
On 2013/07/02 18:23:11, kalman wrote:
> This should be able to be:
> 
> {{#subdoc +partials.manifest_properties}}
> 
> if not, the closing {{/}} needs subdoc

see above. fixed closing tag.

https://codereview.chromium.org/16410002/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/templates/private/manifest_properties.html:11:
}{{:subdoc}}{{?example}} {{*@.value}}{{/}}{{/}}{{^is_last}},{{/}}
On 2013/07/02 18:23:11, kalman wrote:
> can the {{:subdoc}} here be on its own line?
> 
> {{?example}} {{*@.value}}{{/}} should be able to be
> 
> {{?example *@.value}}
> 
> or even better
> 
> {{?example *value}}

same as above about own line, changed {{?example *value}} though.

[not at google - send to devlin, 2013-07-24 21:45:56]

I'm finding manifest_data_source.py a little bit hard to read, it's very compact
and python-library-y. I think if you moved some of it into the FeatureFile
(maybe that should be FeatureList?) class it would be easier to read + more
reasonably unit testable.

Sorry to be so vague here.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_utility.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:47: dest[key] = value
I would rather there were all OO. Create an abstraction for a feature file (call
it... FeatureFile) and query that for typed data, rather than transforming
arbitrary JSON to some other arbitrary JSON format.

You would have properties like extension_types and methods like Merge (how is
this different to dict.update() again?)

Note that I may actually want to move something like this into
json_schema_compiler proper in order to implement feature file compilation - but
don't worry about that yet. Just keep in mind that... I'll be keeping that in
mind.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:54: keyfunc =
lambda item: (levels[item.get('level', 'optional')], item['name'])
this code is a bit obfuscated for me to read, but it seems as though you could
make levels a list and use find() to make it a bit simpler.

this is also a bit too long for a lambda.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:56: def
convert_and_sort_recur(properties):
the "recur" is a bit of an implementation detail..

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:82: def
annotate_recur(parent_level, properties):
likewise

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:113:
_Annotate(properties)
I'd rather see this as

properties = _Annotate(_ConvertAndSortDocs(_Filter(master_dict, platform)))

you can probably use generators to implement it efficiently.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:119: break
maybe we could do some slightly less hacky hackery. elsewhere in the templates
we use {{title}} to display the title, which the templates use in various
places. we could implement that hackery by hand here - replace {{title}} with
either 'Extension' or 'App' depending on the value of |platform|. I think that
would be preferable to this.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:131:
Parse(self._file_system.ReadSingle(self._features_path)))
better name that "master"

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:139: 'master':
master
why do you need to cache the master?

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:107:
'/'.join((svn_constants.API_PATH, '_manifest_features.json')))
something funny happened here.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/subversion_file_system.py (left):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/subversion_file_system.py:11: from
file_system import FileSystem, FileNotFoundError, StatInfo, ToUnicode
could you submit this as a different change?

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:5: "value": {}
let's make sure for empty object to generate {...} in the documentation not {}

likewise [...] not [].

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:6: },
also - I think a better way to solve the problem of "example: false" being
detected as "no example" would be for manifest_data_source to insert a flag like
"has_example" for those APIs that have an example.

I.e. only exposed to templates, no need to specify in here.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:200: "level":
"optional"
let's just make optional the default and apply that to the manifest keys that
come from _manifest_features.json.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/public/extensions/manifest.html
(right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/public/extensions/manifest.html:2:
manifest_source:manifest_source.extensions}}
maybe objective-C style would work better here to keep the other file under 80
characters. that is,

{{+partials.standard_apps_article article:intros.manifest
                          manifest_source:manifest_source.apps}}

maybe that looks weird and

{{+partials.standard_apps_article
     article:intros.manifest
     manifest_source:manifest_source.apps}}

would be better.

[jshumway, 2013-07-26 00:36:46]

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_utility.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:47: dest[key] = value
On 2013/07/24 21:45:56, kalman wrote:
> I would rather there were all OO. Create an abstraction for a feature file
(call
> it... FeatureFile) and query that for typed data, rather than transforming
> arbitrary JSON to some other arbitrary JSON format.

Did my best to 'OOify' it.

> You would have properties like extension_types and methods like Merge (how is
> this different to dict.update() again?)

The difference is that merge (now named MergeWith) merges sub dictionaries as
well, instead of blindly replacing keys. In code...

{'x': {'y': 1}}.update({'x': {'z': 2}}) -> {'x': {'z': 2}}

{'x': {'y': 1}}.MergeWith({'x': {'z': 2}}) -> {'x': {'y': 1, 'z': 2}}

> Note that I may actually want to move something like this into
> json_schema_compiler proper in order to implement feature file compilation -
but
> don't worry about that yet. Just keep in mind that... I'll be keeping that in
> mind.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:54: keyfunc =
lambda item: (levels[item.get('level', 'optional')], item['name'])
On 2013/07/24 21:45:56, kalman wrote:
> this code is a bit obfuscated for me to read, but it seems as though you could
> make levels a list and use find() to make it a bit simpler.

brilliant idea, using list.index().

> this is also a bit too long for a lambda.

Now a named function.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:56: def
convert_and_sort_recur(properties):
On 2013/07/24 21:45:56, kalman wrote:
> the "recur" is a bit of an implementation detail..

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:82: def
annotate_recur(parent_level, properties):
On 2013/07/24 21:45:56, kalman wrote:
> likewise

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:113:
_Annotate(properties)
On 2013/07/24 21:45:56, kalman wrote:
> I'd rather see this as
> 
> properties = _Annotate(_ConvertAndSortDocs(_Filter(master_dict, platform)))
> 
> you can probably use generators to implement it efficiently.

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:119: break
On 2013/07/24 21:45:56, kalman wrote:
> maybe we could do some slightly less hacky hackery. elsewhere in the templates
> we use {{title}} to display the title, which the templates use in various
> places. we could implement that hackery by hand here - replace {{title}} with
> either 'Extension' or 'App' depending on the value of |platform|. I think that
> would be preferable to this.

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:131:
Parse(self._file_system.ReadSingle(self._features_path)))
On 2013/07/24 21:45:56, kalman wrote:
> better name that "master"

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:139: 'master':
master
On 2013/07/24 21:45:56, kalman wrote:
> why do you need to cache the master?

I was talking with Evan about adding manifest properties to the into tables.
Exposing the 'master' dictionary (now named 'all_features') is an API for his
code to consume (and any other code that wants access to the manifest features
file).

It is necessary to expose 'all_features' separately because 'apps' and
'extensions' get mutilated pretty badly from their original dictionary form so
they work with the templates.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/server_instance.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/server_instance.py:107:
'/'.join((svn_constants.API_PATH, '_manifest_features.json')))
On 2013/07/24 21:45:56, kalman wrote:
> something funny happened here.

Very strange, removed.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/server2/subversion_file_system.py:7: from future
import Future
I think this is another weird artifact from the trouble I had with git. I
wouldn't move 'our imports' into the 'stdlib imports' block, and duplicate an
import, intentionally. Anyway, reverted.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/json/manifest.json (right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:5: "value": {}
On 2013/07/24 21:45:56, kalman wrote:
> let's make sure for empty object to generate {...} in the documentation not {}
> 
> likewise [...] not [].

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:6: },
On 2013/07/24 21:45:56, kalman wrote:
> also - I think a better way to solve the problem of "example: false" being
> detected as "no example" would be for manifest_data_source to insert a flag
like
> "has_example" for those APIs that have an example.
> 
> I.e. only exposed to templates, no need to specify in here.

I like that a lot more, changed it.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/json/manifest.json:200: "level":
"optional"
On 2013/07/24 21:45:56, kalman wrote:
> let's just make optional the default and apply that to the manifest keys that
> come from _manifest_features.json.

Done.

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/public/extensions/manifest.html
(right):

https://codereview.chromium.org/16410002/diff/38002/chrome/common/extensions/...
chrome/common/extensions/docs/templates/public/extensions/manifest.html:2:
manifest_source:manifest_source.extensions}}
On 2013/07/24 21:45:56, kalman wrote:
> maybe objective-C style would work better here to keep the other file under 80
> characters. that is,
> 
> {{+partials.standard_apps_article article:intros.manifest
>                           manifest_source:manifest_source.apps}}
> 
> maybe that looks weird and
> 
> {{+partials.standard_apps_article
>      article:intros.manifest
>      manifest_source:manifest_source.apps}}
> 
> would be better.

Done.

[not at google - send to devlin, 2013-07-29 23:30:09]

ugh didn't get far, gotta run home for something and will continue there.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_utility.py (right):

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:1: # Copyright 2013
The Chromium Authors. All rights reserved.
actually I think this should be called FeaturesModel (and the file needs to be
features_model.py).

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:5: class
FeatureFile(object):
document what this is, what the keys are, etc.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:6: def __init__(self,
features_json):
can we make this class immutable? it's much easier to reason about immutable
data and how it behaves when cached.

anything which needs to actually mutate state in some way can return a new
feature.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:32: def Filter(self,
platform):
since we may want to filter based on other properties, perhaps platform should
be an optional argument.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:52: def
InsertSubDocs(self):
perhaps the term children rather than subdocs would be all-around a bit less
confusing for me.

"InsertChildren" isn't quite right though, since you're not inserting children,
you're reorganising the features into a parent/child hierarchy rather than a
flat list. good luck with naming that...

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:25: value =
properties[key]
for key, value in properties.items() ?

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:108:
_ConvertAndSortDocs(extensions_features, 'extension'))
if we immutify the FeatureModel class the all this deep-copying should be
unnecessary.

[jshumway, 2013-07-30 23:15:50]

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_utility.py (right):

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:1: # Copyright 2013
The Chromium Authors. All rights reserved.
On 2013/07/29 23:30:09, kalman wrote:
> actually I think this should be called FeaturesModel (and the file needs to be
> features_model.py).

Done.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:5: class
FeatureFile(object):
On 2013/07/29 23:30:09, kalman wrote:
> document what this is, what the keys are, etc.

Done, though I have a little more to add here.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:6: def __init__(self,
features_json):
On 2013/07/29 23:30:09, kalman wrote:
> can we make this class immutable? it's much easier to reason about immutable
> data and how it behaves when cached.
> 
> anything which needs to actually mutate state in some way can return a new
> feature.

Done.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:32: def Filter(self,
platform):
On 2013/07/29 23:30:09, kalman wrote:
> since we may want to filter based on other properties, perhaps platform should
> be an optional argument.

Done.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:52: def
InsertSubDocs(self):
On 2013/07/29 23:30:09, kalman wrote:
> perhaps the term children rather than subdocs would be all-around a bit less
> confusing for me.
> 
> "InsertChildren" isn't quite right though, since you're not inserting
children,
> you're reorganising the features into a parent/child hierarchy rather than a
> flat list. good luck with naming that...

I'll change it here to RestructureChildren, and change the key's name to
'children' here, in manifest data source, and in the templates.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:25: value =
properties[key]
On 2013/07/29 23:30:09, kalman wrote:
> for key, value in properties.items() ?

Done.

https://codereview.chromium.org/16410002/diff/56001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:108:
_ConvertAndSortDocs(extensions_features, 'extension'))
On 2013/07/29 23:30:09, kalman wrote:
> if we immutify the FeatureModel class the all this deep-copying should be
> unnecessary.

Done.

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

nice, just a few structural comments.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_model.py (right):

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:14: def
LoadFeaturesJson(self, features_json):
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.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:56: del features[name]
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)

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:72: return
FeaturesModel(features)
likewise

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

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:74: def
RestructureChildren(self):
I think that RestructureChidlren actually sounds like it mutates the objects,
which it doesnt.

Something like WithNestedChildren might be more appropriate.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:79: features =
deepcopy(self._features)
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.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:108:
_ConvertAndSortDocs(extensions_features, 'extension'))
So we should probably be doing the RestructureChildren thing in manifest data
source, not in the features model.

The data that you pass to the templates generally is not the pure model. You
need to mutate the model in some way (e.g. adding the has_example things). This
child-nesting thing is a specific requirement of manifest_data_source, so it
belongs here, not as part of the pure model.

It's as simple as changing this to

apps_features =
_Annotate(_ConvertAndSortDocs(_WithNestedChildren(features.Filter(platform='app'))))

then again, I'd probably do something similar to what api_data_source does and
have that be a separate object (JSCModel) so that you can cache it nicely. then
do it all in one go.

(you can still test that method individually, though, from
manifest_data_source_test)

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:113:
'all_features': manifest_features
similar to comment above, I don't think this all features thing belongs here,
it's different - apps/extensions are objects that have been mutated for passing
to templates, whereas all_features is the pure model.

I'd make the all_features thing a separate method on this object, like
ManifestDataSource.GetManifestFeatures(), then this method return the
apps/extensions thing like you have.

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

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_model.py (right):

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:14: def
LoadFeaturesJson(self, features_json):
On 2013/07/31 00:36:39, kalman wrote:
> 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.

Done.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:56: del features[name]
On 2013/07/31 00:36:39, kalman wrote:
> 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)

Done.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:72: return
FeaturesModel(features)
On 2013/07/31 00:36:39, kalman wrote:
> likewise
> 
> all that really needs to be different between these is the top-level object.

Done.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:74: def
RestructureChildren(self):
On 2013/07/31 00:36:39, kalman wrote:
> I think that RestructureChidlren actually sounds like it mutates the objects,
> which it doesnt.
> 
> Something like WithNestedChildren might be more appropriate.

See below.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:79: features =
deepcopy(self._features)
On 2013/07/31 00:36:39, kalman wrote:
> 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.

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.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:108:
_ConvertAndSortDocs(extensions_features, 'extension'))
On 2013/07/31 00:36:39, kalman wrote:
> So we should probably be doing the RestructureChildren thing in manifest data
> source, not in the features model.
> 
> The data that you pass to the templates generally is not the pure model. You
> need to mutate the model in some way (e.g. adding the has_example things).
This
> child-nesting thing is a specific requirement of manifest_data_source, so it
> belongs here, not as part of the pure model.
> 
> It's as simple as changing this to
> 
> apps_features =
>
_Annotate(_ConvertAndSortDocs(_WithNestedChildren(features.Filter(platform='app'))))
> 
> then again, I'd probably do something similar to what api_data_source does and
> have that be a separate object (JSCModel) so that you can cache it nicely.
then
> do it all in one go.
> 
> (you can still test that method individually, though, from
> manifest_data_source_test)

Done.

https://codereview.chromium.org/16410002/diff/71001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:113:
'all_features': manifest_features
On 2013/07/31 00:36:39, kalman wrote:
> similar to comment above, I don't think this all features thing belongs here,
> it's different - apps/extensions are objects that have been mutated for
passing
> to templates, whereas all_features is the pure model.
> 
> I'd make the all_features thing a separate method on this object, like
> ManifestDataSource.GetManifestFeatures(), then this method return the
> apps/extensions thing like you have.

Done.

[not at google - send to devlin, 2013-07-31 18:50:19]

sorry I totally missed out that comment about the typed classes.

It's just much better documented to see exactly what properties one would expect
on an object, and to get good errors when you misspell a property name.

class Features(object):
  def __init__(self, name, platforms, ...):
    self.name = name
    self.platforms = platforms
    ...

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_model.py (right):

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:12: self._features =
features
I meant for the features to be typed, but I forgot to mention that. Dealing with
objects is awkward and unsafe; see model.py in tools/json_schema_compiler for
what I meant. Sorry.

It's a small change though. Define another class like Feature at the end, and
have properties "platforms" and "name" plus whatever else you need.

Feature would take all of those in its constructor.

When constructing a new Feature via those Filter and MergeWith methods,
construct the new Feature from the filtered/merged properties, many of which
would be the same.

That would make Get() unnecessary (we won't expose the internals of feature),
instead you could expose methods like Keys/Values/Items which return (or yield)
the keys and/or values of each feature.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:15: def
LoadFeaturesJson(features_json):
prefer FromJson here, it looks better.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:25: if isinstance(value,
list):
needs some comments.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:32: continue
wow else after a for loop looks weird. can you structure it as a list
comprehension or something.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:36: continue
wouldn't need this, it would be implicit

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:9: def
_ConvertAndSortDocs(properties, platform):
"Convert" needs a less generic name (what's it converting it to?)

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:43: def
_Annotate(properties):
"Annotate" needs a less generic name (what's it annotating it with?)

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:134:
manifest_features.Filter(platform='extension').Get()), 'extension'))
pull this repeated code into a little helper function.

def create_for_platform(platform):
  return _Annotate(_ConvertAndSortDocs(_RestructureChildren(
      manifest_features.Filter(platform=platform).Get()), platform)))

return {
  'apps': create_for_platform('apps'),
  'extensions': create_for_platform('extensions'),
}

[jshumway, 2013-08-01 22:27:50]

Here is the manifest patch with the most recent review comments address.
Patchset 10 is most of the comments, patchset 11 is the same as patchset 10, but
with the new Feature class.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_model.py (right):

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:12: self._features =
features
On 2013/07/31 18:50:20, kalman wrote:
> I meant for the features to be typed, but I forgot to mention that. Dealing
with
> objects is awkward and unsafe; see model.py in tools/json_schema_compiler for
> what I meant. Sorry.
> 
> It's a small change though. Define another class like Feature at the end, and
> have properties "platforms" and "name" plus whatever else you need.
> 
> Feature would take all of those in its constructor.
> 
> When constructing a new Feature via those Filter and MergeWith methods,
> construct the new Feature from the filtered/merged properties, many of which
> would be the same.
> 
> That would make Get() unnecessary (we won't expose the internals of feature),
> instead you could expose methods like Keys/Values/Items which return (or
yield)
> the keys and/or values of each feature.

As discussed via email, here is the solution with a new dictionary type that has
properties for 'name' and 'platform'.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:15: def
LoadFeaturesJson(features_json):
On 2013/07/31 18:50:20, kalman wrote:
> prefer FromJson here, it looks better.

Done.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:25: if isinstance(value,
list):
On 2013/07/31 18:50:20, kalman wrote:
> needs some comments.

Done.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:32: continue
On 2013/07/31 18:50:20, kalman wrote:
> wow else after a for loop looks weird. can you structure it as a list
> comprehension or something.

Done.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:36: continue
On 2013/07/31 18:50:20, kalman wrote:
> wouldn't need this, it would be implicit

This still required for the case where value is not a list and contains a
whitelist.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:9: def
_ConvertAndSortDocs(properties, platform):
On 2013/07/31 18:50:20, kalman wrote:
> "Convert" needs a less generic name (what's it converting it to?)

Done.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:43: def
_Annotate(properties):
On 2013/07/31 18:50:20, kalman wrote:
> "Annotate" needs a less generic name (what's it annotating it with?)

Done.

https://codereview.chromium.org/16410002/diff/74001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:134:
manifest_features.Filter(platform='extension').Get()), 'extension'))
On 2013/07/31 18:50:20, kalman wrote:
> pull this repeated code into a little helper function.
> 
> def create_for_platform(platform):
>   return _Annotate(_ConvertAndSortDocs(_RestructureChildren(
>       manifest_features.Filter(platform=platform).Get()), platform)))
> 
> return {
>   'apps': create_for_platform('apps'),
>   'extensions': create_for_platform('extensions'),
> }

Done.

[not at google - send to devlin, 2013-08-05 22:23:48]

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_model.py (right):

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:29: return
len(self._dict)
Ok so now this just looks like a super magical dictionary.

All of the overrides above here... do we really need them? Please say no...

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:31: def __eq__(self,
other):
cos python is crap you need to define __ne__ as well, but it can just be not
(self == other)

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:39: return
'<Feature(%s)>' % self._dict.__repr__()
also override __str__

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:47: self._dict['name'] =
value
features are supposed to be immutable; why do you need a setter?

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:50: def platform(self):
should this be "platforms"?

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:55:
self._dict['platform'] = value
likewise

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:58:
self._dict['platform'].append(value)
likewise

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_model.py:98:
features[name].add_platform('app')
these should be passed into feature's constructor via the dictionary?

extension_types = value.pop(...)
if ...
  platforms.push('extension')
if ...
  platforms.push('app')
features[name] = Feature({
  'name': name,
  'platforms': platforms
})

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_data_source.py (right):

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:39: # Fix the
'name' manifest property's example.
I don't know what this means, can you give some context?

https://codereview.chromium.org/16410002/diff/94001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:86: value.name =
child_name
So IIRC from our email, the purpose of storing the internals of the Feature as a
dict was mostly for easy merging, but also to avoid having to parse out a whole
bunch of things that we'll never need.

I.e. more an implementation detail than an interface detail.

This was one of the use cases, where we were going to return a new Feature based
on firstly mutating some dictionaries, then creating a feature from them like
that.

If Feature had a method like ToDict() then that'd be a starting point.

[jshumway, 2013-08-08 00:32:19]

Some pretty big changes with this one,

features_model renamed features_utility and simplified, methods changed to pure
functions, FeatureModel and Feature removed. Test file simplified.

Cleaned up manifest_data_source.

Better comments all around.

'platform' renamed 'platforms'

[not at google - send to devlin, 2013-08-08 16:31:18]

lg but want to sort out the mutation issues. It should only take a second.

The server flakiness issue is pretty bad, I keep on having to revert to older
versions and it's taking up a lot of time (I'm sheriff at the moment + need to
finish some stuff off by EOW for the branch point), not to mention being a shame
because we can't enable the new cool features.

One of the flaky things it's deciding to do sometimes is not find API nodes in
the templates, implying that something in ApiDataSource is going wrong. It
sounds like a data corruption issue, so either we have some strange bug where we
commit bad data to stores (quite possible), or it's getting mutated badly
somewhere along the way, or both. I can send you server logs if you'd like -
some help in tracking down these problems would be really nice. As I said I
won't have time for a while.

Perhaps we should run an experimental version of the server which freezes
everything that ever comes out of caches; turn sets into a frozenset, turns
lists into tuples, and turns dicts into... well some type which would be a
frozendict. That might help track down these bugs. There are a bunch of places
we cache: CachingFileSystem, CacheChainObjectStore, CompiledFileSystem, and also
the generic memoization method. It shouldn't be too hard to get something
happening here.

Anyway - back to it - hopefully you now understand my nervousness here. There
are some complex interactions that go on in the server.

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron.yaml (right):

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron.yaml:5: target: 2-19-0
19?

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_utility.py (right):

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_utility.py:5: def
FeaturesFromJson(features_json):
let's go with Parse here and Merge below, then call via features.Merge,
features.Parse, features.Merge when we call them. Less tautology is good.

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_utility.py:46: '''
How about this idiom: python has "sort" for in-place modification and "sorted"
for non-in-place. The former doesn't return anything, to be clear, while the
latter obviously does.

Let's either call this Filtered and return a copy, or make it not return itself
so that it's not confusing.

The latter is fine with me I suppose but like I said I don't like mutating data.
If it's an easy enough change can we please do that, and for MergeFeatures.
Callers might forget to deepcopy() and then we have a bug.

The server is currently flakily failing all the time and I don't know what the
cause is, but introducing code that may be flaky (dependent on when appengine
decides to restart our process) is going to make it harder to find.

[jshumway, 2013-08-14 23:38:38]

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/cron.yaml (right):

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/cron.yaml:5: target: 2-19-0
On 2013/08/08 16:31:19, kalman wrote:
> 19?

odd. should be correct now.

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/features_utility.py (right):

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_utility.py:5: def
FeaturesFromJson(features_json):
On 2013/08/08 16:31:19, kalman wrote:
> let's go with Parse here and Merge below, then call via features.Merge,
> features.Parse, features.Merge when we call them. Less tautology is good.

Done.

https://codereview.chromium.org/16410002/diff/110001/chrome/common/extensions...
chrome/common/extensions/docs/server2/features_utility.py:46: '''
On 2013/08/08 16:31:19, kalman wrote:
> How about this idiom: python has "sort" for in-place modification and "sorted"
> for non-in-place. The former doesn't return anything, to be clear, while the
> latter obviously does.
> 
> Let's either call this Filtered and return a copy, or make it not return
itself
> so that it's not confusing.
> 
> The latter is fine with me I suppose but like I said I don't like mutating
data.
> If it's an easy enough change can we please do that, and for MergeFeatures.
> Callers might forget to deepcopy() and then we have a bug.
> 
> The server is currently flakily failing all the time and I don't know what the
> cause is, but introducing code that may be flaky (dependent on when appengine
> decides to restart our process) is going to make it harder to find.

Done.

[not at google - send to devlin, 2013-08-15 00:31:04]

lgtm

[commit-bot: I haz the power, 2013-08-15 00:33:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jaredshumway94@gmail.com/16410002/127001

[commit-bot: I haz the power, 2013-08-15 05:59:57]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-08-15 06:03:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jaredshumway94@gmail.com/16410002/127001

[commit-bot: I haz the power, 2013-08-15 06:05:20]

Change committed as 217753