Support Data URI in scripts tags #596
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
ksdme
force-pushed
the
ksdme/data-uri-script
branch
3 times, most recently
from
4e1c05a to
daa51eb
Compare
karlseguin
left a comment
karlseguin
left a comment
There was a problem hiding this comment.
Thanks for the PR. It looks great. A few inline comments, but just a few very minor things.
src/browser/browser.zig
Outdated
| // It resolves src using the page's uri. | ||
| // If a base path is given, src is resolved according to the base first. | ||
| // the caller owns the returned string | ||
| // TODO: We are leaking the memory associated with the response. |
There was a problem hiding this comment.
We leverage arena allocators a lot, hence the name of the variable is arena. It's cleared whenever a page is created in createPage. So it isn't leaking, it's just [probably] living for longer than it absolutely has to.
src/browser/browser.zig
Outdated
| const arena = self.arena; | ||
|
|
||
| // Handle data URIs. | ||
| const data_uri: ?DataURI = DataURI.parse(arena, src) catch null; |
There was a problem hiding this comment.
I would make parse return an optional directly, !?[]const u8.
To me, the main benefit is that it gives you an easy way to differentiate between it being an invalid data URI and some other error. Like, as-is, if parse throws an OutOfMemory error, the catch just tries to move on. Not the end of the world, but also not particularly explicit.
If you stick error.Invalid, then this code might be better as:
const data_uri: ?DataURI = DataURI.parse(arena, src) catch |err| switch (err) blk: {
error.Invalid => break :blk null,
else => return err,
};Which is a bit wordy, and why I'd prefer the optional:
if (try DataURI.parse(arena, src)) |data_uri| {
return data_uri.data;
}There was a problem hiding this comment.
This is how I had it earlier, I think I removed it because it felt like returning null prevents you from calling deinit and clean up and temporary allocations.
Fixed up though.
src/browser/datauri.zig
Outdated
| } | ||
|
|
||
| const uri = src[5..]; | ||
| const data_starts = std.mem.indexOf(u8, uri, ",") orelse return error.Invalid; |
There was a problem hiding this comment.
Can use std.mem.indexOfScalar directly. std.mem.indexOf will call it, but only after doing 2 length checks on the needle.
| }; | ||
| } | ||
|
|
||
| pub fn deinit(self: *const DataURI, allocator: Allocator) void { |
There was a problem hiding this comment.
Up to you whether you want to keep it or not (given what I commented on above about the use of an arena). It isnt going to be called, again, because of the arena, but, for completeness, I'm ok with keeping it in. Maybe one day we'll want to call this wit a different type of allocator, so having this is a good reminder that, yes, normally, we should deinit it.
There was a problem hiding this comment.
I think it makes sense to keep it. I like it because it sort of documents the allocations in an expected manner.
ksdme
force-pushed
the
ksdme/data-uri-script
branch
from
daa51eb to
ee97eaa
Compare
|
@karlseguin Fixed up everything. |
|
Great, again, thanks for the PR! It really helps. |
2 participants
Related to #412
Hey guys,
I was trying to play around with Zig and found this project via a tweet from Andreas Kling. I took a shot at the issue linked above (it looked simple and reproducible). I apologize for any stupidity in the changes, like I mentioned, I am new to Zig.
For some context on the TODOs,
I tried fixing up
Mime.parseto use a[]const u8instead so I could use it during data URI parsing. But that required either turning all return values into owned values or moving up the normalized string into the struct. Given that I could find how deinit was being called infetchDatacall stack, I just left that for later.And, I tested the changes with the following,