Remove ReferenceResolver dependency in APIDataSource.

Remove ReferenceResolver dependency in APIDataSource.
This is the first step towards moving APIDataSource to DataSourceRegistry

BUG=275039
NOTRY=true

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

[ahernandez, 2014-03-28 19:04:52]

Could you please take a look at this? I don't want to submit a huge patch for
getting all the crons to work, and this seems like a good first step.

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/api_data_source_test.py (left):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_data_source_test.py:153: def
testFormatDescription(self):
Since we no longer format the description in JSCModel, this test is not needed.

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer.py:33: MAX_REF_LENGTH =
110
It turns out a couple refs are actually longer than 100 characters, so I bumped
the limit a tiny bit.

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer_test.py:119: # There
needs to be more than 110 characters between the invalid ref
WDYT about changing this comment to be a bit more generic? It would be easy to
let it get out of date by specifying the character limit.

[not at google - send to devlin, 2014-03-28 21:48:21]

awesome work. does this change the rendering of anything?

you should also be able to delete the Factory for ReferenceResolver since
nothing actually uses it except for Samples, and that just constructs one right
away. big win.

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

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/api_data_source.py:91: return { 'ref':
link, 'text': link, 'name': link }
Do you know what the difference between "text" and "name" is? We should only
make the "ref" part namespaced. If the tabs API references the "Tab" type, we'd
want that to render as $(ref:tabs.Tab Tab) not $(ref:tabs.Tab tabs.Tab).

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer.py:33: MAX_REF_LENGTH =
110
On 2014/03/28 19:04:52, ahernandez.miralles wrote:
> It turns out a couple refs are actually longer than 100 characters, so I
bumped
> the limit a tiny bit.

yikes, which ones?

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer_test.py:119: # There
needs to be more than 110 characters between the invalid ref
On 2014/03/28 19:04:52, ahernandez.miralles wrote:
> WDYT about changing this comment to be a bit more generic? It would be easy to
> let it get out of date by specifying the character limit.

sgtm

[ahernandez, 2014-03-28 22:03:25]

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer.py:33: MAX_REF_LENGTH =
110
On 2014/03/28 21:48:22, kalman wrote:
> On 2014/03/28 19:04:52, ahernandez.miralles wrote:
> > It turns out a couple refs are actually longer than 100 characters, so I
> bumped
> > the limit a tiny bit.
> 
> yikes, which ones?

fileBrowserHandler.FileHandlerExecuteEventDetails is one example. It only breaks
the character limit when you have
$(ref:fileBrowserHandler.FileHandlerExecuteEventDetails
fileBrowserHandler.FileHandlerExecuteEventDetails)

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer_test.py:119: # There
needs to be more than 110 characters between the invalid ref
On 2014/03/28 21:48:22, kalman wrote:
> On 2014/03/28 19:04:52, ahernandez.miralles wrote:
> > WDYT about changing this comment to be a bit more generic? It would be easy
to
> > let it get out of date by specifying the character limit.
> 
> sgtm

Do you have any suggestions? I was having trouble thinking of something better.

[not at google - send to devlin, 2014-03-28 22:08:43]

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/216943006/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/document_renderer_test.py:119: # There
needs to be more than 110 characters between the invalid ref
On 2014/03/28 22:03:25, ahernandez.miralles wrote:
> On 2014/03/28 21:48:22, kalman wrote:
> > On 2014/03/28 19:04:52, ahernandez.miralles wrote:
> > > WDYT about changing this comment to be a bit more generic? It would be
easy
> to
> > > let it get out of date by specifying the character limit.
> > 
> > sgtm
> 
> Do you have any suggestions? I was having trouble thinking of something
better.

Sure, how about, "DocumentRenderer attempts to detect unclosed $ref() tags by
limiting how far it looks ahead. Lorem Ipsum should be long enough to trigger
that."


http://en.wikipedia.org/wiki/Lorem_ipsum

[ahernandez, 2014-03-28 22:47:30]

PTAL

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:92: return { 'ref':
link, 'text': text, 'name': text }
Is this what we want?

[not at google - send to devlin, 2014-03-28 23:02:17]

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:92: return { 'ref':
link, 'text': text, 'name': text }
On 2014/03/28 22:47:30, ahernandez.miralles wrote:
> Is this what we want?

are you sure that 'ref' shouldn't be 'href'?

the only place I see this stuff used is in ref_link.html and that doesn't care
about a 'name' field, so you might be able to leave this out and see if the
integration tests throw any errors. that said, I'd expect the tests to fail if
that 'href' --> 'ref' change was in error.

in any case I'd structure this code like

href = link if '.' in link else (self._namespace.name + '.' + link)
return { 'href': href, 'text': link }

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:105:
self.api_data_source_factory.Create(None),
It's silly that the ReferenceResolver even needs the API data source at all.
It's being given the API models, and that's all it should need.

Something to fix in a follow-up, should be pretty straighforward.

[ahernandez, 2014-03-28 23:10:55]

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:92: return { 'ref':
link, 'text': text, 'name': text }
On 2014/03/28 23:02:18, kalman wrote:
> On 2014/03/28 22:47:30, ahernandez.miralles wrote:
> > Is this what we want?
> 
> are you sure that 'ref' shouldn't be 'href'?
> 
> the only place I see this stuff used is in ref_link.html and that doesn't care
> about a 'name' field, so you might be able to leave this out and see if the
> integration tests throw any errors. that said, I'd expect the tests to fail if
> that 'href' --> 'ref' change was in error.
> 
> in any case I'd structure this code like
> 
> href = link if '.' in link else (self._namespace.name + '.' + link)
> return { 'href': href, 'text': link }

ReferenceResolver needs the 'name' field for some reason. The reason I renamed
'href' to 'ref' is because ref_link.html now gets rendered into a $(ref:...),
not into a <a href=...></a>.

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/server_instance.py:105:
self.api_data_source_factory.Create(None),
On 2014/03/28 23:02:18, kalman wrote:
> It's silly that the ReferenceResolver even needs the API data source at all.
> It's being given the API models, and that's all it should need.
> 
> Something to fix in a follow-up, should be pretty straighforward.

I think the only reason it needs APIDataSource is because it uses the JSCModel.
Once Jared gets that factored out this should be pretty easy.

[ahernandez, 2014-03-28 23:13:55]

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:92: return { 'ref':
link, 'text': text, 'name': text }
On 2014/03/28 23:10:55, ahernandez.miralles wrote:
> On 2014/03/28 23:02:18, kalman wrote:
> > On 2014/03/28 22:47:30, ahernandez.miralles wrote:
> > > Is this what we want?
> > 
> > are you sure that 'ref' shouldn't be 'href'?
> > 
> > the only place I see this stuff used is in ref_link.html and that doesn't
care
> > about a 'name' field, so you might be able to leave this out and see if the
> > integration tests throw any errors. that said, I'd expect the tests to fail
if
> > that 'href' --> 'ref' change was in error.
> > 
> > in any case I'd structure this code like
> > 
> > href = link if '.' in link else (self._namespace.name + '.' + link)
> > return { 'href': href, 'text': link }
> 
> ReferenceResolver needs the 'name' field for some reason. The reason I renamed
> 'href' to 'ref' is because ref_link.html now gets rendered into a $(ref:...),
> not into a <a href=...></a>.

I'm pretty sure that same link stuff is used here: 

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[not at google - send to devlin, 2014-03-28 23:24:29]

lgtm, could you set a BUG=? should be the DataSourceRegistry one.

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

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:92: return { 'ref':
link, 'text': text, 'name': text }

> 
> I'm pretty sure that same link stuff is used here: 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

I see. Yes that looks like the case. Usually this stuff is just for passing to
the templates, and "name" doesn't get used there, but it looks like that code is
turning links to like "storage.sync.get" to "storage.StorageArea.get". Neat,
though quite hard to read. Accessing into the link structure is a pretty hacky
way of doing that though. You should be able to use 'text' rather than 'name'
there.

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/templates/private/ref_link.html (right):

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/templates/private/ref_link.html:1:
$(ref:{{link.ref}} {{link.text}})
ah I didn't notice this change, nice.

[ahernandez, 2014-03-28 23:27:40]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-03-28 23:28:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/21694300...

[ahernandez, 2014-03-28 23:28:40]

The CQ bit was unchecked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-03-28 23:28:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 23:28:53]

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
7b9ac8b18b49a0b0753c71953fa24802ff91a0d1..db123ace763c04523b6c424e666de9fee8532c0e
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: 3-15-1
+version: 3-16-0
 runtime: python27
 api_version: 1
 threadsafe: false

[not at google - send to devlin, 2014-03-28 23:31:35]

as you rebase, a couple more comments.q

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer.py (right):

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer.py:33: MAX_REF_LENGTH =
110
let's make this... 256.

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer_test.py:120: # how far
it looks ahead. Lorem Ipsum should be long enough to trigger that.
I actually meant to iclude lorem ipsum here, sorry

like

_LOREM_IPSUM = '''
<the contents from wikipedia>
'''

[ahernandez, 2014-03-28 23:34:18]

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/document_renderer_test.py (right):

https://codereview.chromium.org/216943006/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/document_renderer_test.py:120: # how far
it looks ahead. Lorem Ipsum should be long enough to trigger that.
On 2014/03/28 23:31:36, kalman wrote:
> I actually meant to iclude lorem ipsum here, sorry
> 
> like
> 
> _LOREM_IPSUM = '''
> <the contents from wikipedia>
> '''
> 

You just want it as a variable at the top of the test?

[not at google - send to devlin, 2014-03-28 23:36:40]

yep

[ahernandez, 2014-03-29 00:03:50]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-03-29 00:07:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/21694300...

[commit-bot: I haz the power, 2014-03-29 00:12:00]

Change committed as 260316