Test for Brotli decompressor#2066
Conversation
Signed-off-by: Charles Le Borgne <cleborgne@google.com>
Signed-off-by: Charles Le Borgne <cleborgne@google.com>
Signed-off-by: Charles Le Borgne <cleborgne@google.com>
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Looks good to me. (Modulo one question and one optional nit). The issues with the content-encoding header is a curious one...
|
|
||
| #include "source/common/upstream/logical_dns_cluster.h" | ||
| #include "source/extensions/clusters/dynamic_forward_proxy/cluster.h" | ||
| #include "source/extensions/compression/brotli/decompressor/config.h" |
There was a problem hiding this comment.
Up to you, but I think I'd be inclined to add this to the .cc file, not the .h file. (Though many similar includes seem to be in the .h file already)
There was a problem hiding this comment.
Admittedly, I'm going with the "monkey see, monkey do" strategy - my sense of where a .h should be included is not developed yet. That strategy has one virtue, though: it is consistent (good or bad).
If you can tell me what is the strategy here, I can rather move all the offending includes.
There was a problem hiding this comment.
Yeah, that's a great strategy :) My general philosophy is to only put an include in the .h file if it's needed for the .h file itself. In this case, it appears that the only "code" in the .h file is a declaration of a function with no arguments that returns void. So I would think that it shouldn't need any includes. Does it still build if you move all of 'em?
There was a problem hiding this comment.
I moved them all - thanks!
| * @return the builder to facilitate chaining. | ||
| */ | ||
| @VisibleForTesting | ||
| public CronetEngineBuilderImpl setMockCertVerifierForTesting() { |
There was a problem hiding this comment.
It looks like this logic is (roughly) moving from CronetEngineBuilder to NativeCronetEngineBuilderImpl. Out of curiosity, what's that motivation?
There was a problem hiding this comment.
I moved this here because this is an EnvoyMobile enum that is being use, and this is the only class bridging EM and Cronet for the Configuration.
The second weaker reason is that CronetEngineBuilderImpl (where that method was before this PR) is also used by the Java implementation of Cronet, where it makes no sense.
Signed-off-by: Charles Le Borgne <cleborgne@google.com>
Signed-off-by: Charles Le Borgne <cleborgne@google.com>
|
ios_build / swift_async_await is very flaky https://github.com/envoyproxy/envoy-mobile/runs/5478268352?check_suite_focus=true Kicking CI again. |
|
/retest |
| #include "library/common/extensions/filters/http/network_configuration/config.h" | ||
| #include "library/common/extensions/filters/http/platform_bridge/config.h" | ||
| #include "library/common/extensions/filters/http/route_cache_reset/config.h" | ||
| #include "library/common/extensions/retry/options/network_configuration/config.h" |
goaway
left a comment
There was a problem hiding this comment.
LGTM. Thanks @carloseltuerto!
* origin/main: [CI] Only install Android tool dependencies when needed (#2095) update envoy to latest (#2094) [CI] Fix DCO in support rotation bump PRs (#2092) Fix iOS async/await example app (#2093) Test for Brotli decompressor (#2066) docs: fix day (#2089) engine builder: add option to add a list of H2-Raw domain names (#2088) docs: adding the community meeting (#2083) Signed-off-by: JP Simard <jp@jpsim.com>
@goaway : adds a hardcoded dependency to Envoy-Mobile: Brotli Decompressor extension.
Hack: The YAML config for the Brotli Decompressor is in Cronvoy only - the support for Brotli decompressor is not yet available in config.cc
BrotliTest.java in this PR is almost a perfect copy of https://source.chromium.org/chromium/chromium/src/+/main:components/cronet/android/test/javatests/src/org/chromium/net/BrotliTest.java
Description: Adds Brotli support for Cronvoy
Risk Level: Small.
Testing: BrotliTest ported from Chromium, CI
Docs Changes: N/A
Release Notes: N/A
Signed-off-by: Charles Le Borgne cleborgne@google.com