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

Add DecompressionMethods.Brotli? #24986

Closed
stephentoub opened this issue Feb 9, 2018 · 8 comments
Closed

Add DecompressionMethods.Brotli? #24986

stephentoub opened this issue Feb 9, 2018 · 8 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Milestone

Comments

@stephentoub
Copy link
Member

System.Net.DecompressionMethods is an enum that is used by HttpClient handlers to determine whether/what to automatically decode in a response, e.g. if DecompressionMethods.Deflate is set and the response is encoded with deflate, it'll automatically decode the response as part of handing it back.

The enum today is just:

namespace System.Net
{
    [Flags]
    public enum DecompressionMethods
    {
        None = 0,
        GZip = 1,
        Deflate = 2
    }
}

but we now also have BrotliStream, which we could use to support a DecompressionMethods.Brotli and a "br" encoding:

namespace System.Net
{
    [Flags]
    public enum DecompressionMethods
    {
        ...
        Brotli = 4
    }
}

From an implementation perspective, this would mean either System.Net.Http.dll taking a dependency on System.IO.Compression.Brotli.dll, or if we wanted to avoid the dependency, building all of the source into System.Net.Http.dll (there is currently a native dependency, with the native portion compiled into clrcompression.dll on Windows and System.IO.Compression.Native.so/dylib on Unix).

@jnm2
Copy link
Contributor

jnm2 commented Feb 16, 2018

I knew I wasn't wrong to wish for a DecompressionMethods.All member. Since that sums up exactly what I care about.

@ghost
Copy link

ghost commented Feb 27, 2018

    Brotli = 4

@stephentoub, should this be 3 instead?

Also, can we also have All = ~None option as @jnm2 stated?

@stephentoub
Copy link
Member Author

should this be 3 instead?

No, it's a [Flags] enum. It needs to be a unique bit.

@joshfree
Copy link
Member

@karelz can we put this on the short list for 2.2 as part of finishing the brotli e2e?

Related aspnet/BasicMiddleware#217

@karelz
Copy link
Member

karelz commented Apr 20, 2018

Sure why not. We didn't review issues for 2.2 yet. So anything in Future is potential 2.2 at this point.

@jnm2
Copy link
Contributor

jnm2 commented Apr 20, 2018

I am very interested in an All member. Would you please review that possibility?

@stephentoub
Copy link
Member Author

@jnm2, please open a separate issue.

@jnm2
Copy link
Contributor

jnm2 commented Apr 20, 2018

Certainly! Sorry for missing the approval tag on this one.

@stephentoub stephentoub self-assigned this May 15, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net
Projects
None yet
Development

No branches or pull requests

5 participants