[webkit-reviews] review denied: [Bug 81659] [GTK] crash log reports support : [Attachment 132836] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 81659: [GTK] crash log reports support
https://bugs.webkit.org/show_bug.cgi?id=81659

Attachment 132836: Patch
https://bugs.webkit.org/attachment.cgi?id=132836&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
I am a bit of two minds as to whether these routines belong on the Port object
or under common/system somewhere. I think I agree that figuring out what the
right crash log is is a property of the Port and not the operating system,
since, for example, chromium uses different mechanisms than apple on the mac or
Gtk on Linux.

Adam, Eric, any thoughts?

Apart from that, this approach seems reasonable but we need to merge it with
the clean up I'm doing in bugs 81600 and 81603. Also, you might care about the
change in bug 81575. 


View in context: https://bugs.webkit.org/attachment.cgi?id=132836&action=review


> Tools/Scripts/webkitpy/common/system/executive.py:264
> +		   return e.errno == errno.EPERM

Why is this change needed? I don't understand what was failing before or is
fixed now ...

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:65
> +	      
writer.write_crash_report(crashed_driver_output.crashed_process_name,
crashed_driver_output.error, driver_output.pid)

We should coordinate this change with my patches in bug 81603 and 81600 - the
driver should return the crash log and the test result writer should just use
that.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:166
> +	   fs.write_text_file(filename, log if log else error.decode("utf-8"))

see above.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:67
> +	       test_time=0, timeout=False, error='', crashed_process_name=None,
pid=None):

see above; I think this should be crashed_pid, not pid.

> Tools/Scripts/webkitpy/layout_tests/port/gtk.py:146
> +	   return process_name == 'gdb-dump'

This function goes away w/ the patch in bug 81600 ...

> Tools/Scripts/webkitpy/tool/commands/queries.py:375
> +	   for port_name in tool.port_factory.all_port_names():

You probably don't want to iterate across all_port_names(), since that could
presumably get you the same crash log multiple times.