[webkit-reviews] review denied: [Bug 87714] Mac crash logs can take a really long time to be written out. : [Attachment 144489] first patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied  review:
Bug 87714: Mac crash logs can take a really long time to be written out.
https://bugs.webkit.org/show_bug.cgi?id=87714

Attachment 144489: first patch
https://bugs.webkit.org/attachment.cgi?id=144489&action=review

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


Thanks for working on this! The patch looks generally sound but I think some of
the logic needs to be in different places to avoid adding layering dependencies
that shouldn't be there. Let me know if my comments below don't make sense.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:41
> +from webkitpy.layout_tests.models.result_summary import ResultSummary

The Port classes shouldn't have all of these dependencies on test_expectations,
TestResultWriter, ResultSummary, etc.

I think it might make more sense to move the logic in look_for_new_crash_logs
that actually gets the list of <process_name, pid> pairs to look for into
manager.py, and have the Port routine just take a list of tuples to check for.
Actually building the list is generic and should be fast, so there's little
harm in running it all the time for every port.

Then the look_for_new_crash_logs() call can return a list or a dict of found
crash logs, and let the manager instantiate the TestResultWriter to actually
write the files (since, again, this should be generic logic).

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:224
> +		   crash_log = self._get_crash_log(failure.process_name,
failure.pid, None, None, start_time)

I think it might make sense to modify _get_crash_log() so that it doesn't do
the slow-spin on the second pass (since there's probably not much point in
waiting any longer)?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:227
> +		   if not crash_log.startswith("no"):

It might also be better to modify _get_crash_log() to return None and let the
caller come up with a default (here, you don't do anything, in
WebKitDriver.run_test() return the "no crash log found" string ...