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

[bugzilla-daemon at webkit.org]

Alejandro G. Castro <alex at igalia.com> 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 218681: Patch
https://bugs.webkit.org/attachment.cgi?id=218681&action=review

------- Additional Comments from Alejandro G. Castro <alex at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218681&action=review


Patch is looking good, but I would like a pythong reviewer to check it. I like
the result it is very useful. I'm also uploading a blocking patch to this one
because I found some issue in the gtk.py leak detector. Also I would like to
remove the two parameters --leaks and --wrapper=valgrind, at least for the
default situation.

> Tools/ChangeLog:7
> +
> +	   Need a short description (OOPS!).
> +	   Need the bug URL (OOPS!).

You just can remove this.

> Tools/Scripts/webkitpy/port/gtk.py:-163
> -    # FIXME: We should find a way to share this implmentation with Gtk,
> -    # or teach run-launcher how to call run-safari and move this down to
Port.

Why are you removing this comment from the show_results_html_file function?

> Tools/Scripts/webkitpy/port/leakdetector_valgrind_unittest.py:716
> +	  
files['/Users/mock/Library/Logs/layout-test-results/drt-28529-db92e4843be411e3b
ae1d43d7e01ba08-leaks.xml'] = mock_valgrind_output1
> +	  
files['/Users/mock/Library/Logs/layout-test-results/drt-28530-dd7213423be411e3a
a7fd43d7e01ba08-leaks.xml'] = mock_valgrind_output2
> +	  
#files['/Users/mock/Library/Logs/layout-test-results/drt-28531-e8c7d7b83be411e3
90c9d43d7e01ba08-leaks.xml'] = mock_incomplete_valgrind_output
> +	  
#files['/Users/mock/Library/Logs/layout-test-results/drt-28532-ebc9a6c63be411e3
99d4d43d7e01ba08-leaks.xml'] = None
> +	  
#files['/Users/mock/Library/Logs/layout-test-results/drt-28532-fa6d0cd63be411e3
9c72d43d7e01ba08-leaks.xml'] = misformatted_mock_valgrind_output
> +	   filesystem = MockFileSystem(files)

I guess this commented lines mean this is still unfinished? I would add at
least a some empty output, broken output and proper output in different  unit
tests.