chromium-build app now renders console from stored rows.

chromium-build app now renders console from stored rows.

This CL:
* changes the logic in merge_console to construct the merged console from individual stored rows (which are stored by parse_master)
* keeps the old merged console rendering (putting data in ConsoleData, passing it to a template)
* passes all unittests and renders the console properly in dev_appserver mode
* removes the console_merger_utf8 test, as it is covered by test_console_handler_utf8 and test_parse_master
* paves the way for further changes in the next few days to handler.py to add propery query parameter handling


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=176458

[agable, 2012-12-11 08:33:19]

Here it is, the big chromium-app redo. Getting the test data right actually took
way longer than getting the program logic right, but I have a system down for it
and can document it for future use.

Not sure who else should be taking a look at this, please feel free to add
anyone else you think should. I added Ryan just for his experience with
AppEngine in general.

[Do Not Use -- Aaron Gable, 2012-12-19 02:22:49]

On 2012/12/11 08:33:19, agable wrote:
> Here it is, the big chromium-app redo. Getting the test data right actually
took
> way longer than getting the program logic right, but I have a system down for
it
> and can document it for future use.
> 
> Not sure who else should be taking a look at this, please feel free to add
> anyone else you think should. I added Ryan just for his experience with
> AppEngine in general.

Added some tiny changes. Really unclear as to why it took two tries to upload
the patchset (which threw a python error on the first git cl upload, but
apparently uploaded anyway) or why it uploaded all of the files again. The only
real changes in the patchset are three lines in app.py.

[cmp, 2012-12-27 01:19:22]

Aaron, this is good work.  Here are some comments.  Aside from the comments, I'm
missing two things:

- test_console_merger_utf8 :)

- The method moves could have ideally happened in a separate follow-on CL, note
that the diff is made much bigger as a result and harder to read.  In future
CLs, please do moves separate so we can assert that nothing changed within the
methods themselves.

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py
File app.py (left):

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py#oldcode228
app.py:228: master_content = master_content.encode('ascii', 'replace')
I hope that this line being removed and not shuffled somewhere else means it was
redundant (which I was suspicious was the case) but just want to be sure.

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py#oldcode960
app.py:960: content_blob = blobstore.BlobReferenceProperty()
lg

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py#oldcode1029
app.py:1029: content_blob=None)
lg

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py
File app.py (right):

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py#newcode173
app.py:173: logging.info("Saved and cached page with localpath %s" % localpath)
use single quotes everywhere instead of double quotes

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py#newcode255
app.py:255: logging.info("Saved and cached row with localpath %s" % localpath)
use single quotes instead of double quotes

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py
File app_test.py (left):

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py#oldcod...
app_test.py:331: def test_console_merger_utf8(self):
Not sure if you mean to remove test_console_merger_utf8 completely.  That seems
like it may be an accident.

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py
File app_test.py (right):

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py#newcode9
app_test.py:9: import ast
move before line 6

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py#newcod...
app_test.py:299: #     out.write(handled['content'])
I suppose you used this to generate extra data that is in this CL.  We can
either remove these lines or keep them.  If we keep them, let's uncomment them
and name it something besides 'test_' so it's around but not run automatically. 
Some doc lines to describe how it's used would be good, too.

[agable, 2012-12-28 23:56:08]

Responding to comments (some inline).

Re: utf8 test -- given the new data flow of the program,
test_console_merger_utf8 no longer makes sense. console_merger is only ever used
to merge data that has already been put in the datastore... and the only way
data gets into the datastore is if it's already been decoded and unquoted. What
I will do, however, is add a test_parse_master_utf8, which makes much more
sense. That'll be in a patch here shortly.

Re: function reorganization -- thanks for the tip. I thought of that while I was
doing it and wasn't sure what 'best practice' was, but now I know :)

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py
File app.py (right):

https://chromiumcodereview.appspot.com/11535002/diff/28012/app.py#newcode173
app.py:173: logging.info("Saved and cached page with localpath %s" % localpath)
On 2012/12/27 01:19:22, cmp wrote:
> use single quotes everywhere instead of double quotes

Done.

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py
File app_test.py (right):

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py#newcode9
app_test.py:9: import ast
On 2012/12/27 01:19:22, cmp wrote:
> move before line 6

Done.

https://chromiumcodereview.appspot.com/11535002/diff/28012/app_test.py#newcod...
app_test.py:299: #     out.write(handled['content'])
I've decided to remove them. This was helpful for generating some of the inital
test data, but a fuller solution is necessary. I'll put a real
test-data-generator on my back burner and remove this mostly-useless stub.

On 2012/12/27 01:19:22, cmp wrote:
> I suppose you used this to generate extra data that is in this CL.  We can
> either remove these lines or keep them.  If we keep them, let's uncomment them
> and name it something besides 'test_' so it's around but not run
automatically. 
> Some doc lines to describe how it's used would be good, too.

[agable, 2013-01-11 19:31:09]

Added test_parse_master_utf8. This CL is now ready to go live. Going to wrap up
the follow-on so it can go live later today as well.

[cmp, 2013-01-11 19:32:25]

lgtm

[commit-bot: I haz the power, 2013-01-11 22:50:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agable@chromium.org/11535002/35001

[commit-bot: I haz the power, 2013-01-11 22:53:00]

Change committed as 176458