servoshell: Display favicons in tab bar #36680
Conversation
b394531 to
35bbadb
Compare
ports/servoshell/Cargo.toml
Outdated
| egui_extras = { version = "0.31", features = ["all_loaders"] } | ||
| egui_glow = { version = "0.31.1", features = ["winit"] } | ||
| egui-winit = { version = "0.31.1", default-features = false, features = ["clipboard", "wayland"] } | ||
| egui-winit = { version = "0.31.1", default-features = false, features = ["clipboard", "wayland"] } # |
There was a problem hiding this comment.
| egui-winit = { version = "0.31.1", default-features = false, features = ["clipboard", "wayland"] } # | |
| egui-winit = { version = "0.31.1", default-features = false, features = ["clipboard", "wayland"] } |
|
|
||
| if let Some(favicon_url) = webview.favicon_url() { | ||
| tab_frame.content_ui.add( | ||
| egui::Image::new(favicon_url.to_string()) |
There was a problem hiding this comment.
What does this do under the hood? Does egui have a networking stack? I'm ok with that in the short term, but we should make it possible for the engine to fetch the icon and the embedder to use the cached response body instead.
There was a problem hiding this comment.
Yeah, the plan is that eventually the WebView API should expose the favicon as a bitmap as it should be fetched using the cookies, etc of the context that it is loading in.
There was a problem hiding this comment.
Does egui have a networking stack?
Yes, with egui_extras. I also don't like this solution, but it's kinda the only possibility given the current embedding API
|
Unifying the dependencies is proving to be a bit tricky. |
This is still true for egui |
|
@simonwuelker emilk/egui#7406 doesn't make sense to me; the current cargo.toml just means that 3.3 is a minimum requirement, not an exact one. It's still possible to for servo to update a board to any 3.x version independently. |
|
...I keep forgetting that the version in Cargo.toml is a minimum. Thanks. |
|
There are only a couple of small duplicate dependencies left which I don't think pose an issue. However, the updated
The fuzzing dependency is behind a |
|
Yeah, that seems totally reasonable to me. |
|
I suspect the test failures come from a conflict between cryptography providers in egui vs. Servo's unit tests. |
|
Thats annoying, |
|
I think it will be easier to first update the embedding api such that it returns a bitmap for favicons. |
|
This PR now depends on #38949. |
…er (#38949) Currently the embedding API only provides the embedder with the URL for a favicon. This is not great, for multiple reasons: * Loading the icon should happen according to the fetch spec which is not easy for the embedder to recreate (consider CSP, timing information etc) * Rasterizing a svg favicon is not trivial With this change, servo fetches and rasterizes the icon to a bitmap which is then passed to the embedder. Testing: I'm not sure how I can write tests for the embedding api. I've tested the correctness manually using #36680. Prepares for #36680 --------- Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
|
Would anyone mind re-reviewing this change? This PR has changed significantly since it was last approved. |
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
Before:
After:
This PR moves the favicon, title and close button into a single egui Frame. Doing this allows us to get rid of some of the previous layout magic (like setting a border radius on the left corners of the label and the right corners of the button so they appear as one widget). It also ensures that the tab is highlighted when the close button (not the label) is hovered.