Bug 179514 – webkitpy: Remove find_system_pid from port classes

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Even on Windows, find_system_pid is unused.  Remove dead code.

[Radar WebKit Bug Importer]

<rdar://problem/35456900>

[Jonathan Bedard]

Created attachment 326513 [details]
Patch

[Jonathan Bedard]

Created attachment 326514 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 326514 [details]
Patch

r=me

[Jonathan Bedard]

Created attachment 326520 [details]
Patch

[Jonathan Bedard]

(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>.

[WebKit Commit Bot]

Comment on attachment 326520 [details]
Patch

Clearing flags on attachment: 326520

Committed r224695: <https://trac.webkit.org/changeset/224695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

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.

[Jonathan Bedard]

(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.

[Jonathan Bedard]

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>.