[webkit-reviews] review denied: [Bug 117098] [Windows] NRWT is not putting crash logs in proper place : [Attachment 203616] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 117098: [Windows] NRWT is not putting crash logs in proper place
https://bugs.webkit.org/show_bug.cgi?id=117098

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

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


>> Tools/Scripts/webkitpy/common/system/crashlogs.py:83
>> +	    log_directory = port.results_directory()
> 
> I only needed to pass in 'port' to get access to this directory.  Is there
some other way for the CrashLog object to get to the port without passing as
argument?

Since log_directory is only used in the call to files_under, why don't we just
call it there?

> Tools/Scripts/webkitpy/common/system/crashlogs.py:85
> +	   pid_line_regex = re.compile('\s+Global\s+PID:\s+\[(?P<pid>\d+)\]')

Since this doesn't change, why don't we put it as a static value?

> Tools/Scripts/webkitpy/common/system/crashlogs.py:86
> +	   errors = ''

Ser errors to None.

> Tools/Scripts/webkitpy/common/system/crashlogs.py:98
> +		       if (int(match.group('pid')) == pid):

The outer parenthesis is not needed.

> Tools/Scripts/webkitpy/port/win.py:239
> +	       winpid = self._executive.pid_to_sys_pid.get(pid)

Nit: we should call this system_id to be consistent.

> Tools/Scripts/webkitpy/port/win.py:268
> +	       if not crash_log:
> +		   continue
> +	       crash_logs[test_name] = crash_log

It's probably cleaner to write it as:
if crash_log:
    crash_logs[test_name] = crash_log

> Tools/Scripts/webkitpy/port/win.py:272
> +	   self.win_pid = int(pid)

Why are we setting this as an instance variable?  It's not used anywhere else
so I don't think this is intended.
r- because of this.

Also, we should call this system_pid to be consistent.