Even on Windows, find_system_pid is unused. Remove dead code.
<rdar://problem/35456900>
Created attachment 326513 [details] Patch
Created attachment 326514 [details] Patch
Comment on attachment 326514 [details] Patch r=me
Created attachment 326520 [details] Patch
(In reply to Jonathan Bedard from comment #5) > Created attachment 326520 [details] > Patch Pushing back through EWS to test an unrelated fix <https://trac.webkit.org/changeset/224658>.
Comment on attachment 326520 [details] Patch Clearing flags on attachment: 326520 Committed r224695: <https://trac.webkit.org/changeset/224695>
All reviewed patches have been landed. Closing bug.
Comment on attachment 326520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326520&action=review > Tools/Scripts/webkitpy/port/server_process.py:-123 > - self._port.find_system_pid(self.process_name(), self._pid) I know that that return value of this function side is unused. Are we sure this is not simply a bug? I mean, this function was explicitly added in the patch for <https://bugs.webkit.org/show_bug.cgi?id=117098> and has a non-trivial implementation that makes use of platform knowledge. I have not read this patch in detail. It seems weird that this function would have been accidentally given that its implementation requires a non-trivial investment in time to code.
(In reply to Daniel Bates from comment #9) > Comment on attachment 326520 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326520&action=review > > > Tools/Scripts/webkitpy/port/server_process.py:-123 > > - self._port.find_system_pid(self.process_name(), self._pid) > > I know that that return value of this function side is unused. Are we sure > this is not simply a bug? I mean, this function was explicitly added in the > patch for <https://bugs.webkit.org/show_bug.cgi?id=117098> and has a > non-trivial implementation that makes use of platform knowledge. I have not > read this patch in detail. It seems weird that this function would have been > accidentally given that its implementation requires a non-trivial investment > in time to code. The specified reason was to find matching crash-logs. We clearly were not using this functionality, and did not notice that we were missing it for 4 years. Even if we did want this functionality, it should be owned by _get_crash_log(...), not ServerProcess, since the ServerProcess pid is also used to kill a process. (and in that case, we do want to be killing the cygwin process) Lastly, this functionality is a bit redundant (even on Mac) since we pick up crash logs that occur during our test run anyways. The only win this would give us is being able to match a crash log with a specific test automatically.
I looked closer at our Windows results today. There is definitely a bug here, but it seems that the bug is actually in Executive.pid_to_system_pid or WinPort._get_crash_log. I filed a new bug to track that fix, because it will be unique to the WinPort and will not involve the ServerProcess <https://bugs.webkit.org/show_bug.cgi?id=179828>.