jni: update java library interfaces#326
Conversation
| initialize((ConnectivityManager)context.getSystemService(Context.CONNECTIVITY_SERVICE)); | ||
| engine = new EnvoyEngine(); | ||
| initialize((ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE)); | ||
| envoyEngine = new EnvoyEngineImpl(); |
There was a problem hiding this comment.
Assuming an upcoming PR will allow for specifying a custom/mockable implementation of EnvoyEngine
There was a problem hiding this comment.
Yup. This isn't as important at the moment because we're not "using" the new interfaces within our example applications. I plan to take some time to update this when we do a full end to end wire up
library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java
Outdated
Show resolved
Hide resolved
library/java/src/io/envoyproxy/envoymobile/engine/EnvoyHTTPStream.java
Outdated
Show resolved
Hide resolved
library/java/src/io/envoyproxy/envoymobile/engine/types/EnvoyObserver.java
Outdated
Show resolved
Hide resolved
library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java
Outdated
Show resolved
Hide resolved
library/java/src/io/envoyproxy/envoymobile/engine/AndroidEngine.java
Outdated
Show resolved
Hide resolved
|
Overall I think this is great starting point for implementing the Android bridging logic. I'm generally in favor of moving forward and merging it. See comments above. :) |
246a2f8 to
8beebf7
Compare
3f8c4e4 to
3a631e6
Compare
junr03
left a comment
There was a problem hiding this comment.
Just one small change, otherwise lgtm
library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java
Outdated
Show resolved
Hide resolved
library/java/src/io/envoyproxy/envoymobile/engine/JniLibrary.java
Outdated
Show resolved
Hide resolved
3a631e6 to
768e647
Compare
goaway
left a comment
There was a problem hiding this comment.
Ok, I pulled up the actual source from this branch and it's fitting together better in my head now. It looks good.
(I don't think we necessarily need all the types defined in types/, since I think in some cases it makes more sense to bridge to a higher-level construct, but I also think it's fine for now. It also doesn't directly pertain to this PR.)
IMO, let's proceed and iterate on top of this.
|
@goaway The original intent of the data structures defined in Open to changing it and I agree it's separate from this PR |
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
|
From conversation in slack, after consideration, I agree we should keep the stuff in |
768e647 to
db50eb8
Compare
|
thanks @goaway ! |
Makes some relatively minor updates to match up with the JNI interfaces being checked in with envoyproxy/envoy-mobile#326. This diff was larger before, but much of it got checked in with envoyproxy/envoy-mobile#329. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
Makes some relatively minor updates to match up with the JNI interfaces being checked in with envoyproxy/envoy-mobile#326. This diff was larger before, but much of it got checked in with envoyproxy/envoy-mobile#329. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
Updating java library layer to conform to the streaming interfaces changes. Also, tweaks to make the java layer more consistent with the ObjC layer in #327
Signed-off-by: Alan Chiu achiu@lyft.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Description: Updating java library interfaces
Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]