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

Improved WebUtils.ParseQueryString() #15245

Merged
merged 2 commits into from May 28, 2023

Conversation

jstedfast
Copy link
Member

Description of Change

  • Reduce memory allocations
  • Decode parameter values

Issues Fixed

Fixes issue #9223

@@ -1,45 +1,50 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text;

namespace Microsoft.Maui.ApplicationModel
{
static class WebUtils
{
internal static IDictionary<string, string> ParseQueryString(string url)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some standard System.Uri parsing code we can call instead of writing our own?

It seems like there should be a BCL way to return a dictionary of key/value pairs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like there should be a class for this in the BCL but afaik, there isn't.

It is definitely not part of Uri.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jstedfast jstedfast force-pushed the dev/jestedfa/webauthenticatorresults-decode-hex branch from 836612c to 34de1eb Compare May 24, 2023 17:42
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@jstedfast jstedfast force-pushed the dev/jestedfa/webauthenticatorresults-decode-hex branch 3 times, most recently from 8648d2f to 67cba8b Compare May 25, 2023 18:08
@Eilon Eilon added the area/essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label May 25, 2023
@jstedfast jstedfast force-pushed the dev/jestedfa/webauthenticatorresults-decode-hex branch from 67cba8b to f7cc37c Compare May 25, 2023 19:18
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

Comment on lines 52 to 53
var span = query.AsSpan(valueIndex, index - valueIndex);
var chars = new char[span.Length];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how we got optimized Span<T> code now for free!

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK to me now, assuming CI is green.

* Reduce memory allocations
* Decode parameter values

Fixes issue #9223
@jstedfast jstedfast force-pushed the dev/jestedfa/webauthenticatorresults-decode-hex branch from 1cb675e to edeae34 Compare May 25, 2023 19:48
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@jstedfast
Copy link
Member Author

@jonathanpeppers looks like the failure is due to an old version of Xcode. What do we do to progress on this?

@jonathanpeppers
Copy link
Member

Someone on the MAUI team can check? Maybe @rmarinho?

@mattleibow I think you should review, too -- this is something in Essentials.

@rmarinho
Copy link
Member

it's not related we can move this and ignore that failure. We are still looking on a fix.

@jstedfast jstedfast merged commit 9af4ef9 into main May 28, 2023
28 of 29 checks passed
@jstedfast jstedfast deleted the dev/jestedfa/webauthenticatorresults-decode-hex branch May 28, 2023 14:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants