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

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted 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 312079: Patch

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




--- Comment #27 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 312079
  --> https://bugs.webkit.org/attachment.cgi?id=312079
Patch

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

r=me, but please consider the feedback noted.

> Tools/Scripts/webkitpy/common/system/crashlogs.py:74
> +	       if fs.join(dirpath, basename) in self._crash_logs_to_skip:
> +		   return False

Since iOS is the only port that uses skipped crash logs, you could add a check
to see if self._crash_logs_to_skip is not empty before calling fs.join() (which
creates a string just to see if it's in the list) [UNTESTED]:

    if self._crash_logs_to_skip and fs.join(dirpath, basename) in
self._crash_logs_to_skip:

If you do this, please add this code everywhere this check is added.

> Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:37
> -    return """Process:	  {process_name} [{pid}]
> +    return """May have garbage at the beginning

I'd suggest something other than "garbage" here like:

    return """Crash log may not start with Process line