Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
194573005:
Web page front-end for grokdump. (Closed)
|
Created:
6 years, 9 months ago by Jarin Modified:
6 years, 9 months ago Reviewers:
Jakob Kummerow CC:
v8-dev Visibility:
Public. |
DescriptionWeb page front-end for grokdump.
This is a prototype to start discussion about useful features for
a better/friendlier minidump analysis tool.
The change adds an -w option to grokdump.py. With the option on,
grokdump will launch a web server and web browser for browsing the
minidump. It also supports adding persistent comments and listing +
browsing other minidumps (in the same directory) without the need to
restart the web server.
R=jkummerow@chromium.org
BUG=
Committed: https://code.google.com/p/v8/source/detail?r=20281
Patch Set 1 #
Total comments: 61
Patch Set 2 : Addressing reviewer comments #Patch Set 3 : Addressing pylint suggestions #Messages
Total messages: 4 (0 generated)
LGTM, just nits. Great stuff! I haven't spent much time verifying correctness. We'll notice in "production" when something doesn't work. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py File tools/grokdump.py (right): https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode714 tools/grokdump.py:714: aligned_res = [ ] nit: the style guide doesn't like spaces inside empty pairs of (), [], {}. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1592 tools/grokdump.py:1592: COMMENT_RE = re.compile(r"^C (0x[0-9a-fA-F]+) (.*)$") style nit: two empty lines between top-level things (no empty line between constants is fine) https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1603 tools/grokdump.py:1603: f = open(self.cmt_file, "r") The pythonic way to express open/close sequences is: with open(filename, "r") as f: lines = f.readlines() The beauty being that f is automatically closed at the end of the with-block regardless of exceptions being thrown or whatever else happening. Consider replacing the try..except construct with "if os.path.exists(self.cmt_file):". Arguably it is good style to avoid exceptions for commonplace control flow. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1624 tools/grokdump.py:1624: f = open(self.cmt_file, "a") again, prefer "with" https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1629 tools/grokdump.py:1629: self.styles = { } nit: I have a weak preference to initialize all member variables in the constructor (i.e. __init__). https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1631 tools/grokdump.py:1631: # Color all stack addresses nit: trailing full stop please https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1660 tools/grokdump.py:1660: f = open(self.cmt_file, "a") with https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1767 tools/grokdump.py:1767: .code {{ If you used %s-placeholders ("foo %s baz" % bar), you wouldn't need to duplicate all those curly braces... up to you. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1768 tools/grokdump.py:1768: font-family: monospace; nit: funky indentation https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1832 tools/grokdump.py:1832: }} Is this intentionally empty? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1891 tools/grokdump.py:1891: xmlhttp.open("GET", I think using GET to perform actions is considered bad style... then again I guess in this case we don't care. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1924 tools/grokdump.py:1924: location.reload(true) nit: {} around the block please https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1932 tools/grokdump.py:1932: function main() {{ looks like we don't need this urgently. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1962 tools/grokdump.py:1962: <!-- nit: remove this if it's not needed https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1980 tools/grokdump.py:1980: class WebParameterError(Exception): again, two empty lines before and after top-level classes please https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1985 tools/grokdump.py:1985: def fmt(self, qc): naming nit: abbreviations are generally frowned upon. cmt, fmt, qc, wtf? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2000 tools/grokdump.py:2000: print "GET!" Looks like a debugging aid. Is this still needed? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2007 tools/grokdump.py:2007: if parsedurl.path == "/summary.html": s/if/elif/ like below? In turn you could omit all the "return" statements. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2057 tools/grokdump.py:2057: self.send_error(404,'Invalid params') again, I'd prefer if+else over if+return https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2112 tools/grokdump.py:2112: stack_bottom = exception_thread.stack.start + \ nit: the style guide prefers () over trailing \, i.e.: stack_bottom = (exception_thread.stack.start + exception_thread.stack.memory.data_size) Again below. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2132 tools/grokdump.py:2132: print "Adding comment to dump '%s': %s -> '%s'" % ( Still needed? If you don't want to delete it, consider putting it behind the existing "if DEBUG:" global flag. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2141 tools/grokdump.py:2141: print "Adding page annotation to dump '%s': %s -> %s" % ( still needed? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2158 tools/grokdump.py:2158: if maybeaddress == None: nit: the style guide wants "is None" instead of "== None". Also, "maybe_address" would be a tad more readable. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2268 tools/grokdump.py:2268: f.write(" Exception code: %08X\n" % ( nit: I'd break as: f.write(" Exception code: %08X\n" % self.reader.exception.exception.code) (Again below.) https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2295 tools/grokdump.py:2295: (a + size - 1) % size) nit: fits on previous line? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2299 tools/grokdump.py:2299: def format_object(self, address): nit: one empty line is enough between non-toplevel definitions https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2323 tools/grokdump.py:2323: def output_words(self, f, start_address, end_address, \ nit: trailing \ is not necessary https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2353 tools/grokdump.py:2353: end_address, nit: fits on one line? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2442 tools/grokdump.py:2442: f.write("·") Doesn't this need s/·/·/ ? https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2592 tools/grokdump.py:2592: nit: just one empty line https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2746 tools/grokdump.py:2746: f.write(("<input type=\"text\" class=\"dumpcomments\" " nit: the indentation here is particularly inconsistent. Suggestion: f.write("<input type=\"text\" class=\"dumpcomments\" " "id=\"dump-%s\" onchange=\"dump_cmt()\" value=\"%s\">\n" % (cgi.escape(name), desc)) https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode3157 tools/grokdump.py:3157: help="start a web page") nit: s/page/server on localhost:8081/ (is it worth the effort to make the port configurable?)
- Fixed the formatting. - Ran the pages through an HTML validator and fixed the errors. - There is still some bug in disasm view, will fix in the next patchset (column splitting/objdump parsing, address annotations). https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py File tools/grokdump.py (right): https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode714 tools/grokdump.py:714: aligned_res = [ ] On 2014/03/24 15:21:32, Jakob wrote: > nit: the style guide doesn't like spaces inside empty pairs of (), [], {}. Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1592 tools/grokdump.py:1592: COMMENT_RE = re.compile(r"^C (0x[0-9a-fA-F]+) (.*)$") On 2014/03/24 15:21:32, Jakob wrote: > style nit: two empty lines between top-level things (no empty line between > constants is fine) Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1603 tools/grokdump.py:1603: f = open(self.cmt_file, "r") On 2014/03/24 15:21:32, Jakob wrote: > The pythonic way to express open/close sequences is: > > with open(filename, "r") as f: > lines = f.readlines() > > The beauty being that f is automatically closed at the end of the with-block > regardless of exceptions being thrown or whatever else happening. > > Consider replacing the try..except construct with "if > os.path.exists(self.cmt_file):". Arguably it is good style to avoid exceptions > for commonplace control flow. Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1624 tools/grokdump.py:1624: f = open(self.cmt_file, "a") On 2014/03/24 15:21:32, Jakob wrote: > again, prefer "with" Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1629 tools/grokdump.py:1629: self.styles = { } On 2014/03/24 15:21:32, Jakob wrote: > nit: I have a weak preference to initialize all member variables in the > constructor (i.e. __init__). Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1631 tools/grokdump.py:1631: # Color all stack addresses On 2014/03/24 15:21:32, Jakob wrote: > nit: trailing full stop please Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1660 tools/grokdump.py:1660: f = open(self.cmt_file, "a") On 2014/03/24 15:21:32, Jakob wrote: > with Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1767 tools/grokdump.py:1767: .code {{ On 2014/03/24 15:21:32, Jakob wrote: > If you used %s-placeholders ("foo %s baz" % bar), you wouldn't need to duplicate > all those curly braces... up to you. However, with %s you cannot reuse the same argument for several replacements. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1768 tools/grokdump.py:1768: font-family: monospace; On 2014/03/24 15:21:32, Jakob wrote: > nit: funky indentation Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1832 tools/grokdump.py:1832: }} On 2014/03/24 15:21:32, Jakob wrote: > Is this intentionally empty? Removed. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1891 tools/grokdump.py:1891: xmlhttp.open("GET", On 2014/03/24 15:21:32, Jakob wrote: > I think using GET to perform actions is considered bad style... then again I > guess in this case we don't care. Yeah, I am leaving as is. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1924 tools/grokdump.py:1924: location.reload(true) On 2014/03/24 15:21:32, Jakob wrote: > nit: {} around the block please Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1932 tools/grokdump.py:1932: function main() {{ On 2014/03/24 15:21:32, Jakob wrote: > looks like we don't need this urgently. Removed. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1962 tools/grokdump.py:1962: <!-- On 2014/03/24 15:21:32, Jakob wrote: > nit: remove this if it's not needed Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1980 tools/grokdump.py:1980: class WebParameterError(Exception): On 2014/03/24 15:21:32, Jakob wrote: > again, two empty lines before and after top-level classes please Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode1985 tools/grokdump.py:1985: def fmt(self, qc): On 2014/03/24 15:21:32, Jakob wrote: > naming nit: abbreviations are generally frowned upon. cmt, fmt, qc, wtf? Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2000 tools/grokdump.py:2000: print "GET!" On 2014/03/24 15:21:32, Jakob wrote: > Looks like a debugging aid. Is this still needed? Removed. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2057 tools/grokdump.py:2057: self.send_error(404,'Invalid params') On 2014/03/24 15:21:32, Jakob wrote: > again, I'd prefer if+else over if+return Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2132 tools/grokdump.py:2132: print "Adding comment to dump '%s': %s -> '%s'" % ( On 2014/03/24 15:21:32, Jakob wrote: > Still needed? If you don't want to delete it, consider putting it behind the > existing "if DEBUG:" global flag. Removed. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2141 tools/grokdump.py:2141: print "Adding page annotation to dump '%s': %s -> %s" % ( On 2014/03/24 15:21:32, Jakob wrote: > still needed? Removed. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2268 tools/grokdump.py:2268: f.write(" Exception code: %08X\n" % ( On 2014/03/24 15:21:32, Jakob wrote: > nit: I'd break as: > f.write(" Exception code: %08X\n" % > self.reader.exception.exception.code) > (Again below.) Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2295 tools/grokdump.py:2295: (a + size - 1) % size) On 2014/03/24 15:21:32, Jakob wrote: > nit: fits on previous line? Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2299 tools/grokdump.py:2299: def format_object(self, address): On 2014/03/24 15:21:32, Jakob wrote: > nit: one empty line is enough between non-toplevel definitions Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2323 tools/grokdump.py:2323: def output_words(self, f, start_address, end_address, \ On 2014/03/24 15:21:32, Jakob wrote: > nit: trailing \ is not necessary Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2353 tools/grokdump.py:2353: end_address, On 2014/03/24 15:21:32, Jakob wrote: > nit: fits on one line? Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2442 tools/grokdump.py:2442: f.write("·") On 2014/03/24 15:21:32, Jakob wrote: > Doesn't this need s/·/·/ ? Done (the char code above needs ';', too) https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2592 tools/grokdump.py:2592: On 2014/03/24 15:21:32, Jakob wrote: > nit: just one empty line Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode2746 tools/grokdump.py:2746: f.write(("<input type=\"text\" class=\"dumpcomments\" " On 2014/03/24 15:21:32, Jakob wrote: > nit: the indentation here is particularly inconsistent. Suggestion: > f.write("<input type=\"text\" class=\"dumpcomments\" " > "id=\"dump-%s\" onchange=\"dump_cmt()\" value=\"%s\">\n" % > (cgi.escape(name), desc)) Done. https://codereview.chromium.org/194573005/diff/1/tools/grokdump.py#newcode3157 tools/grokdump.py:3157: help="start a web page") On 2014/03/24 15:21:32, Jakob wrote: > nit: s/page/server on localhost:8081/ (is it worth the effort to make the port > configurable?) Done (the port not configurable yet)
Message was sent while issue was closed.
Committed patchset #3 manually as r20281 (presubmit successful). |
