-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Use Environment.ProcessPath instead of Process.GetCurrentProcess().Pr… #10998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10998 +/- ##
===================================================
- Coverage 13.42545% 13.42425% -0.00120%
===================================================
Files 3319 3319
Lines 664894 664894
Branches 74674 74674
===================================================
- Hits 89265 89257 -8
- Misses 573084 573092 +8
Partials 2545 2545
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
@@ -203,7 +203,7 @@ internal static void LogStartHWND(IntPtr hwnd, string fromWhere) | |||
{ | |||
string msg = String.Format("BEGIN: {0:X} -- Setting DWP, process = {1} ({2}) {3}", | |||
hwnd, | |||
System.Diagnostics.Process.GetCurrentProcess().ProcessName, | |||
Path.GetFileNameWithoutExtension(Environment.ProcessPath), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Environment.ProcessPath
will call the Kernel32.GetModuleFileNameW
, see https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamew
System.Diagnostics.Process.GetCurrentProcess().ProcessName
will call Kernel32.QueryFullProcessImageNameW
, see https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamew
I do not think this is the same behavior. But I believe it is the same behavior in the vast majority of cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Process.GetCurrentProcess() without using
will left a Process instance after each calling.
In certain scenarios with high call volume, this pr will make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I believe it is the same behavior in the vast majority of cases.
For WPF both .NET APIs return the same result. There is no difference. Only for other .NET scenarios (not WPF related: framework dependend when running from dotnet.exe, browser etc) there is a difference between Environment.ProcessPath and ProcessName. Yes, they call different winapis but for WPF the result is the same in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One difference is when the file is renamed while the process is running. But the discussion is moot as per the PR discussion.
I agree this is a compatibility risk. |
@miloush This is the edge module code, and it only effect the log message. So that I think it is ok. |
Which should be discussed in the issue description, together with how it was tested. You have to have a special build with LOGGING set, so the impact of this change is questionable. |
@miloush I think it is the debug code. |
Just want to point out that the contents of the wpf/src/Microsoft.DotNet.Wpf/src/Shared/MS/Win32/ManagedWndProcTracker.cs Lines 224 to 235 in 725fc8a
|
Fixes #
Main PR
Description
Replace Process.GetCurrentProcess().ProcessName with Path.GetFileNameWithoutExtension(Environment.ProcessPath) for better performance and cleaner code.
Customer Impact
Regression
Testing
Risk
Microsoft Reviewers: Open in CodeFlow