[Webkit-unassigned] [Bug 160198] Crash Log Clobbering on Unbound Crashes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=160198

Aakash Jain <aakash_jain at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #284599|review?                     |review-
              Flags|                            |

--- Comment #3 from Aakash Jain <aakash_jain at apple.com> ---
Comment on attachment 284599
  --> https://bugs.webkit.org/attachment.cgi?id=284599
Patch

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

mostly looks good to me.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=160198

You might consider including the radar number here. e.g.: <rdar://problem/27472618>

> Tools/ChangeLog:11
> +        (TestResultWriter.output_filename): Only remove file extension if it is clearly a file extension

Nit: end the line with a '.'

> Tools/Scripts/webkitpy/common/system/crashlogs.py:34
>  class CrashLogs(object):

should have two blank lines before class. I wonder why style check did not catch this.

> LayoutTests/ChangeLog:5
> +

Ditto.

> LayoutTests/ChangeLog:8
> +        * fast/harness/results.html: Only remove file extension if it is clearly a file extension

Ditto.

> LayoutTests/fast/harness/results.html:275
> +    if (splitExtension(test)[1].length > 5)

This logic doesn't match with corresponding python code. splitExtension() returns the exact number of characters in extension, while fs.splitext() in python returns one extra character (counting '.').  For e.g. for "test.html" splitExtension(test)[1].length returns 4, and len(fs.splitext(output_filename)[1]) returns 5.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160726/133c2a37/attachment-0001.html>