Skip to content
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

Remove XBAP-related dead code from Application/BrowserInteropHelper, finish cleanup #9865

Merged
merged 10 commits into from
Jan 3, 2025

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Oct 1, 2024

Description

This continues the work started in #9855, removing rest of the internal properties in BrowserInteropHelper and thanks to this we can remove dead code in other places of the codebase (Application, AppSecurityManager, etc.) I've separated the two as those deletions might be a bit harder to grasp, while the first one is easier to understand imho.

  • SiteOfOriginContainer.BrowserSource is never set, thus it is always NULL.
    • This means the public property Uri in BrowserInteropHelper in always NULL.
    • GetReferer method in BindUriHelper depends on this value, if BrowserSource is NULL, it returns NULL.
    • So the value in httpRequest.Referer of WpfWebRequestHelper would end up being NULL anyways, no-op / dead code.
  • Application.MimeType is never set, thus it is always MimeType.Unknown.
    • This means IsViewer of BrowserInteropHelper is always false.
    • That also means that IsInitialViewerNavigation is always false and its underlying value does not matter at all for any operations. So we can remove any setters; and then we can remove it from both conditions; which makes it unused.
  • Application.ServiceProvider is never set (classes doing it don't exist anymore), so it is always NULL.
  • Classes calling the GetService also don't exist anymore, so the whole method does not matter.
  • Any methods removed in AppSecurityManager just call themselves, besides that we were still calling ClearSecurityManager from Application but it is never initialized anymore so there's nothing to clear.
  • As a result, SecurityMgrSite is never initialized anymore, so we can remove it as well.

Customer Impact

Decreased size of assemblies, tiny bit of perf, cleaner codebase for developers.

Regression

No.

Testing

Local build, app run.

Risk

Low, the code is a dead code or it doesn't do anything.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners October 1, 2024 08:28
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Oct 1, 2024
@h3xds1nz h3xds1nz force-pushed the remove-more-xbap-code branch from 318c2f7 to 6c8931f Compare October 1, 2024 21:07
@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Oct 1, 2024

Looks like one of the changes in #9220 was here. Rebased and resolved merge conflicts.

@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Oct 7, 2024

Resolved merge conflicts 01

@h3xds1nz h3xds1nz force-pushed the remove-more-xbap-code branch from d562996 to 4337790 Compare October 11, 2024 07:28
@h3xds1nz
Copy link
Contributor Author

Fix merge conflicts after #9521

@h3xds1nz
Copy link
Contributor Author

Resolved merge conflicts after #9855

@harshit7962 Can we go with this one and #9943 next?

@harshit7962
Copy link
Member

@h3xds1nz, We are done with the test pass of #9943 and will prioritize it. I am yet to review this PR and will include it in the next test run.

@h3xds1nz
Copy link
Contributor Author

@harshit7962 Awesome, let me know at which part you'd like me to resolve the conflicts.

I feel its counter-productive to do it now should a style-PR get merged meanwhile as that will mess up the resolutions.

@harshit7962
Copy link
Member

Sure, will let you know when the PR is ready to be merged. It would be better to resolve the conflicts at that time.

harshit7962
harshit7962 previously approved these changes Dec 31, 2024
@harshit7962
Copy link
Member

@h3xds1nz, we are done with the test pass and its all green. We are ready to take this change in.

@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Jan 3, 2025

@h3xds1nz, we are done with the test pass and its all green. We are ready to take this change in.

@harshit7962 Great, should be merged/rebased properly I hope.
I can't compile right now so I have to verify with CI I did pick the correct usings.

@harshit7962 harshit7962 merged commit 605ebf9 into dotnet:main Jan 3, 2025
8 checks passed
@harshit7962
Copy link
Member

@h3xds1nz, thanks for the contribution.

We are working on getting in the PRs that have passed the tests. Please let us know if any of these you would like us to prioritize.

Also, feel free to mention any PR that you would like us to include in the next test run.

@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Jan 3, 2025

@harshit7962 Great, glad to see this one in.

The untested ones in order of importance (perf gains):

  1. Remove allocations on all base converters, improve TokenizerHelper #9364
  2. Update PropertyValues in TriggerBase/FrameworkElementFactory via reference #10003
  3. Remove double lookups on SizeLimitedCache<K, V>, improve performance #9949
  4. Replace ArrayList in DataFormats with List<DataFormat> and refactor the class #9199
  5. Simplify CriticalCopyPixels in BitmapSource by removing duplicate type checks #9395
  6. Optimize FontSourceCollection creation from a filesystem directory, reduce allocs #9844 (this is a big gain but the customer impact is lower as its more of an edge case)

Do note there are two unit test PRs that could be merged imho:

  1. Add test coverage for public API surface of TextDecorationCollectionConverter #9895
  2. Add test coverage for public API surface of MouseActionConverter #9891 (this one includes a fix that slipped in through the WPF tests)

Regarding the ones that have passed tests so I can do further work/would be good to merge:

  1. Optimize ResourceContainer/AssemblyLoadEventHandler methods, remove allocs #9822
    1.1) Optimize retrieval of simple name from Assembly.FullName, fix up faulty methods #9739
  2. Optimize parsing of color strings into KnownColor enum, remove allocations #9669
  3. Replace occurrences of params T[] with params ReadOnlySpan<T> where possible #9481 (@dipeshmsft was reviewing it)
  4. Modify AvTrace call chain to use params ReadOnlySpan<object> instead of an array #9468 (this provides a regression fix)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants