-
Notifications
You must be signed in to change notification settings - Fork 311
Backport Navigation (and friends) #1253
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
base: zigdom
Are you sure you want to change the base?
Conversation
09ae1c7 to
b3e2c1b
Compare
| } | ||
|
|
||
| pub fn onRemovePage(self: *Navigation) void { | ||
| self._proto = undefined; |
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.
Wasn't expecting that...Is there a reason for having these onRemovePage and onNewPage at all? Can the Navigation instance not be created once (in the Session)...nothing here references the *Page or anything tied to the page lifetime (e.g. window) as far as I can see.
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.
The Navigation has an EventTarget so with libdom, we had to create a new EventTarget whenever a new Page was created (or we'd get a segfault). With zigdom (from my understanding), since we create a new Factory whenever we get a new page, we need to create a new EventTarget for it whenever we create a new page.
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.
It can be created outside the factory, or the factory methods could take an opts that allows an explicit allocator to be passed in (though I don't love that approach, since it's an if check on every create).
135e861 to
1ca3fa0
Compare
1ca3fa0 to
a2b2559
Compare
a2b2559 to
02a0727
Compare
|
This looks good. Up to you if you think it's worth moving the Navigation instance to the session-owned arena so that we don't to reset between page navigation events - but I do think it would be cleaner. I'm trying to think if there are any implications to doing it (since nothing else does)...can't think of any. |
This backports
Navigationmostly and all of the related changes that I made across various PRs.This backports a number of PRs (including various with zigdom_backport labels)
HistoryWebAPI. #1086NavigationWebAPI #1138set_hashto Location #1200onload#1206This also brings over the URL stitch (now resolve) fix and the regression test that was solved by #1113.