Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CSS import support #139

Merged
merged 12 commits into from
Nov 3, 2024
Merged

Conversation

kokoISnoTarget
Copy link
Contributor

@kokoISnoTarget kokoISnoTarget commented Sep 18, 2024

This adds support for CSS imports.
This also changes the net logic/structure.

@kokoISnoTarget
Copy link
Contributor Author

@nicoburns any thoughts about this? If you feel this is useless/unneeded, I would close it.

@nicoburns
Copy link
Collaborator

nicoburns commented Oct 31, 2024

@nicoburns any thoughts about this? If you feel this is useless/unneeded, I would close it.

@kokoISnoTarget Hello! Apologies for letting this sit for so long. I definitely want this feature, although I would not consider it super-high priority.

My main issue with this PR is the introduction of a static variable. I believe it ought to be possible to implement this without it (perhaps using an Arc?). And I would really like to avoid introducing one, as it makes composing modules together much more complex.

I also still think we will want to move the "net" crate to a simpler "url in, bytes out" model. And generally simplify the interface between modules (I don't think all the traits we have currently are necessary, but I haven't quite worked out how it would work without them yet). But that doesn't need to be done here.

I like how this PR moves the net stuff into a net module.

@kokoISnoTarget
Copy link
Contributor Author

I also still think we will want to move the "net" crate to a simpler "url in, bytes out" model.

I wouldn't know what would be responsible for the bytes (I guess the DOM or an implementation using the DOM), we'd also lose the capability to parse something like CSS on the "fetch" thread.

And generally simplify the interface between modules (I don't think all the traits we have currently are necessary, but I haven't quite worked out how it would work without them yet). But that doesn't need to be done here.

If we do the "URL in, bytes out" we could remove RequestHandler and the Callback traits and make the NetProvider a bit simpler.

@nicoburns
Copy link
Collaborator

Let's keep this out of scope of this PR, but:

I wouldn't know what would be responsible for the bytes (I guess the DOM or an implementation using the DOM), we'd also lose the capability to parse something like CSS on the "fetch" thread.

So I think the idea would be that in the short-term it would just be handled internally by blitz-dom (in the load_resource function I guess). Long-term, I'd like to have a separated blitz-media crate which ends up a "provider" to blitz-dom similar to blitz-net, and could have it's own threadpool. I think it's probably best to keep media decoding and networking threads separate due to their very different workload characteristics (IO-heavy vs CPU-heavy). Async networking shouldn't need too many threads at all!

If we do the "URL in, bytes out" we could remove RequestHandler and the Callback traits and make the NetProvider a bit simpler.

Exactly. I also think it would be possible to make blitz-net suitable for loading the initial HTML document. Which would be really nice as (unlike the current code in our examples) it knows how to load from either the network or the filesystem.

examples/assets/servo.html Show resolved Hide resolved
examples/assets/servo-color-negative-no-container.png Outdated Show resolved Hide resolved
Comment on lines 18 to 16
fn bytes(self: Box<Self>, bytes: Bytes, callback: SharedCallback<Self::Data>);
fn bytes(self: Box<Self>, bytes: Bytes);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this changed (callback is no longer passed in by net crate)? I feel like the Document/HtmlDocument/HtmlSink should never need to know about the callbacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed that because it make the API simpler and the callback is only needed when we want to put a resource into the DOM, that does not happen in cases like the @import rule.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this back please? It is definitely simpler if the dom crate never knows about the callbacks (well, for now, only temporarily in the "resource handlers", but once we make it "url in, bytes out" the dom won't need the callbacks at all).

Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very hard for me to determine exactly what has changed in the util/net code due to the file rename. Would perhaps have been better as a separate PR (making things easy to review will definitely get them reviewed faster...). But this new code looks ok to me :)

@nicoburns nicoburns merged commit 815a1aa into DioxusLabs:main Nov 3, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants