Embed default resources in Servo applications using a servo-default-resources crate#43182
Embed default resources in Servo applications using a servo-default-resources crate#43182jschwe merged 12 commits intoservo:mainfrom
servo-default-resources crate#43182Conversation
|
So, there is a bit of a tricky bit here:
I'm tending towards splitting resource protocol handler related things into a separate folder in our repository, and am considering having two different resource readers. One in The I'm also thinking about how to search for the resources directory, since "next to the binary" doesn't work well for the standard in-tree approach, and probably we need to have code to support both developer experience for in-tree builds, while also handling production builds appropriatly. |
|
The |
|
Perhaps we should just remove support for the "user-agent-js" directory. This seems like something that should be handled by the embedder using the |
In #43182 it was [suggested to remove the userscripts](#43182 (comment)) option, since embedders can use the (new) `UserContentManager` API instead. Testing: This removes a commandline flag from servoshell. Existing tests passing is sufficient. Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
04463a9 to
7810e2a
Compare
I pushed a proposal / draft for this, which uses mach (well an environment variable really) to determine if we are a developer build or not and use the in-tree resources. When packaging we package the named resources into the global resource directory, which servoshell will search for. I'm not really happy, but I'm also been going a bit in circles and don't have a great idea otherwise on how to load the resources from the embedder-traits (supporting both crates.io builds and local mach builds) when developing and from somewhere else when shipping a packaged servo. |
7810e2a to
b40690e
Compare
We want to bake the resources in, since it simplifies our tests. This requires us to ship the resources in the package. rippy.png is also referenced from image_cache - We simply duplicate it, since the image is small, to avoid adding unnecessarily complex solutions like adding a dedicated crate. Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
b40690e to
da9506c
Compare
|
I pushed a different proposal (loosely based on a suggestion of @mrobinson ) baking in resources on most platforms by default (but optional, as feature flag. To make it convenient to override, this uses the |
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
mrobinson
left a comment
There was a problem hiding this comment.
Looks pretty good to me! I have a few comments and one request: Let's move default-resources out of shared as that is meant for the *-traits and *-api crates. It will likely be eliminated sometime in the future.
| use servo_base::generic_channel as base_channel; | ||
| use servo_base::generic_channel::GenericSend; | ||
| use servo_base::id::TEST_WEBVIEW_ID; | ||
| use servo_default_resources as _; |
There was a problem hiding this comment.
Is this necessary just so that this doesn't show up in tools like machete?
There was a problem hiding this comment.
It's necessary to make sure rust links the crate (and hence the resource reader is registered). I think extern crate syntax would also work. I'll check machete out and see if it warns about this. I see there is a way to ignore false positives, so probably we can do something. (I'm wondering if machete is being used by rust-rover under the hood)/
There was a problem hiding this comment.
machete doesn't complain about these usages. But we still need this, otherwise the default resources crate will not be linked (declaring the dependency in Cargo.toml is not enough, which was a bit surprising to me, but I guess makes sense).
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
broken by "Move default-resource out of shared" Signed-off-by: Jonathan Schwender <schwenderjonathan@gmail.com>
|
@mrobinson I believe I have addressed all comments. Let me know if this PR is good to go. |
mrobinson
left a comment
There was a problem hiding this comment.
In many ways this seems simpler than what we had before which is really nice.
servo-default-resources crate
This PR considers the following constraints:
../../../resources/<file>file references).nextestspawns each test in its own process, so we don't want to explicitly initialize the resource handler for every#[test]fn)(File) Resources that are only accessed from servoshell are out of scope of this PR, since it mainly focusses on unblocking publishing
libservoto crates.io.Baking the resources into the binary by default simplifies the setup a lot. We already supported that before, but only for testing purposes and explicitly not for production builds.
Using
inventoryadds a simple way for the embedder to replace the default baked in resources, while also keeping the test usage of baked in resources simple.rippy.png is also referenced from image_cache - We simply duplicate it, since the image is small, to avoid adding unnecessarily complex solutions like adding a dedicated crate.
Testing: Covered by existing tests. mach try full
Fixes: Part of #43145