[webkit-reviews] review denied: [Bug 76935] [Windows] NRWT doesn't save crash logs on Apple's Windows port : [Attachment 203370] 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 76935: [Windows] NRWT doesn't save crash logs on Apple's Windows port
https://bugs.webkit.org/show_bug.cgi?id=76935

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

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


Looks okay but needs to use more webkitpy abstractions to be consistent.

> Tools/Scripts/webkitpy/port/win.py:141
> +	   possible_paths = [self._filesystem.join(os.environ['PROGRAMFILES'],
"Windows Kits", "8.0", "Debuggers", "x64", "ntsd.exe"),
> +			     self._filesystem.join(os.environ['PROGRAMFILES'],
"Windows Kits", "8.0", "Debuggers", "x86", "ntsd.exe"),
> +			     self._filesystem.join(os.environ['PROGRAMFILES'],
"Debugging Tools for Windows (x86)", "ntsd.exe"),
> +			     self._filesystem.join(os.environ['ProgramW6432'],
"Debugging Tools for Windows (x64)", "ntsd.exe"),
> +			     self._filesystem.join(os.environ['SYSTEMROOT'],
"system32", "ntsd.exe")]

Nit: wrong indentation. self. should appear exactly 4 spaces to the right of
possible.

> Tools/Scripts/webkitpy/port/win.py:157
> +	   with open(command_file, "w") as text_file:
> +	       text_file.write(".logopen /t \"%s\\CrashLog%s.txt\"\n" %
(cygpath(self.results_directory()), self.CRASH_LOG_PREFIX))
> +	       text_file.write(".srcpath \"%s\"\n" %
cygpath(os.environ['WEBKIT_SOURCE']))
> +	       text_file.write("!analyze -vv\n")
> +	       text_file.write("~*kpn\n")
> +	       text_file.write("q\n")

Please use self._filesystem.write_text_file.
cygpath(os.environ['WEBKIT_SOURCE']) should be read off of
WebKitFinder.webkit_base.

> Tools/Scripts/webkitpy/port/win.py:162
> +	   value = subprocess.Popen(["regtool", "--wow32", "get",
registry_key], bufsize=1024, stdout=subprocess.PIPE).communicate()[0]

Use webkitpy.common.system.executive.

> Tools/Scripts/webkitpy/port/win.py:174
> +	   rc = subprocess.call(["regtool", "--wow32", "set", "-s",
str(registry_key), str(value)])
> +	   if rc == 2:
> +	       rc = subprocess.call(["regtool", "--wow32", "add", "-s",
str(registry_key)])
> +	       if rc == 0:
> +		   rc = subprocess.call(["regtool", "--wow32", "set", "-s",
str(registry_key), str(value)])
> +	   if rc:
> +	       _log.warn("Error setting key: %s to value %s.  Error=%ld." %
(key, value, rc))
> +	       return False

Ditto.

> Tools/Scripts/webkitpy/port/win.py:190
> +	   ntsd_path = self._ntsd_location()
> +	   if None == ntsd_path:

Do: not ntsd_path.

> Tools/Scripts/webkitpy/port/win.py:199
> +	   command_file = self.create_debugger_command_file()
> +	   if None == command_file:

Ditto.

> Tools/Scripts/webkitpy/port/win.py:201
> +	   debugger_options = "\"" + cygpath(ntsd_path) + "\" -p %ld -e %ld -g
-lines -cf \"" + cygpath(command_file) + "\""

Why not '"%s" -p %ld -e %ld -g -lines -cf "%s"' % (cygpath(ntsd_path),
cygpath(command_file))?