Skip to content

Conversation

@rymis
Copy link
Contributor

@rymis rymis commented Feb 21, 2020

As long as httplib has gzip support it is easy to add deflate. I did it with automatic encoding detection because some clients could compress content with GZip and to set deflate compression instead of correct gzip.

As a discussion starting point, I'd like to add ContentTranscoder abstraction to encode and decode custom streams, like LZ4, Snappy, etc.

@yhirose
Copy link
Owner

yhirose commented Feb 22, 2020

@rymis, thank you for the pull request. Could you explain more about "because some clients could compress content with GZip and to set deflate compression instead of correct gzip"? I would like to stick to the HTML spec unless the behavior is a widely accepted common practice. Thanks!

@rymis
Copy link
Contributor Author

rymis commented Feb 24, 2020

Sorry, I thought a little and realized, I was wrong. I read that some client library was sending incorrect Content-Encoding somewhere on stackoverflow, but I think it is not wise to support this behavior. I'll fix the MR to be standard complient. The actual RFC looks like:

4.2.2.  Deflate Coding

   The "deflate" coding is a "zlib" data format [RFC1950] containing a
   "deflate" compressed data stream [RFC1951] that uses a combination of
   the Lempel-Ziv (LZ77) compression algorithm and Huffman coding.

      Note: Some non-conformant implementations send the "deflate"
      compressed data without the zlib wrapper.


4.2.3.  Gzip Coding

   The "gzip" coding is an LZ77 coding with a 32-bit Cyclic Redundancy
   Check (CRC) that is commonly produced by the gzip file compression
   program [RFC1952].  A recipient SHOULD consider "x-gzip" to be
   equivalent to "gzip".

httplib.h Outdated
strm.opaque = Z_NULL;

// 15 is the value of wbits, which should be at the maximum possible value
// to ensure that any gzip stream can be decoded. The offset of 16 specifies
Copy link
Owner

@yhirose yhirose Feb 24, 2020

Choose a reason for hiding this comment

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

Since '32' is now used for the automatic stream type detection, this comment ("The offset of 16 specifies...") is no longer valid. Could you update it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for sure

@yhirose
Copy link
Owner

yhirose commented Feb 24, 2020

@rymis, I checked the revised code and looks good to me except the comment for inflateInit2(&strm, 32 + 15) . Could you update it? Thanks!

@yhirose
Copy link
Owner

yhirose commented Feb 24, 2020

Also one more request. Could you squash all the commits into a commit, so that the commit history could be simpler?

Copy link
Owner

@yhirose yhirose left a comment

Choose a reason for hiding this comment

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

I believe the comment for inflateInit2 should be like this:

    // 15 is the value of wbits, which should be at the maximum possible value
    // to ensure that any gzip stream can be decoded. The offset of 32 specifies
    // that the stream type should be automatically detected either gzip or deflate.
    is_valid_ = inflateInit2(&strm, 32 + 15) == Z_OK;	 

Also I just wonder why the unit test case GetMethod200withPercentEncoding sneaked in...

@rymis
Copy link
Contributor Author

rymis commented Feb 24, 2020

It's because of squashing of commits. It came from master :( I'll change the comment.

@yhirose
Copy link
Owner

yhirose commented Feb 24, 2020

Actually I can squash the commits on my side. Thanks for the adjustments.

@yhirose yhirose merged commit f2bb9c4 into yhirose:master Feb 24, 2020
@yhirose
Copy link
Owner

yhirose commented Feb 24, 2020

I have just merged it into the master. Thanks for your contribution!

@rymis
Copy link
Contributor Author

rymis commented Feb 24, 2020

Thanks!

yhirose pushed a commit that referenced this pull request May 2, 2020
ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants