add brotli support to Compression and Decompression stream #6060
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6060 +/- ##
==========================================
- Coverage 70.37% 70.31% -0.06%
==========================================
Files 408 408
Lines 108811 108913 +102
Branches 18000 18030 +30
==========================================
+ Hits 76574 76584 +10
- Misses 21430 21515 +85
- Partials 10807 10814 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It seems the wpt's were added prematurely without being marked tentative. It's good to have this ready but let's hold off landing until it lands in the actual spec. |
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
Aside from the fact that brotli has not been added to the spec yet, several issues with the impl as is. It looks like there are some bits here in internal.c++ that have nothing to do with brotli that are possibly breaking. Those would be separated out to a different pr.
| break; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
This feels like an odd way to structure this when there's already a switch body here. Seems they can be coalesced.
| if (format == "deflate") return Format::DEFLATE; | ||
| if (format == "deflate-raw") return Format::DEFLATE_RAW; | ||
| if (format == "brotli") return Format::BROTLI; | ||
| KJ_UNREACHABLE; |
There was a problem hiding this comment.
This isn't really unreachable. This should probably throw a proper exception, just in case
| static int getWindowBits(kj::StringPtr format) { | ||
| void initBrotli() { | ||
| if (mode == Mode::COMPRESS) { | ||
| auto* instance = BrotliEncoderCreateInstance( |
There was a problem hiding this comment.
It would be more idiomatic to wrap the unit/cleanup into a smart pointer
| size_t brotliAvailIn = 0; | ||
| // Brotli state structs are opaque, so kj::Own would require complete types. | ||
| BrotliEncoderState* brotliEncoderState = nullptr; | ||
| BrotliDecoderState* brotliDecoderState = nullptr; |
There was a problem hiding this comment.
Definitely not a fan of the use of raw pointers in here. This should be using kj::Owns and kj::Arrays etc.
| JSG_REQUIRE( | ||
| format == "deflate" || format == "gzip" || format == "deflate-raw" || format == "brotli", | ||
| TypeError, | ||
| "The compression format must be either 'deflate', 'deflate-raw', 'gzip', or 'brotli'."); |
There was a problem hiding this comment.
Better to call the function that parses these and have that throw the error directly to avoid the double string comparisons
| // This case caused me a moment of confusion during testing, so I think it's worth | ||
| // a specific error message. | ||
| throwTypeErrorAndConsoleWarn( | ||
| return rejectInvalidChunk( |
There was a problem hiding this comment.
This is a behavioral change that might require a compat flag I believe
3 participants
Adds brotli support to CompressionStream and DecompressionStream. After this change, almost all of the compression web-platform tests pass.