GPII-2578: Make more use of the process reporter#169
GPII-2578: Make more use of the process reporter#169stegru wants to merge 11 commits intoGPII:masterfrom
Conversation
# Conflicts: # gpii/node_modules/processHandling/processHandling.js # gpii/node_modules/processHandling/test/testProcessHandling.js # package-lock.json
|
CI job failed: https://ci.gpii.net/job/windows-tests/420/ |
|
CI job passed: https://ci.gpii.net/job/windows-tests/421/ |
|
CI job passed: https://ci.gpii.net/job/windows-tests/422/ |
klown
left a comment
klown
left a comment
There was a problem hiding this comment.
Looks good to me -- thanks Steve! I have a few minor suggestions.
| var hProcess = windows.kernel32.OpenProcess(c.PROCESS_TERMINATE, 0, pids[n]); | ||
| var procs = processesBridge.findProcessesByCommand(filename); | ||
|
|
||
| if (procs) { |
There was a problem hiding this comment.
findProcessesByCommand() always returns an array, even if it's empty. No need to check if procs is non-null
| ManagementObject[] someProcesses = Array.FindAll( | ||
| processes, p => p.GetPropertyValue("Name").ToString() == input | ||
| processes, | ||
| p => string.Equals(p.GetPropertyValue("Name").ToString(), input, StringComparison.OrdinalIgnoreCase) |
There was a problem hiding this comment.
Thanks for catching this. I had no idea that Windows processes were case insensitive. Live and learn.
There was a problem hiding this comment.
For some unknown reason, the test-window.exe process was being reported as TEST-WINDOW.EXE.
| args: ["{arguments}.0"] | ||
| // process name or id, optional. | ||
| }, | ||
| compareProcessNames: { |
There was a problem hiding this comment.
I'm wondering if this belongs upstream in universal as part of the "gpii.processes" base grade. I don't see anything Windows specific about it. The implementation uses the standard JavaScript String.toLower() function. Is there anything OS specific here?
There was a problem hiding this comment.
I was on the fence with this.
My reason was the code isn't windows-specific, but what it does is specific. But, it does seem to be a lot of "fluff" just for a === statement - especially considering the "real" comparison is in the .csx file.
I've moved the actual checking into universal, but left the decision of ignoring the case in windows.
There was a problem hiding this comment.
That makes sense.
The macOS file system can be either case insensitive or case sensitive depending on how it's configured (factory setting is insensitive). I don't know about Android or iOS, but I suspect the former is sensitive and the latter is insensitive. This is an invoker that's context dependent (infusion-speak).
| "Node commmand name", "node.exe", aProcInfo.command | ||
| ); | ||
| }); | ||
| var procInfo = processesBridge.findFirstProcessByCommand("NoDE.eXe"); |
There was a problem hiding this comment.
Love it! Someone hiccoughed typing "nOde.ExE". :-)
| var exe = gpii.windows.getProcessPath(gpii.windows.getWindowProcessId(hwnd)); | ||
|
|
||
| var proc = processesBridge.findProcessByPid(gpii.windows.getWindowProcessId(hwnd)); | ||
| var exe = proc && proc.fullPath; |
There was a problem hiding this comment.
After seeing at least two occurrences of proc && proc.fullPath, my gut says to add a getProcessPath() invoker to the base grade in universal.
There was a problem hiding this comment.
Cool, I'll go give that pull request a peek.
|
CI job passed: https://ci.gpii.net/job/windows-tests/429/ |
| var procs = processesBridge.findProcessesByCommand(filename); | ||
|
|
||
| if (procs) { | ||
| for (var n = 0, len = procs.length; n < len; n++) { |
There was a problem hiding this comment.
I should have thought of this before, but ... using an iterator like fluid.each() here would be more transparent.
|
Closing: This Pr is a subset of #170 |
3 participants
There's some overlap with processHandling and processReporter. This PR removes functions from processHandling that have been implemented in processReporter.
processHandling now (mostly) just contains process actions.