Docserver: Factor SamplesModel out of SamplesDataSource

Docserver: Factor SamplesModel out of SamplesDataSource

BUG=275039
NOTRY=True

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

[ahernandez, 2014-08-04 20:40:03]

I have moved the last DataSource into DataSourceRegistry. PTAL.

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

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/caching_file_system.py:46: dir_path =
ToDirectory(dir_path)
I had to add a few of these because the path '' was getting transformed into
'/', which is not a valid docserver path.

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

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/samples_data_source.py:75: #
TODO(ahernandez): Implement.
How should I go about implementing this?

[not at google - send to devlin, 2014-08-04 23:00:03]

Moving this review over to Rockot so that you can get a better range of
reviewers.

[Ken Rockot(use gerrit already), 2014-08-05 01:57:21]

lgtm pending Cron impl for SamplesDataSource

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

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/samples_data_source.py:75: #
TODO(ahernandez): Implement.
On 2014/08/04 20:40:03, ahernandez wrote:
> How should I go about implementing this?

I think calling CreateSamplesView for each platform should be sufficient?

[not at google - send to devlin, 2014-08-05 16:04:25]

awesome! so happy!

drive-by with a couple of places you can delete some code (but lgtm otherwise).

for the record, ideally I'd like to see Jared's work completed with pulling out
the samples sources into a configuration file, but this is a big step in the
right direction. and there's not much point trying to go back to Jared's patch
until those github and apps samples issues are fixed.

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

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:90: jsc_view['samples']
= _LazySamplesGetter(
do we still need this _LazySamplesGetter? that was a hack because there was a
mutual dependency between SamplesDataSource and APIDataSource, but now that
you've broken that it shouldn't matter?

i.e. you should be able to replace this with a version of lines 47-49.

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/template_renderer.py (left):

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_renderer.py:51: })
you can delete more code than this. in fact I'd just inline _CreateDataSources
at the call site, it's all-but-1-line.

[ahernandez, 2014-08-05 19:15:54]

I've implemented Cron and addressed the other comments, PTAL.

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

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_renderer.py:41: if
additional_context:
This is a pretty insignificant change, but I couldn't resist doing it while I
was in here.

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

https://codereview.chromium.org/437323003/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:75: jsc_view['samples']
= type('getter', (object,), {
I hope this isn't too hacky, I thought it would be good to keep it lazy.

[not at google - send to devlin, 2014-08-05 19:56:35]

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

https://codereview.chromium.org/437323003/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:75: jsc_view['samples']
= type('getter', (object,), {
On 2014/08/05 19:15:54, ahernandez wrote:
> I hope this isn't too hacky, I thought it would be good to keep it lazy.

ok - fair enough, makes sense. That's pretty hacky :)

you should at least be able to store the SamplesModel locally so it's not so
crazy.

and add a comment.

[not at google - send to devlin, 2014-08-05 19:59:18]

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

https://codereview.chromium.org/437323003/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/server2/template_renderer.py:41: if
additional_context:
On 2014/08/05 19:15:53, ahernandez wrote:
> This is a pretty insignificant change, but I couldn't resist doing it while I
> was in here.

it's actually a bit dangerous doing this, because the {} in the
additional_context={} is global, not scoped to each function. so if you were
ever to modify additional_context - which you don't, but who knows in the future
- that modification would persist. I've seen some very subtle bugs caused by
this behaviour so basically I just always set it to None, though if you *really*
want to inline it you could do

render_context.update(additional_context or {})

[ahernandez, 2014-08-05 21:03:06]

Ken (or Ben, I'm not picky :)), looking for an OK on the Cron
implementation/other changes I've made. PTAL.

[ahernandez, 2014-08-07 17:23:16]

On 2014/08/05 21:03:06, ahernandez wrote:
> Ken (or Ben, I'm not picky :)), looking for an OK on the Cron
> implementation/other changes I've made. PTAL.

Ping. Ken, PTAL.

[Ken Rockot(use gerrit already), 2014-08-07 18:04:31]

On 2014/08/07 17:23:16, ahernandez wrote:
> On 2014/08/05 21:03:06, ahernandez wrote:
> > Ken (or Ben, I'm not picky :)), looking for an OK on the Cron
> > implementation/other changes I've made. PTAL.
> 
> Ping. Ken, PTAL.

Oh, sorry. lgtm

[ahernandez, 2014-08-07 18:17:06]

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

[commit-bot: I haz the power, 2014-08-07 18:23:50]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/43732300...

[commit-bot: I haz the power, 2014-08-07 20:12:11]

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

[commit-bot: I haz the power, 2014-08-07 20:12:12]

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
be9834926d142a565bcf460d27e037927604eaa1..b6ebba166aff7b20114bb6387562b92345473d3f
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-39-0
+version: 3-39-1
 runtime: python27
 api_version: 1
 threadsafe: false

[ahernandez, 2014-08-07 21:08:55]

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

[commit-bot: I haz the power, 2014-08-07 21:21:55]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/43732300...

[commit-bot: I haz the power, 2014-08-07 22:05:21]

Change committed as 288149