[webkit-reviews] review granted: [Bug 84196] run-webkit-tests picked up an old crash log : [Attachment 137778] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 84196: run-webkit-tests picked up an old crash log
https://bugs.webkit.org/show_bug.cgi?id=84196

Attachment 137778: Patch
https://bugs.webkit.org/attachment.cgi?id=137778&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137778&action=review


>>> Tools/Scripts/webkitpy/common/system/crashlogs.py:38
>>> +	 def find_newest_log(self, process_name, pid=None,
include_errors=False, newer_than=None):
>> 
>> I'm not sure if newer_than makes sense as the variable name. I think
start_time would be semantically clearer.
> 
> Interesting, newer_than seems clear to me (show me crash logs newer than X),
but start_time not at all (logs don't have a start time) ... why do you think
the opposite? Are you thinking that you're looking for processes started after
X? The process doesn't have to have started after X, though, just crashed after
X.
> 
> Maybe "created_after" would be better, or "crashed_after", or just "after"?
> 
> Note that there is a minor correctness issue in that you actually want ctime
> X, not mtime > X, but there is no filesystem.ctime() routine. I could add
one, but it seemed unnecessary since ctime will presumably equal mtime nearly
all of the time ...

Ah, I see what you're saying. Okay.