Bug 63468

Created attachment 98748 [details] Patch

Comment on attachment 98748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98748&action=review > Tools/Scripts/webkitpy/common/system/crashlog.py:44 > + if sys.platform == "darwin": > + self._capture_log_darwin(process_name) Its funny how initing one of these does the search. I think I would have called this thing CrashLogs and had a most_recent_log(process_name=) function instead. > Tools/Scripts/webkitpy/common/system/crashlog.py:58 > + log_directory = self._filesystem.expanduser("~") > + log_directory = os.path.join(log_directory, "Library", "Logs") > + if self._filesystem.exists(os.path.join(log_directory, "DiagnosticReports")): > + log_directory = os.path.join(log_directory, "DiagnosticReports") > + else: > + log_directory = os.path.join(log_directory, "CrashReporter") I would have made this a separate function. > Tools/Scripts/webkitpy/common/system/executive.py:301 > + ps_process = self.popen(['ps', '-eo', 'pid,comm'], stdout=self.PIPE, stderr=self.PIPE) Really? Python doesn't offer us a better way than running ps? I guess not: http://ubuntuforums.org/showthread.php?t=463850 > Tools/Scripts/webkitpy/common/system/executive.py:311 > + return running_pids Do we need to sort these? Might add a comment which explains why we don't. Does this handle pid-wrapping? do we care? > Tools/Scripts/webkitpy/common/system/executive.py:323 > + while self.check_running_pid(pid): > + time.sleep(0.25) Can't we just waitpid? I guess on unix it has to be a child process? I wonder if the same restriction applies to windows?

(In reply to comment #2) > (From update of attachment 98748 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98748&action=review > > > Tools/Scripts/webkitpy/common/system/crashlog.py:44 > > + if sys.platform == "darwin": > > + self._capture_log_darwin(process_name) > > Its funny how initing one of these does the search. I think I would have called this thing CrashLogs and had a most_recent_log(process_name=) function instead. I'm not sure I fully understand. You're saying we should use a free function instead of a constructor? What would the CrashLog class have in it? > > Tools/Scripts/webkitpy/common/system/crashlog.py:58 > > + log_directory = self._filesystem.expanduser("~") > > + log_directory = os.path.join(log_directory, "Library", "Logs") > > + if self._filesystem.exists(os.path.join(log_directory, "DiagnosticReports")): > > + log_directory = os.path.join(log_directory, "DiagnosticReports") > > + else: > > + log_directory = os.path.join(log_directory, "CrashReporter") > > I would have made this a separate function. Done. > > Tools/Scripts/webkitpy/common/system/executive.py:301 > > + ps_process = self.popen(['ps', '-eo', 'pid,comm'], stdout=self.PIPE, stderr=self.PIPE) > > Really? Python doesn't offer us a better way than running ps? I guess not: http://ubuntuforums.org/showthread.php?t=463850 This is what the internets recommended. > > Tools/Scripts/webkitpy/common/system/executive.py:311 > > + return running_pids > > Do we need to sort these? Might add a comment which explains why we don't. Does this handle pid-wrapping? do we care? I've sorted it. I'm sure it matters that much. I suspect that we'll have some race conditions because of parallelism, but we'll have to deal with them when they occur. > > Tools/Scripts/webkitpy/common/system/executive.py:323 > > + while self.check_running_pid(pid): > > + time.sleep(0.25) > > Can't we just waitpid? I guess on unix it has to be a child process? I wonder if the same restriction applies to windows? Under the covers, check_running_pid does what the perl code used to do and what the Internet recommends. The perl code specifically has a comment about not using waitpid for this purpose.

Created attachment 98751 [details] Patch

Created attachment 98757 [details] Patch

Comment on attachment 98757 [details] Patch Missing your new file. ;(

Created attachment 98759 [details] Patch

Comment on attachment 98759 [details] Patch LGTM.

Committed r89843: <http://trac.webkit.org/changeset/89843>