[Webkit-unassigned] [Bug 119446] [GTK] Parse Valgrind xml leak files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119446


Dirk Pranke <dpranke at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #208024|review?                     |review-
               Flag|                            |




--- Comment #6 from Dirk Pranke <dpranke at chromium.org>  2013-08-05 13:00:45 PST ---
(From update of attachment 208024)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.