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

Unify Windows shared source for BlazorWebView #4159

Merged
merged 6 commits into from
Jan 17, 2022
Merged

Conversation

Eilon
Copy link
Member

@Eilon Eilon commented Jan 17, 2022

The WebView2 (Windows) code for BlazorWebView had two versions of the code that had drifted apart significantly. In the long term this could easily lead to bugs where only one version is updated and a bug remains in the other.

Why were there ever two versions? The WebView2 types have different identities between WPF+WinForms and WinUI. This means one codebase can't run on both. The original solution was to create abstractions, e.g. ICoreWebView2 and then implement one for WPF+WinForms, and one for WinUI. That was somewhat acceptable up until the code needed to start diverging dramatically due to WinUI WebView2 having very different patterns for doing file access (100% async), which necessitated having diverging APIs between the two systems. So, the code for the wrappers was copied and modified, creating a divergent implementation.

What now? I unified all the duplicated code using shared source and where deviations were necessary, I used my old friend #ifdef. It turns out there wasn't that much deviation, but the code is now significantly cleaner because all the WebView2 APIs are called directly instead of via wrappers.

Summary: This PR should contain zero functional changes and zero new code paths except for the omission of doing everything through wrappers.

@Eilon Eilon added the area-blazor Blazor Hybrid / Desktop, BlazorWebView label Jan 17, 2022
@Eilon Eilon requested a review from a team January 17, 2022 07:23
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Eilon Eilon enabled auto-merge (squash) January 17, 2022 15:07
@Eilon Eilon merged commit 36d7e0e into main Jan 17, 2022
@Eilon Eilon deleted the eilon/fix-shared-source branch January 17, 2022 15:10
@SteveSandersonMS
Copy link
Member

Wow, this is an immense improvement. So much deleted code!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Blazor Hybrid / Desktop, BlazorWebView
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants