Upload file to app engine and generate public url for dmprof visualizer.

Upload file to app engine and generate public url for dmprof visualizer.

User is able to upload their local dmprof file to app engine. A public url will
be generated for the uploaded file to show the breakdown with default template.
Everyone can see it by generated url.

Because Entity in ndb of app engine only permits 1MB object to be stored.
Store json file in blobstore and file reference in profiler entity.

BUG=259206
NOTRY=True

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

[junjianx, 2013-09-17 11:07:24]

I finished file uploading and public url generating by app engine. Please review
my code, thanks :)

[sullivan, 2013-09-17 14:58:47]

This code could really use some unit tests! App engine has an excellent unit
testing framework which provides local stubs for webapp2, datastore, etc, so
that you can actually do get(), post(), inspect the datastore, etc in your
tests:

https://developers.google.com/appengine/docs/python/tools/localunittesting

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:25: blob_str = ndb.StringProperty()
Why not ndb.BlobKeyProperty?

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:31: content = ndb.PickleProperty()
ndb.JsonProperty?

Also, isn't this redundant with json.loads(key.string_id())?

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:32: tags =
ndb.StringProperty(repeated=True)
Where are tags used?

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:52: prof_key =
ndb.Key(urlsafe=prof_id)
It looks like it's also possible to construct a Profiler from a run_id? Not sure
if using that instead of the urlsafe form of the key would make it more
human-readable?

Also, I don't think it matters in this case, but just to be safe: It's possible
to decode the urlsafe key and easily discover the Kind and id:
https://developers.google.com/appengine/docs/python/ndb/keyclass#Key_urlsafe
This means that it would be possible for a user to modify the url to see what
profiles are in the data store. I don't see how that could harm anything, but if
they're meant to be private to the person who uploaded the profile, they're not.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:82: profiler.put()
Did you mean to have a hierarchy of Profile objects? It looks like what you
meant to do is have one Profile per run_id, all with no parents. That would look
like this:

prof_key = ndb.Key('Profiler', run_id)
if not prof_key.get()
  # Profile for this run_id does not exist
  profiler = Profiler(id=run_id, blob_key=blob_info.key())
  profiler.put()

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:94: template.put()
It looks like the templates are already stored in the json that goes into
blobstore. So why are they stored in the datastore as well? Any reason not to
just pull the template from the JSON in the client-side javascript, or in the
get()

Why can there be multiple templates in json_data? And is default_template
required? Looks like it fails silently if default_template is not set, and then
in get() you access the content on line 62 and it will raise an exception
accessing .content of None.

And why is there a hierarchy like there is for profile? I think you don't want
to set parents for the templates, either.

And is there a limit on the content length? I can't find the documentation on
max key length, but you probably don't want it to be more than 200 characters or
so.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/index.html (right):

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/index.html:23: var profiler = new
Profiler({{ json }}, {{ template }});
{{ json }} and {{ template }} will be html-escaped by default; I think you
probably want {{ json|safe }} and {{ template|safe }} since these are json,
right?

[junjianx, 2013-09-18 02:15:03]

On 2013/09/17 14:58:47, sullivan wrote:
> This code could really use some unit tests! App engine has an excellent unit
> testing framework which provides local stubs for webapp2, datastore, etc, so
> that you can actually do get(), post(), inspect the datastore, etc in your
> tests:
> 
> https://developers.google.com/appengine/docs/python/tools/localunittesting
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> File tools/deep_memory_profiler/visualizer/app.py (right):
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/app.py:25: blob_str =
ndb.StringProperty()
> Why not ndb.BlobKeyProperty?
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/app.py:31: content =
ndb.PickleProperty()
> ndb.JsonProperty?
> 
> Also, isn't this redundant with json.loads(key.string_id())?
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/app.py:32: tags =
> ndb.StringProperty(repeated=True)
> Where are tags used?
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/app.py:52: prof_key =
> ndb.Key(urlsafe=prof_id)
> It looks like it's also possible to construct a Profiler from a run_id? Not
sure
> if using that instead of the urlsafe form of the key would make it more
> human-readable?
> 
> Also, I don't think it matters in this case, but just to be safe: It's
possible
> to decode the urlsafe key and easily discover the Kind and id:
> https://developers.google.com/appengine/docs/python/ndb/keyclass#Key_urlsafe
> This means that it would be possible for a user to modify the url to see what
> profiles are in the data store. I don't see how that could harm anything, but
if
> they're meant to be private to the person who uploaded the profile, they're
not.
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/app.py:82: profiler.put()
> Did you mean to have a hierarchy of Profile objects? It looks like what you
> meant to do is have one Profile per run_id, all with no parents. That would
look
> like this:
> 
> prof_key = ndb.Key('Profiler', run_id)
> if not prof_key.get()
>   # Profile for this run_id does not exist
>   profiler = Profiler(id=run_id, blob_key=blob_info.key())
>   profiler.put()
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/app.py:94: template.put()
> It looks like the templates are already stored in the json that goes into
> blobstore. So why are they stored in the datastore as well? Any reason not to
> just pull the template from the JSON in the client-side javascript, or in the
> get()
> 
> Why can there be multiple templates in json_data? And is default_template
> required? Looks like it fails silently if default_template is not set, and
then
> in get() you access the content on line 62 and it will raise an exception
> accessing .content of None.
> 
> And why is there a hierarchy like there is for profile? I think you don't want
> to set parents for the templates, either.
> 
> And is there a limit on the content length? I can't find the documentation on
> max key length, but you probably don't want it to be more than 200 characters
or
> so.
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> File tools/deep_memory_profiler/visualizer/index.html (right):
> 
>
https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
> tools/deep_memory_profiler/visualizer/index.html:23: var profiler = new
> Profiler({{ json }}, {{ template }});
> {{ json }} and {{ template }} will be html-escaped by default; I think you
> probably want {{ json|safe }} and {{ template|safe }} since these are json,
> right?

Really appreciate your review with so many help suggestions :)

[junjianx, 2013-09-20 06:31:08]

To write testable code, I separated business logic layer and Models to
services.py. After that, I added unit test for services.py.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:25: blob_str = ndb.StringProperty()
On 2013/09/17 14:58:47, sullivan wrote:
> Why not ndb.BlobKeyProperty?

Done.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:31: content = ndb.PickleProperty()
I replaced PickleProperty with JsonProperty.
But it must be converted to string using json.dumps when constructing ndb.Key
and being passed to jinja2 template. I don't know more elegant method.

On 2013/09/17 14:58:47, sullivan wrote:
> ndb.JsonProperty?
> 
> Also, isn't this redundant with json.loads(key.string_id())?

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:32: tags =
ndb.StringProperty(repeated=True)
Tag will be used in next issue to help user managing templates, maybe I
shouldn't include them in this issue. Removed relative code.

On 2013/09/17 14:58:47, sullivan wrote:
> Where are tags used?

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:52: prof_key =
ndb.Key(urlsafe=prof_id)
On 2013/09/17 14:58:47, sullivan wrote:
> It looks like it's also possible to construct a Profiler from a run_id? Not
sure
> if using that instead of the urlsafe form of the key would make it more
> human-readable?
> 
> Also, I don't think it matters in this case, but just to be safe: It's
possible
> to decode the urlsafe key and easily discover the Kind and id:
> https://developers.google.com/appengine/docs/python/ndb/keyclass#Key_urlsafe
> This means that it would be possible for a user to modify the url to see what
> profiles are in the data store. I don't see how that could harm anything, but
if
> they're meant to be private to the person who uploaded the profile, they're
not.

Done.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:82: profiler.put()
On 2013/09/17 14:58:47, sullivan wrote:
> Did you mean to have a hierarchy of Profile objects? It looks like what you
> meant to do is have one Profile per run_id, all with no parents. That would
look
> like this:
> 
> prof_key = ndb.Key('Profiler', run_id)
> if not prof_key.get()
>   # Profile for this run_id does not exist
>   profiler = Profiler(id=run_id, blob_key=blob_info.key())
>   profiler.put()

Done.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/app.py:94: template.put()
Template represents one category of breakdown. Template doesn't bind with
profiler. One template can be used to breakdown any profiler if possible.
User can create his own template (He maybe found certain breakdown graph very
interesting) and share it with others.
This is the reason we manage profiler and templates separately in database.

I think default_template should be a required attribute in json.

And we plan to add template manager in the future. User can search existing
template and add a new one.

On 2013/09/17 14:58:47, sullivan wrote:
> It looks like the templates are already stored in the json that goes into
> blobstore. So why are they stored in the datastore as well? Any reason not to
> just pull the template from the JSON in the client-side javascript, or in the
> get()
> 
> Why can there be multiple templates in json_data? And is default_template
> required? Looks like it fails silently if default_template is not set, and
then
> in get() you access the content on line 62 and it will raise an exception
> accessing .content of None.
> 
> And why is there a hierarchy like there is for profile? I think you don't want
> to set parents for the templates, either.
> 
> And is there a limit on the content length? I can't find the documentation on
> max key length, but you probably don't want it to be more than 200 characters
or
> so.

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/index.html (right):

https://codereview.chromium.org/23781012/diff/5001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/index.html:23: var profiler = new
Profiler({{ json }}, {{ template }});
On 2013/09/17 14:58:47, sullivan wrote:
> {{ json }} and {{ template }} will be html-escaped by default; I think you
> probably want {{ json|safe }} and {{ template|safe }} since these are json,
> right?

Done.

[sullivan, 2013-09-20 13:59:50]

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:51: default_key =
services.CreateTemplates(blob_info)
Reading the code, right now I think if there is no default_key, the Profile and
Templates will be created, and then services.CreateTemplates will raise an
exception and the user will get a 500 error. It would probably be better to give
the user a helpful response in this case. Also, should any data be stored in the
datastore if there is no default_key (probably a sign that there's something
wrong with the data, isn't it?)

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app_unittest.py (right):

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app_unittest.py:48:
self.assertEqual(services.Template.query().count(), 2)
Might want to do some testing of the content of the Templates to make sure
they're correct.

Also might want to add a test to check what happens if there is no
default_template.

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/services.py (right):

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/services.py:63: return default_key
default_key doesn't get initialized, so won't you get an UnboundLocalError if
there is no default_key in the JSON? Wouldn't it be better to return None here,
and then present a helpful error message to the user?

[junjianx, 2013-09-24 05:53:53]

I added validation for default_template attribute in main logic.

In the future, file validation should be done separately from main logic. And
error message should also be shown in a single error page. (shown in main page
previously) User can redirect to home page from there.

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:51: default_key =
services.CreateTemplates(blob_info)
On 2013/09/20 13:59:51, sullivan wrote:
> Reading the code, right now I think if there is no default_key, the Profile
and
> Templates will be created, and then services.CreateTemplates will raise an
> exception and the user will get a 500 error. It would probably be better to
give
> the user a helpful response in this case. Also, should any data be stored in
the
> datastore if there is no default_key (probably a sign that there's something
> wrong with the data, isn't it?)

Done.
I think validation of uploaded file should be done separately. Temporarily, only
default_template attribute is validated in main logic.

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app_unittest.py (right):

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app_unittest.py:48:
self.assertEqual(services.Template.query().count(), 2)
On 2013/09/20 13:59:51, sullivan wrote:
> Might want to do some testing of the content of the Templates to make sure
> they're correct.
> 
> Also might want to add a test to check what happens if there is no
> default_template.

Done.

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/services.py (right):

https://codereview.chromium.org/23781012/diff/12001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/services.py:63: return default_key
On 2013/09/20 13:59:51, sullivan wrote:
> default_key doesn't get initialized, so won't you get an UnboundLocalError if
> there is no default_key in the JSON? Wouldn't it be better to return None
here,
> and then present a helpful error message to the user?

Done.

[Dai Mikurube (NOT FULLTIME), 2013-09-24 07:22:11]

The code basically looks good (while I might overlook some AppEngine-specific
things). Some comments.

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app_unittest.py (right):

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app_unittest.py:6: import services
Better to split standard libraries and your libraries by an empty line.

import json
...

from google.app...
...

import services

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/run_tests.py (right):

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/run_tests.py:10: 
One more empty line.

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/run_tests.py:28: if __name__ ==
'__main__':
This clause should be simpler, just calling main() and a very simple
initialization is fine. See dmprof.py.

An option is moving things to main().

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/testdata/sample.json (right):

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/testdata/sample.json:1: {
This test data looks too big. Could you make a more focused sample data like
error_sample.json?

[junjianx, 2013-09-24 08:11:20]

I fixed review problems indicated by @Dai.

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app_unittest.py (right):

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app_unittest.py:6: import services
On 2013/09/24 07:22:11, Dai Mikurube wrote:
> Better to split standard libraries and your libraries by an empty line.
> 
> import json
> ...
> 
> from google.app...
> ...
> 
> import services

Done.

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/run_tests.py (right):

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/run_tests.py:10: 
On 2013/09/24 07:22:11, Dai Mikurube wrote:
> One more empty line.

Done.

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/run_tests.py:28: if __name__ ==
'__main__':
On 2013/09/24 07:22:11, Dai Mikurube wrote:
> This clause should be simpler, just calling main() and a very simple
> initialization is fine. See dmprof.py.
> 
> An option is moving things to main().

Done.

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/testdata/sample.json (right):

https://codereview.chromium.org/23781012/diff/26001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/testdata/sample.json:1: {
On 2013/09/24 07:22:11, Dai Mikurube wrote:
> This test data looks too big. Could you make a more focused sample data like
> error_sample.json?

Done.

[Dai Mikurube (NOT FULLTIME), 2013-09-24 09:15:15]

Some more comments.

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:40: template_values['template'] =
'null'
The strings 'null' might be confusing. How are these strings used finally?

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:41: 
Why this empty line?

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:42:
self.response.write(page_template.render(template_values))
It's the same as below. Can we move it out of if-else?

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/testdata/sample.json (right):

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/testdata/sample.json:13: "run_id":
"jxu-linux-20191-1376364201",
Wow, it doesn't have any snapshot. And, any difference from error_sample.json?

[junjianx, 2013-09-24 09:46:43]

I fixed review problems indicated by @Dai.

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:40: template_values['template'] =
'null'
On 2013/09/24 09:15:15, Dai Mikurube wrote:
> The strings 'null' might be confusing. How are these strings used finally?

Done.

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:41: 
On 2013/09/24 09:15:15, Dai Mikurube wrote:
> Why this empty line?

Done.

https://codereview.chromium.org/23781012/diff/36001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:42:
self.response.write(page_template.render(template_values))
On 2013/09/24 09:15:15, Dai Mikurube wrote:
> It's the same as below. Can we move it out of if-else?

Done.

[Dai Mikurube (NOT FULLTIME), 2013-09-24 10:13:09]

Two more comments.

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:38: if run_id and tmpl_id:
It looks you added 'null' ones in Patch Set 5. Weren't they required ones? My
comment meant that the string 'null' doesn't have no meaning.

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/testdata/sample.json (right):

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/testdata/sample.json:13: "run_id":
"jxu-linux-20191-1376364201",
How about my comment on this file previously?

[junjianx, 2013-09-24 10:25:19]

Just replied Dai's comments.

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:38: if run_id and tmpl_id:
On 2013/09/24 10:13:09, Dai Mikurube wrote:
> It looks you added 'null' ones in Patch Set 5. Weren't they required ones? My
> comment meant that the string 'null' doesn't have no meaning.

I moved verification of these two variables to the index.html template.
I think it looks more elegant.

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/testdata/sample.json (right):

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/testdata/sample.json:13: "run_id":
"jxu-linux-20191-1376364201",
On 2013/09/24 10:13:09, Dai Mikurube wrote:
> How about my comment on this file previously?

Sorry for neglecting last comment.
error_sample.json has no default_template attribute.

[sullivan, 2013-09-24 12:29:20]

lgtm

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/app.py (right):

https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/app.py:59: 'upload_msg': 'No default key
was found.'
Maybe 'No default template key was found'? FWIW, I've had very good luck with
users understanding descriptive error messages and either filing good bug
reports or being able to address the issue themselves.

[junjianx, 2013-09-25 01:02:51]

On 2013/09/24 12:29:20, sullivan wrote:
> lgtm
> 
>
https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
> File tools/deep_memory_profiler/visualizer/app.py (right):
> 
>
https://codereview.chromium.org/23781012/diff/40001/tools/deep_memory_profile...
> tools/deep_memory_profiler/visualizer/app.py:59: 'upload_msg': 'No default key
> was found.'
> Maybe 'No default template key was found'? FWIW, I've had very good luck with
> users understanding descriptive error messages and either filing good bug
> reports or being able to address the issue themselves.

Thanks for your review :)

[Dai Mikurube (NOT FULLTIME), 2013-09-25 02:48:45]

lgtm

[junjianx, 2013-09-25 04:40:08]

On 2013/09/25 02:48:45, Dai Mikurube wrote:
> lgtm

Thanks for your review :)

[commit-bot: I haz the power, 2013-09-25 04:40:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junjianx@chromium.org/23781012/52001

[commit-bot: I haz the power, 2013-09-25 04:42:03]

Change committed as 225141