-
Notifications
You must be signed in to change notification settings - Fork 288
browser: update urls after redirection #504
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
Conversation
krichprollsch
commented
Apr 3, 2025
ede9058 to
4302be5
Compare
src/browser/browser.zig
Outdated
|
|
||
| // update uri after eventual redirection | ||
| var buf: std.ArrayListUnmanaged(u8) = .{}; | ||
| defer buf.deinit(arena); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can treat the allocator like an ArenaAllocator or like an std.mem.Allocator.
If we treat it like an Arena, we can remove unnecessary frees, which has a super-tiny performance cost.
If we treat it like an Allocator, we gain some flexibility with respect to changing the implementation.
It isn't a system-wide decisions, there are places where we'll have an arena and want to treat it like a generic allocator, and there are places where we'll definitely want to take advantage of the arena's behavior. The reason I like to call these ArenaAllocators arena is that it's clear to the writer & reader what the behavior is.
TL;DR - The deinit is almost certainly a no-op and could be removed. It's up to you.
src/browser/browser.zig
Outdated
| var buf: std.ArrayListUnmanaged(u8) = .{}; | ||
| defer buf.deinit(arena); | ||
|
|
||
| buf.clearRetainingCapacity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does nothing
src/browser/browser.zig
Outdated
| .query = true, | ||
| .fragment = true, | ||
| }, buf.writer(arena)); | ||
| self.rawuri = try buf.toOwnedSlice(arena); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you re-use the buffer later, which I guess is why you copy the contents. But, toOwnedSlice frees the underlying memory, so the underlying slice won't get re-used. I think you can use buf.items to avoid the allocation + copy.
src/browser/browser.zig
Outdated
| try self.session.window.replaceLocation(&self.location); | ||
|
|
||
| // prepare origin value. | ||
| buf.clearRetainingCapacity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As-above, if you stick with the toOwnedSlice(arena), then this code does nothing. toOwnedSlice(arena) calls clearAndFree().
If, above, you decide to do instead:
self.rawuri = buf.items
Then here you'll want to reset the buffer:
buf = .{};
as the clearRetainingCapacity() will break self.rawuri
src/browser/browser.zig
Outdated
| .scheme = true, | ||
| .authority = true, | ||
| }, buf.writer(arena)); | ||
| self.origin = try buf.toOwnedSlice(arena); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the option to use buf.items here.
|
@karlseguin thanks! I fixed the buffer usage. |