mozilla
uniffies viaduct
Converts viaduct to use uniffi.
It ended up being more straightforward than I thought and I had the capacity so 🤷
The biggest thing to consider is how this impacts desktop. Desktop uses viaduct, see https://searchfox.org/mozilla-central/source/toolkit/components/viaduct
My gut is telling me that I should add back all the deleted code here (pretty easy to add back) and put it under a cargo feature, let's say gecko and have it disabled by default. Then use conditional compilation to decide whether we use uniffi or the old way of doing things.
A few things before this is ready to land:
- [ ] Need to make sure desktop is well supported as I've mentioned above
- [ ] A bit more testing on fenix, tested it and it seems fine sending network requests, but I've changed a few things since and I wanna try a few edge cases (how are failures handled, slow network, responses/requests with large bodies, etc)
- [ ] Need to make sure the threading model here is OK, not 100% convinced yet, but I think it is.. just need to think it through a little
Codecov Report
Merging #4741 (c99ff96) into main (2f18703) will increase coverage by
0.32%. The diff coverage is6.77%.
@@ Coverage Diff @@
## main #4741 +/- ##
==========================================
+ Coverage 80.53% 80.85% +0.32%
==========================================
Files 48 48
Lines 5220 5203 -17
==========================================
+ Hits 4204 4207 +3
+ Misses 1016 996 -20
| Impacted Files | Coverage Δ | |
|---|---|---|
| components/viaduct/src/backend/ffi.rs | 0.00% <0.00%> (ø) |
|
| components/viaduct/src/lib.rs | 29.57% <0.00%> (-3.76%) |
:arrow_down: |
| components/viaduct/build.rs | 100.00% <100.00%> (ø) |
|
| components/viaduct/src/backend.rs | 88.88% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 2f18703...c99ff96. Read the comment docs.
The biggest thing to consider is how this impacts desktop. Desktop uses viaduct, see https://searchfox.org/mozilla-central/source/toolkit/components/viaduct
Ouch, hrmph. I'll dig deeper, but we should be aggressive here - it would be a shame to revert alot of this code and end up with even more moving parts than now and I don't think the usage is particularly widespread yet. cc @bendk
Ouch, hrmph. I'll dig deeper, but we should be aggressive here - it would be a shame to revert alot of this code and end up with even more moving parts than now and I don't think the usage is particularly widespread yet. cc @bendk
Like, if we had a C++ UniFII backend? I feel like there's a red flag waving in front of me :)
I don't think this code changes very often and there's no pressing need to merge this. What if we held it here in a PR for a while and see if we could get something working on desktop?
I don't think this code changes very often and there's no pressing need to merge this. What if we held it here in a PR for a while and see if we could get something working on desktop?
Yeah I'm very happy to keep this on hold, maybe early in the new year we can explore what it would take to have the C++ code to consume this new version of viaduct 🤷
it would be a shame to revert alot of this code and end up with even more moving parts than now and I don't think the usage is particularly widespread yet.
Usage isn't widespread yet! But glean on desktop is using it (not sure if there are other consumers though)
But could you use something like this?
body = this.body?.toByteArray().inputStream()
I nearly said the same thing, but I think that would be slightly tricky without changing semantics - the trick would be to avoid calling Request.Body with null (which would mean an extra null check)
this has grown awfully stale, I'll close it. Any effort to uniffi viaduct will likely look different given the implication on desktop