Docserver: Consolidate features caching and access.

Docserver: Consolidate features caching and access.

This is just a refactor to reduce some code duplication and homogenize features processing a bit.

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

[Ken Rockot(use gerrit already), 2013-09-19 18:15:39]

PTAL

Note that the additions of devtools.* to _api_features is necessary to get them
listed on the extensions API list. These are the only APIs which should be
listed but which weren't already in _api_features.

[not at google - send to devlin, 2013-09-19 19:21:52]

awesome, glad this is happening; some superficial comments but generally try to
move the feature logic into feature_utility.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_api_features.json:163: "extension_types":
["extension"]
Hopefully this won't cause the devtools tests to fail, they're pretty sensitive.
I guess we'll see.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:46:
file_system.ReadSingle(permission_features_path))
It's preferable to do heavy lifting outside construction of these objects; all
model objects are aggressively cached (file system results etc) so that's fine,
and I'm not 100% confident that thing won't break if we run code during the
construction of a ServerInstance. Even so, best not to.

However, these features also need to be parsed. And indeed, they're being parsed
elsewhere.

Perhaps we should add a class like FeaturesBundle which has parsed features
files for manifest, permissions, and API (manifest using manifest_features, the
others using features_utility). Check out manifest data source; basically pull
that into a separate class. Manifest features need special treatment; I'm not
sure if the others do yet. Not yet, though I think they eventually will.

That will clean up code in a few places: here, manfiest_data_source, and
availablity_finder.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:69: feature_type,
feature_name = dependency.split(':')
This is the sort of stuff that feature_utility c/should handle if it doesn't
already.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:74: if
type(feature) is list:
isinstance(feature, list):

but again feature_utility should handle it.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:93: platforms =
set(['apps', 'extensions'])
when creating fixed-length lists use (...) not [...]. Involves double-parens
here but whatever.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:94: for dependency
in api.get('dependencies', []):
() not []

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:107: return
'chrome'
no else's after return's.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:116: for (name,
api) in self._api_features.items():
name, api not (name, api)

iteritems not items.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:120: for platform
in api_dict.keys():
iterkeys

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

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/server_instance.py:124:
self.manifest_features_path = svn_constants.MANIFEST_FEATURES_PATH
thanks for the cleanup

[Ken Rockot(use gerrit already), 2013-09-20 20:53:37]

I took a pass at refactoring features. No tests written yet and existing tests
are pretty broken now.

ManifestDataSource is no longer Cronning whatever it was doing before - I assume
FeaturesBundle should be doing something similar to that was doing? I have no
idea what's going on.

Anyway, it turns out there are some dependency cycles that make a unified
FeaturesBundle somewhat difficult to manage, so it's not as rosy as I thought it
would be. Still a step forward I think.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:93: platforms =
set(['apps', 'extensions'])
On 2013/09/19 19:21:52, kalman wrote:
> when creating fixed-length lists use (...) not [...]. Involves double-parens
> here but whatever.

Done.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:94: for dependency
in api.get('dependencies', []):
On 2013/09/19 19:21:52, kalman wrote:
> () not []

Done.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:107: return
'chrome'
On 2013/09/19 19:21:52, kalman wrote:
> no else's after return's.

Done.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:116: for (name,
api) in self._api_features.items():
On 2013/09/19 19:21:52, kalman wrote:
> name, api not (name, api)
> 
> iteritems not items.

Done.

https://codereview.chromium.org/23867003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_list_data_source.py:120: for platform
in api_dict.keys():
On 2013/09/19 19:21:52, kalman wrote:
> iterkeys

Done.

[not at google - send to devlin, 2013-09-23 18:31:30]

Right, so two comments:

- I should document this stuff better, but the "data source" classes are
specifically for classes which feed data into templates. this is the top-level
keys in TemplateDataSource. any code which is specifically for rendering pages
should be in data source classes; anything which is pure-model shouldn't; so the
pure-model stuff is what belongs in featureBundle, PermissionsFeatures, etc.

As a side-not we're mid-refactor pulling all of the datasource classes into
DataSourceRegistry to provide a better dependency boundary. it's a bit of a slow
process but we'll get there eventually.

- needs tests :)

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_features.py (right):

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_features.py:15: # No specific
constraints == available on all platforms!
this... should never happen. but what *can* happen here is an 'all' keyword I
think.

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features.py (right):

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features.py:26: def Parse(self,
features_json):
I don't quite understand the inheritance structure here. why can't Feature
basically implement everything and each of the *Feature classes provide
functionality on top of that? The overriding behaviour of
Parse/FilterByPLatform/CreateCache is confusing. Particularly since you've
implemented it here and then reimplemented it in the other places.

and rather than the magical get('all'), get('apps'), get('extensions'), have
real methods.

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:32:
self._features['permission'])
storing these in a dict seems unnecessary. just have 3 explicit members and
instead of a "get" method access via "manifest", "permission", "api".

If this were C++ you'd have methods manifest(), permission(), and api() but I've
heard that's not pythonic (but neither is pythonic a word).

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:34: def
AnnotateWithTemplateData(self, template_data_source_factory):
Yeah it's a shame we need this. I don't think we will for long, but, it's
probably unavoidable at the moment. We should
(1) move the data-source population code back into PermisiosnsDataSource. PDS
still depends on PermissionFeatures though.
(2) break the dependency of PermissionsDataSource on TemplateDataSource.

All of which we can do as some later point. Well... see my other comments; maybe
the Registrar change (if you choose to do it now) will actually make a
difference here.

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/manifest_features.py (right):

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_features.py:68: def
_ListifyAndSortDocs(features, app_name):
most of this code is specifically for passing to the manifest.html pages and as
such should be in manifest_data_source.py.

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/permission_features.py (right):

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/permission_features.py:12: def
_ListifyPermissions(permissions):
this should be in the permission_data_source since it's specifically for
populating the 'permissions' template field. same goes for any other data for
that purpose. The pure-model PermissionFeatures stuff (which filters by
extension type etc) should be in here.

Which brings me to another point - we're mid-refactor in moving the data source
into data_source_registry. I think you might be able to do this for
permissions_data_source now and save a bit of effort. If not - that's cool, we
can do it later.

https://codereview.chromium.org/23867003/diff/13001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/permission_features.py:76: }
so... this overrides CreateCache from Features?

if you move the listification into api_data_source hopefully that's not
necessary, because it's a bit surprising.

[Ken Rockot(use gerrit already), 2013-10-01 02:12:21]

Much simpler approach here; features are once again plain old dicts, now managed
directly by FeaturesBundle. No Features or stupid inheritance structure.

All existing tests updated - no features_bundle_test yet.

[not at google - send to devlin, 2013-10-01 16:34:33]

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:60: return
sorted(map(lambda name: name.replace('_', '.'), template_names))
see comments below (working my way from bottom to top in this file for some
reason) about sorting. shouldn't be necessary.

also: prefer list comprehension to map unless there's a really good reason not
to.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:74: public_apis =
self._cache.GetFromFileListing(self._public_template_path)
this "public api" naming confuses me because it look like you're also including
private APIs in that. it's something like "GetDocumentedAPIs" or something?
"GetAPIsWithPublicDocumentation"?

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:76: for platform
in api_dict.iterkeys():
to cut down on a level of nesting here you could write above

api_dict = {
  'apps': MakeDictForPlatform('apps'),
  'extensions': MakeDictForPlatform('extensions'),
}

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:78: if platform in
api['platforms']:
platform in api['platforms'] or 'all' in api['platforms']

or

any(key in api['platforms'] for key in (platform, 'all'))

? hey they're exactly the same length, funny.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:81: for platform
in api_dict.iterkeys():
maybe I'm tired; but this inner loop is just to sort the lists, right? and
attach the 'last' to it?

I wouldn't worry about sorting it, just show in order that the features file
presents (... works so long as it uses an OrderedDict, which, it should). and
can you do the 'last' thing in the loop beforehand?

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:86:
apis[-1]['last'] = True
bleh this is so common. WDYT about adding a method to doc_server_utils like
"MarkLast" or something.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:102: return
sorted(api['name'] for api in apis)
likewise, don't worry about sorting it.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:107: return
self._apis.get(key)
it looks like it's ok to be using an ObjectStore to cache this rather than an
ad-hoc in-memory cache. Using an ObjectStore to cache this will let other
instances benefit from this work (ObjectStore also does in-memory caching).

the times when it's not ok to use an ObjectStore: if the data relies on
per-instance data, such as the request parameters. but that's pretty rare.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:13: def __init__(self,
file_system, compiled_fs_factory, json_paths):
this json_paths thing is only necessary to support that features_file_overrides
case, right? so when you delete that you can simplify this code as well.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:37: '''Provides access
to properties of API, Manifest, and Permission features.'''
'''Foo
'''

not

'''Foo'''.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:43: features_files =
features_files_override
rather than this, create a FakeFeaturesBundle class which stubs out
Get{Permission,Manifest,API}Features.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:73: def
GetAPIFeatures(self, force_refresh=False):
doesn't look like force_refresh is used.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:82:
features_utility.AddPlatformsFromDependencies(feature, self)
AddPlatformsFromDependencies is only used in this file. could you put it in this
file util classes are [an unfortunately occasionally useful] evil.

then you might be able to avoid the GetManifestFeatures/GetPermissionFeatures
caching weirdness.

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

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:82: def
GetPlatformsForExtensionTypes(extension_types):
this can be private?

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:90: def
AddPlatformsFromDependencies(feature, features_bundle):
as I mentioned earlier - you can put this in features_bundle.py.

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

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:125: return
self._cache.get(key)
you could also consider doing this with an ObjectStore,and maybe separate out
the apps and extensions into separate keys if you want to have more fine-grained
caching; though, not necessary. simple enough as it is. though that said the old
way of using a CompiledFileSystem seems simpler.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/permissions_data_source.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/permissions_data_source.py:81: return
self._cache.get(key)
same comment as in manifest_data_source

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source_test.py:47:
LocalFileSystem.Create(), _FakeFactory(), {})
you would use the FakeFeatureBundle() class here.

[Ken Rockot(use gerrit already), 2013-10-01 22:18:15]

Cleanup, FeaturesBundle tests, and ObjectStore replaces in-memory caches in
{APIList,Manifest,Permissions}DataSource.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:60: return
sorted(map(lambda name: name.replace('_', '.'), template_names))
On 2013/10/01 16:34:34, kalman wrote:
> see comments below (working my way from bottom to top in this file for some
> reason) about sorting. shouldn't be necessary.
> 
> also: prefer list comprehension to map unless there's a really good reason not
> to.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:74: public_apis =
self._cache.GetFromFileListing(self._public_template_path)
On 2013/10/01 16:34:34, kalman wrote:
> this "public api" naming confuses me because it look like you're also
including
> private APIs in that. it's something like "GetDocumentedAPIs" or something?
> "GetAPIsWithPublicDocumentation"?

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:76: for platform
in api_dict.iterkeys():
On 2013/10/01 16:34:34, kalman wrote:
> to cut down on a level of nesting here you could write above
> 
> api_dict = {
>   'apps': MakeDictForPlatform('apps'),
>   'extensions': MakeDictForPlatform('extensions'),
> }

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:78: if platform in
api['platforms']:
I think you're thinking of extension_types. platforms is always a list over the
set {'apps', 'extensions'}

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:81: for platform
in api_dict.iterkeys():
The sorting is actually enforced by tests. It looks like the order of these
lists determines their order on API index pages.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:86:
apis[-1]['last'] = True
On 2013/10/01 16:34:34, kalman wrote:
> bleh this is so common. WDYT about adding a method to doc_server_utils like
> "MarkLast" or something.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:102: return
sorted(api['name'] for api in apis)
On 2013/10/01 16:34:34, kalman wrote:
> likewise, don't worry about sorting it.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_list_data_source.py:107: return
self._apis.get(key)
On 2013/10/01 16:34:34, kalman wrote:
> it looks like it's ok to be using an ObjectStore to cache this rather than an
> ad-hoc in-memory cache. Using an ObjectStore to cache this will let other
> instances benefit from this work (ObjectStore also does in-memory caching).
> 
> the times when it's not ok to use an ObjectStore: if the data relies on
> per-instance data, such as the request parameters. but that's pretty rare.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/features_bundle.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:13: def __init__(self,
file_system, compiled_fs_factory, json_paths):
On 2013/10/01 16:34:34, kalman wrote:
> this json_paths thing is only necessary to support that
features_file_overrides
> case, right? so when you delete that you can simplify this code as well.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:37: '''Provides access
to properties of API, Manifest, and Permission features.'''
On 2013/10/01 16:34:34, kalman wrote:
> '''Foo
> '''
> 
> not
> 
> '''Foo'''.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:43: features_files =
features_files_override
On 2013/10/01 16:34:34, kalman wrote:
> rather than this, create a FakeFeaturesBundle class which stubs out
> Get{Permission,Manifest,API}Features.

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:73: def
GetAPIFeatures(self, force_refresh=False):
Oops. Was going to be used as a Cron hack. Shouldn't be necessary.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_bundle.py:82:
features_utility.AddPlatformsFromDependencies(feature, self)
On 2013/10/01 16:34:34, kalman wrote:
> AddPlatformsFromDependencies is only used in this file. could you put it in
this
> file util classes are [an unfortunately occasionally useful] evil.
> 
> then you might be able to avoid the GetManifestFeatures/GetPermissionFeatures
> caching weirdness.

Done, and on that note features_utility.Filtered is now the only
features_utility function used outside of FeaturesBundle. Might (should) be able
to get rid of it eventually.

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

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:82: def
GetPlatformsForExtensionTypes(extension_types):
On 2013/10/01 16:34:34, kalman wrote:
> this can be private?

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/features_utility.py:90: def
AddPlatformsFromDependencies(feature, features_bundle):
On 2013/10/01 16:34:34, kalman wrote:
> as I mentioned earlier - you can put this in features_bundle.py.

Done.

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

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/manifest_data_source.py:125: return
self._cache.get(key)
On 2013/10/01 16:34:34, kalman wrote:
> you could also consider doing this with an ObjectStore,and maybe separate out
> the apps and extensions into separate keys if you want to have more
fine-grained
> caching; though, not necessary. simple enough as it is. though that said the
old
> way of using a CompiledFileSystem seems simpler.

Done, minus fine-grained caching.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/permissions_data_source.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/permissions_data_source.py:81: return
self._cache.get(key)
On 2013/10/01 16:34:34, kalman wrote:
> same comment as in manifest_data_source

Done.

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/template_data_source_test.py (right):

https://codereview.chromium.org/23867003/diff/26001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/template_data_source_test.py:47:
LocalFileSystem.Create(), _FakeFactory(), {})
On 2013/10/01 16:34:34, kalman wrote:
> you would use the FakeFeatureBundle() class here.

Done.

[not at google - send to devlin, 2013-10-01 23:09:33]

lgtm

[commit-bot: I haz the power, 2013-10-01 23:33:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/23867003/56001

[commit-bot: I haz the power, 2013-10-01 23:33:26]

Failed to apply patch for chrome/common/extensions/docs/server2/app.yaml:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/docs/server2/app.yaml
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/docs/server2/app.yaml.rej

Patch:       chrome/common/extensions/docs/server2/app.yaml
Index: chrome/common/extensions/docs/server2/app.yaml
diff --git a/chrome/common/extensions/docs/server2/app.yaml
b/chrome/common/extensions/docs/server2/app.yaml
index
94ca06cad1d209bc9044326551699282de9606b3..30e7ec03368e9ba7a5bd4715a344e5790b906183
100644
--- a/chrome/common/extensions/docs/server2/app.yaml
+++ b/chrome/common/extensions/docs/server2/app.yaml
@@ -1,5 +1,5 @@
 application: chrome-apps-doc
-version: 2-28-4
+version: 2-28-5
 runtime: python27
 api_version: 1
 threadsafe: false

[commit-bot: I haz the power, 2013-10-01 23:39:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/23867003/74001

[commit-bot: I haz the power, 2013-10-02 00:04:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-10-02 04:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/23867003/83001

[commit-bot: I haz the power, 2013-10-02 04:11:57]

Failed to apply patch for chrome/common/extensions/docs/server2/app.yaml:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/extensions/docs/server2/app.yaml
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/extensions/docs/server2/app.yaml.rej

Patch:       chrome/common/extensions/docs/server2/app.yaml
Index: chrome/common/extensions/docs/server2/app.yaml
diff --git a/chrome/common/extensions/docs/server2/app.yaml
b/chrome/common/extensions/docs/server2/app.yaml
index
30e7ec03368e9ba7a5bd4715a344e5790b906183..0cc778c23f21aa4ac934aad12c4be82eb61ec17d
100644
--- a/chrome/common/extensions/docs/server2/app.yaml
+++ b/chrome/common/extensions/docs/server2/app.yaml
@@ -1,5 +1,5 @@
 application: chrome-apps-doc
-version: 2-28-5
+version: 2-28-6
 runtime: python27
 api_version: 1
 threadsafe: false

[commit-bot: I haz the power, 2013-10-02 04:14:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/23867003/87001

[commit-bot: I haz the power, 2013-10-02 04:40:31]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-10-02 04:54:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rockot@chromium.org/23867003/87001

[commit-bot: I haz the power, 2013-10-02 07:20:21]

Change committed as 226413