[webkit-reviews] review denied: [Bug 119446] [GTK] Parse Valgrind xml leak files : [Attachment 208024] Parse Valgrind XML files

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 5 13:01:04 PDT 2013


Dirk Pranke <dpranke at chromium.org> has denied Brian Holt
<brian.holt at samsung.com>'s request for review:
Bug 119446: [GTK] Parse Valgrind xml leak files
https://bugs.webkit.org/show_bug.cgi?id=119446

Attachment 208024: Parse Valgrind XML files
https://bugs.webkit.org/attachment.cgi?id=208024&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=208024&action=review


I didn't review everything in this patch yet (in particular, I haven't looked
at memcheck_analyze.py much. However, this code is clearly not written to
generally conform to the webkitpy conventions and (IMO) should be cleaned up to
do so, first.

You also should write at least some unittests for this.

> Tools/Scripts/webkitpy/port/common.py:35
> +import subprocess

This code  duplicates a lot of the work in webkitpy.common.system and
webkitpy.port.server_process . Please use that code instead and/or get rid of
the duplicate logic here.

>> Tools/Scripts/webkitpy/port/common.py:43
>> +
> 
> Two empty lines here, while one would be enough between definitions. Please
fix all across the file (it happens all the time in this patch)

Actually PEP-8 requires two blanks between top-level definitions.

> Tools/Scripts/webkitpy/port/common.py:219
> +	    "_ZN4base8internal15RunnableAdapter*Run*"),

Most of this file is pretty generic, but this routine is clearly pretty
specific to one particular caller or purpose. You should rename this to
something more indicative. Also, somehow it seems unlikely that the base::
functions will be needed somewhere :).

> Tools/Scripts/webkitpy/port/gdb_helper.py:31
> +''' A bunch of helper functions for querying gdb.'''

Again, you should probably be using the routines in webkitpy.system instead.

> Tools/Scripts/webkitpy/port/gtk_leakdetector.py:27
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

I would probably just fold these classes into gtk.py rather than create a
separate file.

> Tools/Scripts/webkitpy/port/memcheck_analyze.py:666
> +def _main():

Normally people use main() for this rather than _main() . Also, in webkitpy
land, we wouldn't have this at all and would either split this into a separate
helper script in Tools/Scripts or a _unittest.py file.


More information about the webkit-reviews mailing list