-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Optimize JS escaping #27528
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
Optimize JS escaping #27528
Conversation
ac9a6dc to
63a4b56
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Looks like I need some conditional compilation to make the Regex source generation work. |
|
Hopefully it is good now |
src/Controls/tests/DeviceTests/Elements/WebView/WebViewTests.cs
Outdated
Show resolved
Hide resolved
mattleibow
left a comment
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.
I see this code is duplicated. Could we move it to a webview utils and then both controls use it? I think there will be a couple of other things I will add in the future. I already noticed some with hybrid web view. So a separate class will be useful.
|
ok, it is now using a common helper class |
|
@mattleibow it would be nice to see this merged, all the platforms could benefit |
|
/rebase |
25ec409 to
1390efe
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
PureWeen
left a comment
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.
Build failing
|
@mattleibow @PureWeen friendly ping, @symbiogenesis committed with a fix, please run the pipeline again |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@copilot can you fix this error? D:\a\1\s\src\Core\src\Handlers\WebView\WebViewHelper.cs(16,20): error CS1503: Argument 1: cannot convert from 'char' to 'string' [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=netstandard2.0] |
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.
Pull Request Overview
This PR optimizes the JavaScript escaping logic to improve performance and reduce allocations.
- Introduces a centralized helper (WebViewHelper.EscapeJsString) that leverages conditional compilation and generated regex on .NET 6+
- Removes duplicate and less efficient JS escaping implementations from HybridWebViewHandler and WebView
- Updates and adds unit tests to validate the new escaping behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Core/src/Handlers/WebView/WebViewHelper.cs | New implementation using generated regex to optimize JS escaping |
| src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs | Replaces local JS escape logic with a call to WebViewHelper.EscapeJsString |
| src/Controls/tests/Core.UnitTests/WebViewTests.cs | Unit tests updated/added to cover the new JS escaping behavior |
| src/Controls/src/Core/WebView/WebView.cs | Removed duplicate local JS escaping and now reuses WebViewHelper.EscapeJsString |
Comments suppressed due to low confidence (2)
src/Controls/tests/Core.UnitTests/WebViewTests.cs:59
- Consider adding a test case for strings with multiple consecutive backslashes preceding a single quote to ensure the escaping logic handles these edge cases correctly.
var result = WebViewHelper.EscapeJsString(input);
src/Controls/src/Core/WebView/WebView.cs:293
- The removal of the local JS escaping logic in favor of a centralized helper improves consistency; please ensure that all use cases formerly handled by the local implementation are covered by the unit tests.
internal static string EscapeJsString(string js) => WebViewHelper.EscapeJsString(js);
Co-authored-by: MartyIX <[email protected]>
4dc6683 to
b98fdc4
Compare
|
/azp run MAUI-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* optimize JS escaping * add tests * copy to HybridWebView * use conditional compilation * Update src/Controls/tests/DeviceTests/Elements/WebView/WebViewTests.cs Co-authored-by: MartyIX <[email protected]> * Move tests * use common WebViewHelper class * add missing using * fix build * Fix CI * More tests --------- Co-authored-by: Edward Miller <[email protected]> Co-authored-by: MartyIX <[email protected]> Co-authored-by: Matthew Leibowitz <[email protected]>
* optimize JS escaping * add tests * copy to HybridWebView * use conditional compilation * Update src/Controls/tests/DeviceTests/Elements/WebView/WebViewTests.cs Co-authored-by: MartyIX <[email protected]> * Move tests * use common WebViewHelper class * add missing using * fix build * Fix CI * More tests --------- Co-authored-by: Edward Miller <[email protected]> Co-authored-by: MartyIX <[email protected]> Co-authored-by: Matthew Leibowitz <[email protected]>
* optimize JS escaping * add tests * copy to HybridWebView * use conditional compilation * Update src/Controls/tests/DeviceTests/Elements/WebView/WebViewTests.cs Co-authored-by: MartyIX <[email protected]> * Move tests * use common WebViewHelper class * add missing using * fix build * Fix CI * More tests --------- Co-authored-by: Edward Miller <[email protected]> Co-authored-by: MartyIX <[email protected]> Co-authored-by: Matthew Leibowitz <[email protected]>
* optimize JS escaping * add tests * copy to HybridWebView * use conditional compilation * Update src/Controls/tests/DeviceTests/Elements/WebView/WebViewTests.cs Co-authored-by: MartyIX <[email protected]> * Move tests * use common WebViewHelper class * add missing using * fix build * Fix CI * More tests --------- Co-authored-by: Edward Miller <[email protected]> Co-authored-by: MartyIX <[email protected]> Co-authored-by: Matthew Leibowitz <[email protected]>
Before:
After: