Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Port System.Net.WebUtility missing members #13875

Merged
safern merged 4 commits into from
Nov 22, 2016
Merged

Port System.Net.WebUtility missing members #13875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9bc38d
Port WebUtility missing members
safern Nov 21, 2016
2edb129
Port WebUtility missing html encoding and decoding members
safern Nov 22, 2016
411b2a1
Nit fix
safern Nov 22, 2016
0752110
PR feedback
safern Nov 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,7 +1532,9 @@ namespace System.Net
public static partial class WebUtility
{
public static string HtmlDecode(string value) { throw null; }
public static void HtmlDecode(string value, System.IO.TextWriter output) { }
public static string HtmlEncode(string value) { throw null; }
public static void HtmlEncode(string value, System.IO.TextWriter output) { }
public static string UrlDecode(string encodedValue) { throw null; }
public static byte[] UrlDecodeToBytes(byte[] encodedValue, int offset, int count) { throw null; }
public static string UrlEncode(string value) { throw null; }
Expand Down
14 changes: 12 additions & 2 deletions src/System.Runtime.Extensions/src/System/Net/WebUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public static class WebUtility

public static string HtmlEncode(string value)
{
if (String.IsNullOrEmpty(value))
if (string.IsNullOrEmpty(value))
{
return value;
}
Expand All @@ -50,6 +50,11 @@ public static string HtmlEncode(string value)
return StringBuilderCache.GetStringAndRelease(sb);
}

public static void HtmlEncode(string value, TextWriter output)
{
output.Write(HtmlEncode(value));
Copy link
Copy Markdown
Contributor

@justinvp justinvp Nov 22, 2016

Choose a reason for hiding this comment

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

This is sufficient to bring the functionality to .NET Core, but it can be improved... In desktop, the encoded chars are written directly to the TextWriter, whereas with this, a (potentially large) string is being allocated and then that string is written to the TextWriter. It'd be nice to avoid the string allocation.

In desktop, the shared implementation is in HtmlEncode(string, TextWriter) and HtmlEncode(string) allocates a StringWriter to pass to HtmlEncode(string, TextWriter). However, in .NET Core, since the other overload wasn't present, HtmlEncode(string) was changed to use StringBuilder (acquired from a StringBuilderCache) instead of using StringWriter.

This could be reworked to share an implementation, without having to switch back to the desktop implementation.

Something like...

public static string HtmlEncode(string value)
{
    ...

    StringBuilder sb = StringBuilderCache.Acquire(value.Length);
    HtmlEncode(value, index, sb, (w, s) => w.Append(s), (w, c) => w.Append(c));
    return StringBuilderCache.GetStringAndRelease(sb);
}

public static void HtmlEncode(string value, TextWriter output)
{
    ...

    HtmlEncode(value, index, output, (w, s) => w.Write(s), (w, c) => w.Write(c));
}

private static unsafe void HtmlEncode<TWriter>(string value, int index, TWriter writer, Action<TWriter, string> writeString, Action<TWriter, char> writeChar)
{
    ...

    // Use writeString(writer, "whatever") or writeChar(writer, 'w')
}

Of course, this could be done subsequently in a separate PR.

Copy link
Copy Markdown
Contributor

@AlexGhiondea AlexGhiondea Nov 22, 2016

Choose a reason for hiding this comment

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

This approach works. @safern and I discussed this before the PR went up.

The downside seems to be that it would make the existing code slower and would introduce a couple of allocations for the 2 lambdas.

Copy link
Copy Markdown
Contributor

@justinvp justinvp Nov 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

would introduce a couple of allocations for the 2 lambdas.

The compiler will lazily allocate and cache/reuse the lambda delegate instances.

Copy link
Copy Markdown
Member Author

@safern safern Nov 22, 2016

Choose a reason for hiding this comment

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

This would be better than the approach we discussed earlier, right? @AlexGhiondea

Just to give you a little bit of context @justinvp, we were thinking of creating an internal abstract class WriterBaseClass (this is just a random name), and making two other internal classes that would inherit from this one, those to classes would be:

SBWrapperand TextWriterWrapperwhere them would override virtual write(char)and write(string) methods from WriterBaseClassthat would write either to the StringBuilderor the TextWriter.

SBWrapper would override ToString and call StringBuilderCache.GetStringAndRelease(sb).

But I think your approach works better cause this one would have extra allocations as well and would have to make virtual calls.

Copy link
Copy Markdown
Contributor

@justinvp justinvp Nov 22, 2016

Choose a reason for hiding this comment

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

By the way, I noticed the same problem being solved using delegates in a recent PR:

corefx/src/System.Runtime.Extensions/src/System/Security/SecurityElement.cs

Lines 546 to 560 in 768e18c

public override string ToString()
{
StringBuilder sb = new StringBuilder();
ToString("", sb, (obj, str) => ((StringBuilder)obj).Append(str));
return sb.ToString();
}
internal void ToWriter(StreamWriter writer)
{
ToString("", writer, (obj, str) => ((StreamWriter)obj).Write(str));
}
private void ToString(string indent, object obj, Action<object, string> write)

Copy link
Copy Markdown
Contributor

@AlexGhiondea AlexGhiondea Nov 22, 2016

Choose a reason for hiding this comment

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

@safern I think they are just as good. Splitting into base classes makes it easier if you have more than 2 methods you would need to wrap. Otherwise you end up with a lot of lambdas you need to write :).

There should not be a lot of performance difference between calling via a delegate and calling via callvirt.

And if we don't think the original code is used on the hot path, then this becomes a question of what is the nicest way to write the code and @justinvp came up with an elegant one.

Copy link
Copy Markdown
Member Author

@safern safern Nov 22, 2016

Choose a reason for hiding this comment

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

Ok I see! Thanks for explaining :) @AlexGhiondea
Then I'll go for @justinvp solution!

Will update this PR.

Copy link
Copy Markdown
Contributor

@jamesqo jamesqo Nov 22, 2016

Choose a reason for hiding this comment

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

@justinvp Your solution seems like it would be slower for the main path, since an extra function call would be added for every char/string appended. Since it's more common to call the overload that returns a string rather than one that writes it to a TextWriter, it seems like what's currently checked in is the best solution.

}

private static unsafe void HtmlEncode(string value, int index, StringBuilder output)
{
Debug.Assert(value != null);
Expand Down Expand Up @@ -138,7 +143,7 @@ private static unsafe void HtmlEncode(string value, int index, StringBuilder out

public static string HtmlDecode(string value)
{
if (String.IsNullOrEmpty(value))
if (string.IsNullOrEmpty(value))
{
return value;
}
Expand All @@ -154,6 +159,11 @@ public static string HtmlDecode(string value)
return StringBuilderCache.GetStringAndRelease(sb);
}

public static void HtmlDecode(string value, TextWriter output)
{
output.Write(HtmlDecode(value));
}

[SuppressMessage("Microsoft.Usage", "CA1806:DoNotIgnoreMethodResults", MessageId = "System.UInt16.TryParse(System.String,System.Globalization.NumberStyles,System.IFormatProvider,System.UInt16@)", Justification = "UInt16.TryParse guarantees that result is zero if the parse fails.")]
private static void HtmlDecode(string value, StringBuilder output)
{
Expand Down
1 change: 1 addition & 0 deletions src/System.Runtime.Extensions/tests/System.Runtime.Extensions.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
<Compile Include="System\StringComparer.netstandard1.7.cs" />
<Compile Include="System\Runtime\Versioning\VersioningHelperTests.cs" />
<Compile Include="System\AppDomainTests.cs" />
<Compile Include="System\Net\WebUtility.netstandard1.7.cs" />
<Compile Include="$(CommonTestPath)\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs">
<Link>Common\System\Runtime\Serialization\Formatters\BinaryFormatterHelpers.cs</Link>
</Compile>
Expand Down
4 changes: 3 additions & 1 deletion src/System.Runtime.Extensions/tests/System/Net/WebUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Text;
using Xunit;

namespace System.Net.Tests
{
public class WebUtilityTests
public partial class WebUtilityTests
{
// HtmlEncode + HtmlDecode
public static IEnumerable<object[]> HtmlDecode_TestData()
Expand Down
35 changes: 35 additions & 0 deletions src/System.Runtime.Extensions/tests/System/Net/WebUtility.netstandard1.7.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Globalization;
using System.IO;
using Xunit;

namespace System.Net.Tests
{
public partial class WebUtilityTests
{
[Theory]
[MemberData(nameof(HtmlDecode_TestData))]
public static void HtmlDecode_TextWriterOutput(string value, string expected)
{
if(value == null)
expected = string.Empty;
StringWriter output = new StringWriter(CultureInfo.InvariantCulture);
WebUtility.HtmlDecode(value, output);
Assert.Equal(expected, output.ToString());
}

[Theory]
[MemberData(nameof(HtmlEncode_TestData))]
public static void HtmlEncode_TextWriterOutput(string value, string expected)
{
if(value == null)
expected = string.Empty;
StringWriter output = new StringWriter(CultureInfo.InvariantCulture);
WebUtility.HtmlEncode(value, output);
Assert.Equal(expected, output.ToString());
}
}
}