[webkit-reviews] review denied: [Bug 184039] webkitpy: Implement coredumpctl support on linux : [Attachment 336581] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 184039: webkitpy: Implement coredumpctl support on linux
https://bugs.webkit.org/show_bug.cgi?id=184039

Attachment 336581: Patch

https://bugs.webkit.org/attachment.cgi?id=336581&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 336581
  --> https://bugs.webkit.org/attachment.cgi?id=336581
Patch

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

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:32
> +import datetime

Not in sorted order. Please sort all the  import lines suchthat they are in
sorted order according to the UNIX sort tool.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:60
> +	   for ntry in range(5):

This is a suboptimal way to loop as we malloc a list. What does ntry stand for?
Did you mean to write retries?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:62
> +		   # Loopping, it means we conceder the logs might not be ready

Please remove this comment as it is redundant given the comment on line 59.
Also, I am surprised we don’t have a convenience function in webktpy to do
waiting for us.

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:64
> +		   time.sleep(1)

Is there not a more direct way to know if systemd generating the backtrack so
we can avoid busy waiting?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:67
> +		   info = subprocess.check_output(['coredumpctl', 'info',

Can we write this logic using the Executive object? Can this class take an
Executive object in its constructor?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:71
> +		   # The trace might not be ready yet

:( Can we find a more deterministic and direct way to test this. Writing code
that sleeps and hopes the data becomes available when we wake artificially
slows down testing.