Extensions Docs Server: Generalize $ref's to work for any schema node

Extensions Docs Server: Generalize $ref's to work for any schema node

You can now use a $ref to refer to any top level type, event, method, or
property in an API. The docs server searches through the API to find a match
for the node, and renders the link accordingly.

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

[Aaron Boodman, 2012-10-30 05:48:10]

I'd like to let kalman review this when he returns.

[not at google - send to devlin, 2012-11-01 22:00:49]

Thanks for being patient :)

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/api_data_source.py:59: self._api_list =
api_list_data_source.GetAllNames()
These JSCModel objects are pickled, right? That's what we've made the cache
factories do. So, it seems problematic to me that we're assigning these as
member variables.

It's a bit yuck, but perhaps passing them through ToDict is the only way to go
for now.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/api_data_source.py:387:
return_dict.update(handlebar.ToDict(resolve_refs=self._resolve_refs))
Perhaps you'll need to explain to me what all this resolve_refs stuff is.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/docs_server_utils.py (right):

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:26: def
_FindInAPI(node_name, api):
Find what in API? And what does it return? I was surprised to see that this
returns a string, not the node itself. It makes sense when you look at the
callers, but perhaps a name along the lines of "FindSchemaGroupName" or
"ClassifySchemaNode". "Type" would be appropriate but it's overloaded.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:29: if type_['name']
== node_name:
except the property is "id".

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:35: if event['name']
== node_name:
how about

for name in ['functions', 'events']:
  for item in api.get(name, []):
    ...

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:39: return 'property'
... except "properties" is a dictionary not a list. We might also want to check
it first, since it's cheapest to look up a dictionary and it looks like the most
commonly referenced thing is runtime.lastError.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:40: return None
And also, we need to search deeply through nodes. I see references to things
like "method-StorageArea-clear", and this wouldn't find it.

I'd actually just suggest doing a more generic search through the dictionary,
but I think that would show up false positives. So what you have is fine.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:42: def
GetLinkToRefType(namespace_name, ref_type, api_data_source, api_list):
You're gonna hate me here but now that there's injected dependencies consider
pulling this out into a class like "ReferenceResolver".

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:48: type_name =
_FindInAPI(name, api_data_source[namespace_name])
as I mentioned above, "type" is overloaded in this context and it's rather
confusing. I suggested "group" above but it's not the best name, maybe you can
think of something better.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:61: return { 'href':
'#%s-%s' % (type_name, ref_type), 'text': ref_type }
It seems like the above two conditions could be combined by just stripping the
namespace name if it's there.

if ref_type.startwith('%s.' % namespace_name):
  name = ...
else:
  name = ref_type

?

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:67: api =
api_data_source['.'.join(parts[:i])]
'.'.join(parts[:i]) is repeated, assign to variable api_name?

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:71: type_name =
_FindInAPI(name, api)
again, s/type_name/?/

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:75: 'href':
'%s.html#%s-%s' % ('.'.join(parts[:i]), type_name, name),
re-use api_name here.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/samples_data_source.py:47:
self._api_data_source = api_data_source_factory.Create(None,
Could you pass in a fake request rather than None? and explain why.

[not at google - send to devlin, 2012-11-01 22:05:15]

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/docs_server_utils.py (right):

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:40: return None
> And also, we need to search deeply through nodes. I see references to things
> like "method-StorageArea-clear", and this wouldn't find it.

This part of the comment was made before I read past the bottom. Never mind, I
can see it's handled below.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:49: except
FileNotFoundError:
But speaking of that, this seems like a bug where .s in the names aren't being
handled.

[cduvall, 2012-11-02 01:53:53]

Welcome back!

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

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:59: self._api_list =
api_list_data_source.GetAllNames()
On 2012/11/01 22:00:49, kalman wrote:
> These JSCModel objects are pickled, right? That's what we've made the cache
> factories do. So, it seems problematic to me that we're assigning these as
> member variables.
> 
> It's a bit yuck, but perhaps passing them through ToDict is the only way to go
> for now.

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/api_data_source.py:387:
return_dict.update(handlebar.ToDict(resolve_refs=self._resolve_refs))
On 2012/11/01 22:00:49, kalman wrote:
> Perhaps you'll need to explain to me what all this resolve_refs stuff is.

Done, explained in the Create method above.

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

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:26: def
_FindInAPI(node_name, api):
On 2012/11/01 22:00:49, kalman wrote:
> Find what in API? And what does it return? I was surprised to see that this
> returns a string, not the node itself. It makes sense when you look at the
> callers, but perhaps a name along the lines of "FindSchemaGroupName" or
> "ClassifySchemaNode". "Type" would be appropriate but it's overloaded.

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:29: if type_['name']
== node_name:
On 2012/11/01 22:00:49, kalman wrote:
> except the property is "id".

This has been processed by JSCModel, so all nodes have the 'name' property.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:35: if event['name']
== node_name:
On 2012/11/01 22:00:49, kalman wrote:
> how about
> 
> for name in ['functions', 'events']:
>   for item in api.get(name, []):
>     ...

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:39: return 'property'
On 2012/11/01 22:00:49, kalman wrote:
> ... except "properties" is a dictionary not a list. We might also want to
check
> it first, since it's cheapest to look up a dictionary and it looks like the
most
> commonly referenced thing is runtime.lastError.

This has been processed by JSCModel, so it's a list.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:40: return None
On 2012/11/01 22:05:15, kalman wrote:
> > And also, we need to search deeply through nodes. I see references to things
> > like "method-StorageArea-clear", and this wouldn't find it.
> 
> This part of the comment was made before I read past the bottom. Never mind, I
> can see it's handled below.

I made this more robust so it checks to make sure things like
"method-StorageArea-clear" actually exist.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:42: def
GetLinkToRefType(namespace_name, ref_type, api_data_source, api_list):
On 2012/11/01 22:00:49, kalman wrote:
> You're gonna hate me here but now that there's injected dependencies consider
> pulling this out into a class like "ReferenceResolver".

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:48: type_name =
_FindInAPI(name, api_data_source[namespace_name])
On 2012/11/01 22:00:49, kalman wrote:
> as I mentioned above, "type" is overloaded in this context and it's rather
> confusing. I suggested "group" above but it's not the best name, maybe you can
> think of something better.

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:49: except
FileNotFoundError:
On 2012/11/01 22:05:15, kalman wrote:
> But speaking of that, this seems like a bug where .s in the names aren't being
> handled.

I tried this with refs to app.window.create and links to
storage.StorageArea.get, and it worked for both.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:61: return { 'href':
'#%s-%s' % (type_name, ref_type), 'text': ref_type }
On 2012/11/01 22:00:49, kalman wrote:
> It seems like the above two conditions could be combined by just stripping the
> namespace name if it's there.
> 
> if ref_type.startwith('%s.' % namespace_name):
>   name = ...
> else:
>   name = ref_type
> 
> ?

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:67: api =
api_data_source['.'.join(parts[:i])]
On 2012/11/01 22:00:49, kalman wrote:
> '.'.join(parts[:i]) is repeated, assign to variable api_name?

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:71: type_name =
_FindInAPI(name, api)
On 2012/11/01 22:00:49, kalman wrote:
> again, s/type_name/?/

Done.

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/docs_server_utils.py:75: 'href':
'%s.html#%s-%s' % ('.'.join(parts[:i]), type_name, name),
On 2012/11/01 22:00:49, kalman wrote:
> re-use api_name here.

Done.

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

https://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/server2/samples_data_source.py:47:
self._api_data_source = api_data_source_factory.Create(None,
On 2012/11/01 22:00:49, kalman wrote:
> Could you pass in a fake request rather than None? and explain why.

Done.

[not at google - send to devlin, 2012-11-02 17:26:47]

Cheers :)

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/docs_server_utils.py (right):

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:29: if type_['name']
== node_name:
On 2012/11/02 01:53:53, cduvall wrote:
> On 2012/11/01 22:00:49, kalman wrote:
> > except the property is "id".
> 
> This has been processed by JSCModel, so all nodes have the 'name' property.

Ah, of course.

http://codereview.chromium.org/11315018/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/docs_server_utils.py:49: except
FileNotFoundError:
On 2012/11/02 01:53:53, cduvall wrote:
> On 2012/11/01 22:05:15, kalman wrote:
> > But speaking of that, this seems like a bug where .s in the names aren't
being
> > handled.
> 
> I tried this with refs to app.window.create and links to
> storage.StorageArea.get, and it worked for both.

I meant like, if there's a reference to 'app.window.create' from the 'app' docs,
I don't think this will work? I've mentioned it in the comments for the latest
patch.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:64: def
_GetLinkToRefType(self, ref, ref_resolver):
Rename method similar to rename suggestion in ReferenceResolver.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:66: if ref_data is
None:
nit: invert condition.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:98: def ToDict(self,
ref_resolver):
Consider introducing a DictBuilder class that is constructed with the JSC model
and the ref resolver as its dependencies, rather than needing to thread
ref_resolver everywhere.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:288:
api_list_data_source):
As I mentioned in handler, should be passing the factory here rather than a
concrete class.

If you make a ReferenceResolver.Factory class like I suggest in
SamplesDataSource I believe this problem will go away.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:309: def Create(self,
request, samples_data_source, resolve_refs=True):
It's nice for optional arguments to be False by default. Perhaps disable_refs or
something?

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:309: def Create(self,
request, samples_data_source, resolve_refs=True):
Having a separate SetSamplesDataSourceFactory method on APIDataSource.Factory
(called after its construction), rather than injecting this at creation time, is
a better way to avoid circular dependencies.

Don't forget to assert that it's been set before Create is called, though.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source.py:321: self._ref_resolver
if resolve_refs else None)
nit: push this logic into where ToDict is called (store both ref_resolver and
resolve_refs).

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/api_data_source_test.py:47: 
nit: extra \n

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/handler.py (right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/handler.py:137:
api_list_data_source_factory.Create())
Looks suspicious. Why isn't APIDataSource.Factory creating this in its Create
call?

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/reference_resolver.py:34: def
GetLinkToRefType(self, namespace_name, ref_type):
The "type" seems incorrect now that we're resolving arbitrary references.
Perhaps rename this method to just "GetLink" and "ref_type" to just "ref".

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/reference_resolver.py:41:
self._api_data_source[namespace_name])
General comment: do we need all these __getitem__ stuff any more? What relies on
it?

Either way, better to just use get() here rather than [].

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/reference_resolver_test.py:81:
resolver.GetLinkToRefType('foo', 'bar_p3'))
Yay unit tests. Nice.

Another test you should add is for namespace configurations like app vs
app.window. That is, in addition to having "bar.bon", have a namespace "bar" and
make sure the code knows how to resolve those too.

In addition, it's pretty diabolical, but make sure that if there's an ambiguous
property then it... does something sensible.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/samples_data_source.py:50:
api_list_data_source_factory.Create())
A good clue for when you need to introduce a Factory class for constructing a
dependency is if you need to pass through an argument (here
api_data_source_factory and api_list_data_source_factory) purely to satisfy the
dependency for constructing a different class.

I.e. it's ReferenceResolver.Factory time.

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
File
chrome/common/extensions/docs/server2/test_data/test_json/fake_data_source.json
(right):

http://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/test_data/test_json/fake_data_source.json:24:
"bar.bon": {
As I mentioned, another test would be to have a "bar" namespace too (as you
know, it happens with the "app"/"app.window"/etc namespaces). With different
data obviously.

[cduvall, 2012-11-03 01:30:05]

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:64: def
_GetLinkToRefType(self, ref, ref_resolver):
On 2012/11/02 17:26:48, kalman wrote:
> Rename method similar to rename suggestion in ReferenceResolver.

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:66: if ref_data is
None:
On 2012/11/02 17:26:48, kalman wrote:
> nit: invert condition.

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:98: def ToDict(self,
ref_resolver):
On 2012/11/02 17:26:48, kalman wrote:
> Consider introducing a DictBuilder class that is constructed with the JSC
model
> and the ref resolver as its dependencies, rather than needing to thread
> ref_resolver everywhere.

I redid JSCModel so that it acts as a DictBuilder. Now the whole output of
ToDict is cached, so this should speed things up a little.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:288:
api_list_data_source):
On 2012/11/02 17:26:48, kalman wrote:
> As I mentioned in handler, should be passing the factory here rather than a
> concrete class.
> 
> If you make a ReferenceResolver.Factory class like I suggest in
> SamplesDataSource I believe this problem will go away.

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:309: def Create(self,
request, samples_data_source, resolve_refs=True):
On 2012/11/02 17:26:48, kalman wrote:
> It's nice for optional arguments to be False by default. Perhaps disable_refs
or
> something?

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:309: def Create(self,
request, samples_data_source, resolve_refs=True):
On 2012/11/02 17:26:48, kalman wrote:
> Having a separate SetSamplesDataSourceFactory method on APIDataSource.Factory
> (called after its construction), rather than injecting this at creation time,
is
> a better way to avoid circular dependencies.
> 
> Don't forget to assert that it's been set before Create is called, though.

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source.py:321: self._ref_resolver
if resolve_refs else None)
On 2012/11/02 17:26:48, kalman wrote:
> nit: push this logic into where ToDict is called (store both ref_resolver and
> resolve_refs). 

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/api_data_source_test.py (right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/api_data_source_test.py:47: 
On 2012/11/02 17:26:48, kalman wrote:
> nit: extra \n

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/handler.py (right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/handler.py:137:
api_list_data_source_factory.Create())
On 2012/11/02 17:26:48, kalman wrote:
> Looks suspicious. Why isn't APIDataSource.Factory creating this in its Create
> call?

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/reference_resolver.py:34: def
GetLinkToRefType(self, namespace_name, ref_type):
On 2012/11/02 17:26:48, kalman wrote:
> The "type" seems incorrect now that we're resolving arbitrary references.
> Perhaps rename this method to just "GetLink" and "ref_type" to just "ref".

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/reference_resolver.py:41:
self._api_data_source[namespace_name])
On 2012/11/02 17:26:48, kalman wrote:
> General comment: do we need all these __getitem__ stuff any more? What relies
on
> it?
> 
> Either way, better to just use get() here rather than [].

Took out the __getitem__ stuff.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/reference_resolver_test.py (right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/reference_resolver_test.py:81:
resolver.GetLinkToRefType('foo', 'bar_p3'))
On 2012/11/02 17:26:48, kalman wrote:
> Yay unit tests. Nice.
> 
> Another test you should add is for namespace configurations like app vs
> app.window. That is, in addition to having "bar.bon", have a namespace "bar"
and
> make sure the code knows how to resolve those too.
> 
> In addition, it's pretty diabolical, but make sure that if there's an
ambiguous
> property then it... does something sensible.

Done.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/samples_data_source.py (right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/samples_data_source.py:50:
api_list_data_source_factory.Create())
On 2012/11/02 17:26:48, kalman wrote:
> A good clue for when you need to introduce a Factory class for constructing a
> dependency is if you need to pass through an argument (here
> api_data_source_factory and api_list_data_source_factory) purely to satisfy
the
> dependency for constructing a different class.
> 
> I.e. it's ReferenceResolver.Factory time.

Cool, thanks for the architecture tips they're super helpful.

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
File
chrome/common/extensions/docs/server2/test_data/test_json/fake_data_source.json
(right):

https://codereview.chromium.org/11315018/diff/8017/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/test_data/test_json/fake_data_source.json:24:
"bar.bon": {
On 2012/11/02 17:26:48, kalman wrote:
> As I mentioned, another test would be to have a "bar" namespace too (as you
> know, it happens with the "app"/"app.window"/etc namespaces). With different
> data obviously.

Done.

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:308: if
self._samples_data_source_factory is None:
I have this chunk here because SamplesDataSource needs a ReferenceResolver, and
ReferenceResolver needs an APIDataSource. So we need to be able to make an
APIDataSource without the factory having a SampelsDataSource.Factory.

The same applies below, with ReferenceResolver and APIDataSource being
interdependent.

[not at google - send to devlin, 2012-11-05 19:47:50]

lgtm

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:165: for action in
event.actions]
these can be inlined again?

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:279:
self._json_cache_no_refs = cache_factory.Create(
Nice. Quick comment on why having separate caches with refs vs no refs is
necessary?

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:294:
self._ref_resolver_factory = None
# These must be set later via the SetFooDataSourceFactory methods.

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:16: _VERSION = 2
why does this need to change?

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:56: parts =
ref.split('.')
It would be nice to write it in such a way that the two special cases above
weren't necessary.

Firstly there's nothing particularly wrong with generating an anchor to
"foo.html#blah" from within foo.html itself. It will be equivalent to "#blah".

And secondly perhaps the code below here can be written such that it works if
parts is a singleton list.

Anyway, no biggie if not.

[cduvall, 2012-11-06 00:58:54]

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:165: for action in
event.actions]
On 2012/11/05 19:47:50, kalman wrote:
> these can be inlined again?

Done.

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:279:
self._json_cache_no_refs = cache_factory.Create(
On 2012/11/05 19:47:50, kalman wrote:
> Nice. Quick comment on why having separate caches with refs vs no refs is
> necessary?

Done.

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/api_data_source.py:294:
self._ref_resolver_factory = None
On 2012/11/05 19:47:50, kalman wrote:
> # These must be set later via the SetFooDataSourceFactory methods.

Done.

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/intro_data_source.py:16: _VERSION = 2
On 2012/11/05 19:47:50, kalman wrote:
> why does this need to change?

This is just to satisfy the presubmit check.

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/reference_resolver.py (right):

https://codereview.chromium.org/11315018/diff/16001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/reference_resolver.py:56: parts =
ref.split('.')
On 2012/11/05 19:47:50, kalman wrote:
> It would be nice to write it in such a way that the two special cases above
> weren't necessary.
> 
> Firstly there's nothing particularly wrong with generating an anchor to
> "foo.html#blah" from within foo.html itself. It will be equivalent to "#blah".
> 
> And secondly perhaps the code below here can be written such that it works if
> parts is a singleton list.
> 
> Anyway, no biggie if not.

I ended up needing a special case for $refs not prefixed by the namespace name.
This allows it to handle things like $ref:StorageArea.get (in the storage API).
I can't think of a clean way to eliminate this special case.