Fill json data in index.html template for dmprof visualizer

Fill json data in index.html template for dmprof visualizer.

Fill json data in index.html template by python script and
try to open page in chrome using xdg-open command. If page
open failed, it will output the path of generated html file.

index.html containing the content of index.js would be
generated by python script, so index.html and index.js are
deleted.

BUG=259206
NOTRY=True

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

[junjianx, 2013-09-04 03:22:31]

I wrote python script for template generating. Please review my code, thanks.

[Dai Mikurube (NOT FULLTIME), 2013-09-04 04:06:40]

Thanks for working on it. Comments:

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/template.py (right):

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:11: 
nit: better to have two empty lines between top-level items in Python. Same for
others.

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:51: data =
json.dumps(json.load(file(argv[1], 'r')))
Do we need to json.load() and then json.dumps()? Just read() doesn't work?

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:52: html = open('index.html',
'w')
1) Let's fix the directory to put the temporary html file. A user may run this
script from another directory. You can use:
* __file__
* os.path.abspath()
* os.path.dirname()
* os.path.join()
in Python. tools/deep_memory_profiler/lib/sorter.py may help your understanding.

In my thought, tools/deep_memory_profiler/graphs/ may be a reasonable directory
for this purpose. But, the name is up to you.

2) Let's use a temporary file name in the fixed directory. A user may not want
to overwrite an old file. A 'tempfile' package and tempfile.mkstemp() in Python
may help you. See tools/deep_memory_profiler/subcommands/upload.py and
http://docs.python.org/2/library/tempfile.html

3) Finally, let's create '.gitignore' in tools/deep_memory_profiler and ignore
the directory created in (1).

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:56: if
commands.getoutput('type google-chrome') != 'google-chrome not found':
'type' works only for bash. Also, the user may not have google-chrome in $PATH.

Consider using 'xdg-open' here with checking that the environment is Linux.
'xdg-open' exists almost all the Linux distributions. If it's non-Linux or
xdg-open doesn't exist, you can finish the script just with printing the file
path.

[junjianx, 2013-09-04 06:55:36]

I fixed the review problems, please review my code again, thanks

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/template.py (right):

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:11: 
On 2013/09/04 04:06:40, Dai Mikurube wrote:
> nit: better to have two empty lines between top-level items in Python. Same
for
> others.

Done.

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:51: data =
json.dumps(json.load(file(argv[1], 'r')))
On 2013/09/04 04:06:40, Dai Mikurube wrote:
> Do we need to json.load() and then json.dumps()? Just read() doesn't work?

Done.

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:52: html = open('index.html',
'w')
On 2013/09/04 04:06:40, Dai Mikurube wrote:
> 1) Let's fix the directory to put the temporary html file. A user may run this
> script from another directory. You can use:
> * __file__
> * os.path.abspath()
> * os.path.dirname()
> * os.path.join()
> in Python. tools/deep_memory_profiler/lib/sorter.py may help your
understanding.
> 
> In my thought, tools/deep_memory_profiler/graphs/ may be a reasonable
directory
> for this purpose. But, the name is up to you.
> 
> 2) Let's use a temporary file name in the fixed directory. A user may not want
> to overwrite an old file. A 'tempfile' package and tempfile.mkstemp() in
Python
> may help you. See tools/deep_memory_profiler/subcommands/upload.py and
> http://docs.python.org/2/library/tempfile.html
> 
> 3) Finally, let's create '.gitignore' in tools/deep_memory_profiler and ignore
> the directory created in (1).

Done.

https://codereview.chromium.org/23933002/diff/3001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:56: if
commands.getoutput('type google-chrome') != 'google-chrome not found':
On 2013/09/04 04:06:40, Dai Mikurube wrote:
> 'type' works only for bash. Also, the user may not have google-chrome in
$PATH.
> 
> Consider using 'xdg-open' here with checking that the environment is Linux.
> 'xdg-open' exists almost all the Linux distributions. If it's non-Linux or
> xdg-open doesn't exist, you can finish the script just with printing the file
> path.

Done.

[Dai Mikurube (NOT FULLTIME), 2013-09-04 07:39:47]

The first line of the issue description should be requisite minimum. Recommended
to be less than 80 characters.

It's good to describe more in next lines with an empty line after the first
line.


And, some more comments.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/template.py (right):

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:2: #
We don't need an empty line here.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:8: import sys
import subprocess should be before import sys in alphabetical order.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:10: import tempfile
nit: an empty line after 'import tempfile'

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:14: _TEMPLATE = """
The generated html starts with an empty line while <!DOCTYPE html> is better at
the first line.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:50: 
nit: two empty lines here. (a top-level item means a no-indent block in Python.)

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:58: dmp_path =
os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
nit: I recommend s/dmp/dmprof/. "dmp" is ambiguous.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:63: html_handle, html_path =
tempfile.mkstemp('.html', 'graphs', html_dir)
A generated html has a single graph. Just 'graph' looks better for a html file
instead of 'graphs'.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:69: if os.name == 'posix':
To check the platform, 'sys.platform' is better.
http://docs.python.org/2/library/sys.html#sys.platform

For example, FreeBSD goes to os.name == 'posix', but it may not have xdg-open.
(I'm not confident for FreeBSD, but a similar thing can easily happen for other
POSIX ones.)

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:73: print 'xdg-open failed:',
exception
This 'print' should go to stderr since it's clearly an error message. (The next
print may not be an error message.)

print >> sys.stderr, 'xdg-...'

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:77: 
nit: two empty lines here.

[junjianx, 2013-09-04 07:55:36]

I fixed review problems, please review my code again, thanks.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
File tools/deep_memory_profiler/visualizer/template.py (right):

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:2: #
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> We don't need an empty line here.

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:8: import sys
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> import subprocess should be before import sys in alphabetical order.

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:10: import tempfile
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> nit: an empty line after 'import tempfile'

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:14: _TEMPLATE = """
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> The generated html starts with an empty line while <!DOCTYPE html> is better
at
> the first line.

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:50: 
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> nit: two empty lines here. (a top-level item means a no-indent block in
Python.)

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:58: dmp_path =
os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> nit: I recommend s/dmp/dmprof/. "dmp" is ambiguous.

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:63: html_handle, html_path =
tempfile.mkstemp('.html', 'graphs', html_dir)
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> A generated html has a single graph. Just 'graph' looks better for a html file
> instead of 'graphs'.

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:69: if os.name == 'posix':
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> To check the platform, 'sys.platform' is better.
> http://docs.python.org/2/library/sys.html#sys.platform
> 
> For example, FreeBSD goes to os.name == 'posix', but it may not have xdg-open.
> (I'm not confident for FreeBSD, but a similar thing can easily happen for
other
> POSIX ones.)

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:73: print 'xdg-open failed:',
exception
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> This 'print' should go to stderr since it's clearly an error message. (The
next
> print may not be an error message.)
> 
> print >> sys.stderr, 'xdg-...'

Done.

https://codereview.chromium.org/23933002/diff/8001/tools/deep_memory_profiler...
tools/deep_memory_profiler/visualizer/template.py:77: 
On 2013/09/04 07:39:48, Dai Mikurube wrote:
> nit: two empty lines here.

Done.

[Dai Mikurube (NOT FULLTIME), 2013-09-04 07:58:51]

The code looks good, but could you fix the issue description as I said in the
comment #4 of mine?

[junjianx.google, 2013-09-04 08:03:20]

oh, sorry, I neglected that comment.
Does the issue description look good now?


On Wed, Sep 4, 2013 at 4:58 PM, <dmikurube@chromium.org> wrote:

> The code looks good, but could you fix the issue description as I said in
> the
> comment #4 of mine?
>
>
https://codereview.chromium.**org/23933002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Dai Mikurube (NOT FULLTIME), 2013-09-04 08:07:27]

Needs some more fix.

1) The first line still should have "for dmprof visualizer".

2) The other lines are also recommended to be shorter than 80 characters. Break
the lines.

3) Optional: better to have the same text in the issue title and the first line.

[Dai Mikurube (NOT FULLTIME), 2013-09-04 08:20:51]

lgtm

[sullivan, 2013-09-04 14:15:22]

lgtm

https://codereview.chromium.org/23933002/diff/13001/tools/deep_memory_profile...
File tools/deep_memory_profiler/visualizer/template.py (right):

https://codereview.chromium.org/23933002/diff/13001/tools/deep_memory_profile...
tools/deep_memory_profiler/visualizer/template.py:65:
html_file.write(Template(_TEMPLATE).safe_substitute({ 'DATA': data }))
Are there any concerns about XSS here? For example, the file could contain
arbitrary JavaScript which runs on the user's machine. Should it be verified to
be valid JSON first?

[commit-bot: I haz the power, 2013-09-04 16:39:13]

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

[commit-bot: I haz the power, 2013-09-04 17:28:51]

Change committed as 221222

[junjianx.google, 2013-09-05 05:45:41]

hello, @sullivan

Thank you for the review.
This python script is only for local usage of dmprof, so we think that
there is no need to think about security problem.

xu


On Wed, Sep 4, 2013 at 11:15 PM, <sullivan@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.**org/23933002/diff/13001/tools/**
>
deep_memory_profiler/**visualizer/template.py<https://codereview.chromium.org/23933002/diff/13001/tools/deep_memory_profiler/visualizer/template.py>
> File tools/deep_memory_profiler/**visualizer/template.py (right):
>
> https://codereview.chromium.**org/23933002/diff/13001/tools/**
>
deep_memory_profiler/**visualizer/template.py#**newcode65<https://codereview.chromium.org/23933002/diff/13001/tools/deep_memory_profiler/visualizer/template.py#newcode65>
> tools/deep_memory_profiler/**visualizer/template.py:65:
> html_file.write(Template(_**TEMPLATE).safe_substitute({ 'DATA': data }))
> Are there any concerns about XSS here? For example, the file could
> contain arbitrary JavaScript which runs on the user's machine. Should it
> be verified to be valid JSON first?
>
>
https://codereview.chromium.**org/23933002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.