[webkit-reviews] review denied: [Bug 171976] webkitpy: Process crash-logs for iOS devices : [Attachment 311771] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 171976: webkitpy: Process crash-logs for iOS devices
https://bugs.webkit.org/show_bug.cgi?id=171976

Attachment 311771: Patch

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




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

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

> Tools/ChangeLog:12
> +	   * Scripts/webkitpy/common/system/crashlogs.py:

This ChangeLog entry is out-of-date or there is a bug in prepare-ChangeLog. In
particular, this ChangeLog is missing an entry for the modification of function
CrashLogs.is_crash_log(). Please regenerate this ChangeLog file or file a bug
about prepare-ChangeLog.

> Tools/Scripts/webkitpy/common/system/crashlogs.py:58
> +		   return False

Is this correct?

> Tools/Scripts/webkitpy/common/system/crashlogs.py:88
> +		   return False

Is this correct?

> Tools/Scripts/webkitpy/common/system/crashlogs.py:151
> +		       for line in log_contents.splitlines():
> +			   match = process_regex.match(line)
> +			   if match:
> +			       break
> +

Ewww. We should not duplicate code. I suggest that we extract out the
process_regex and this parsing logic into a common helper method, say
parse_darwin_crash_log(), that takes a path and symbolicates the crash log,
parses it, and returns a (process name, pid) tuple on success and None if
parsing fails.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:203
> +	   # Cache crash logs already on host.
> +	   try:
> +	       crash_log_directory = self._port.path_to_crash_logs()
> +	   except NotImplementedError:
> +	       return True
> +	   for i in xrange(self._options.child_processes):
> +	       target_host = self._port.target_host(i)
> +	       if not target_host.cached_crash_logs:
> +		   target_host.cached_crash_logs =
target_host.filesystem.files_under(crash_log_directory)
> +

This is not the appropriate place to do port-specific logic. Port specific
logic should live in the Port class. Can we move this logic into
Port.setup_test_run()?

> Tools/Scripts/webkitpy/port/device.py:70
> +	   return path

This is not correct. The code in CrashLogs.py assumes
symbolicate_crash_log_if_needed() returns the contents of the crash log. Either
this code needs to change or the code in CrashLogs.py needs to change.

> Tools/Scripts/webkitpy/port/ios_device.py:73
> +	       raise NotImplementedError

Raising a NotImplementedError exception is inconsistent with the behavior
described in other functions when the Apple Additions module is unavailable.
For example, in _device_for_worker_number_map() we raise
RuntimeError(self.NO_ON_DEVICE_TESTING) when the Apple Additions module is
unavailable.