Docserver: SidenavDataSource refactor, transition to DataSourceRegistry

Docserver: SidenavDataSource refactor, transition to DataSourceRegistry

The next step in moving towards the DataSourceRegistry architecture
(see bug).

The SidenavDataSource is the first DataSource to rely on the contents of
a request and the DataSourceRegistry had to change. Now DataSources are
given both a ServerInstance and a Request when created. Strings and
Manifest DataSources have been updated.

SidenavDataSource contained some very old code and was in need of
refactoring. The Factory had to be stripped out because it is now
unneeded and doesn't work with the registry.

CreateDataSource was moved into the RenderServlet and is added to the
template data source in Create to give the DataSources access to 
a Request.

BUG=275039
NOTRY=true

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

[Jeffrey Yasskin, 2013-08-28 19:12:36]

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/data_source_registry.py:18: for Cron,
|request| should be ommitted.
sp: ommitted -> omitted

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:11: _JSON_FILE_NAME
= '%s/%s_sidenav.json'
This is only used in one place, so it might be better to just inline it.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:15: '''Add a
'level' key to each item in |items|. 'level' corresponds to how deep
Thanks for adding comments.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:26: sidenav can be
expanded to show which menu item has been selected.
Mention what this function returns.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:66: def
_CreateSidenavDict(self, _, content):
It doesn't look like you use self, so can you make this a non-member function?

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source_test.py:97:
logging.disable('warning')
We should test that the warnings are generated ... You could use
logging.getLogger('').addHandler(your_test_handler), and removeHandler after the
test, or you could import a library like
http://pythonhosted.org/testfixtures/logging.html, if we don't already have one.
We have pymock, but I don't see anything specific to logging.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source_test.py:144:
sidenav_data_source.Cron()
Is there anything you can do to assert that Cron() actually loads the data it
should load?

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_data_source.py:56: def
Create(self, request, path, data_sources):
Please document the types of these arguments, and the expected keys and value
types for dictionaries.

[jshumway, 2013-08-29 00:13:09]

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/data_source_registry.py:18: for Cron,
|request| should be ommitted.
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> sp: ommitted -> omitted

Done.

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:11: _JSON_FILE_NAME
= '%s/%s_sidenav.json'
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> This is only used in one place, so it might be better to just inline it.

Done.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:26: sidenav can be
expanded to show which menu item has been selected.
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> Mention what this function returns.

Done.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source.py:66: def
_CreateSidenavDict(self, _, content):
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> It doesn't look like you use self, so can you make this a non-member function?

Done.

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source_test.py:97:
logging.disable('warning')
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> We should test that the warnings are generated ... You could use
> logging.getLogger('').addHandler(your_test_handler), and removeHandler after
the
> test, or you could import a library like
> http://pythonhosted.org/testfixtures/logging.html, if we don't already have
one.
> We have pymock, but I don't see anything specific to logging.

Okay, wasn't too sure what the best way to actually test the logging output was.

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source_test.py:144:
sidenav_data_source.Cron()
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> Is there anything you can do to assert that Cron() actually loads the data it
> should load?

There is a bad way that requires lots of private variable accesses. There is
however precedent for it in my redirector patch
(https://codereview.chromium.org/22217004/diff/28001/chrome/common/extensions/...).

Though looking back at that patch, the only reason we left it like this was to
push the new server version quickly. I am going to add a new method to the
caches to check if a key exists in cache so that these checks don't have to be
as hacky. Added some TODOs.

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/template_data_source.py:56: def
Create(self, request, path, data_sources):
On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> Please document the types of these arguments, and the expected keys and value
> types for dictionaries.

Done.

[not at google - send to devlin, 2013-08-29 16:30:13]

quick drive-by

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

https://codereview.chromium.org/22824042/diff/5001/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/sidenav_data_source_test.py:144:
sidenav_data_source.Cron()
On 2013/08/29 00:13:09, jshumway wrote:
> On 2013/08/28 19:12:37, Jeffrey Yasskin wrote:
> > Is there anything you can do to assert that Cron() actually loads the data
it
> > should load?
> 
> There is a bad way that requires lots of private variable accesses. There is
> however precedent for it in my redirector patch
>
(https://codereview.chromium.org/22217004/diff/28001/chrome/common/extensions/...).
> 
> Though looking back at that patch, the only reason we left it like this was to
> push the new server version quickly. I am going to add a new method to the
> caches to check if a key exists in cache so that these checks don't have to be
> as hacky. Added some TODOs.

We have an integration test which runs the cron job then renders every public
page in its offline state. This ensures that everything gets cached (of course -
there was that bug where the wrong file system was getting used - that would
also be catchable in an integration test come to think of it, by running a
different server).

Point is, private variable access is not a sustainable way to test this, nor a
complete one, and similarly inspecting the contents of the object store is
"slightly better" but still effectively testing an implementation detail of this
class.

Better would be to do a similar thing that the integration test does. For each
test, run the Cron method (you could even put it in SetUp but that may be overly
simplified). Then re-create the class in its offline configuration. So long as
the code coverage is good, this would be a good high-level test of the Cron
functionality.

https://codereview.chromium.org/22824042/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/docs/server2/sidenav_data_source_test.py (right):

https://codereview.chromium.org/22824042/diff/12001/chrome/common/extensions/...
chrome/common/extensions/docs/server2/sidenav_data_source_test.py:18: def
CaptureLogging(f):
this looks useful. could you add it to test_util?

[Jeffrey Yasskin, 2013-08-30 00:05:39]

lgtm aside from kalman's comments.

[jshumway, 2013-08-30 17:05:49]

Moved CaptureLogging to test_util.py, deleted the now unused test_data files.
Talked with Ben about a better DataSource test framework, but we are going to
save that for another patch.

[commit-bot: I haz the power, 2013-08-30 17:06:09]

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

[commit-bot: I haz the power, 2013-08-30 17:17:39]

Change committed as 220600