add Navigation WebAPI
#1138
add Navigation WebAPI
#1138
Conversation
mookums
force-pushed
the
navigation
branch
4 times, most recently
from
620a5d3 to
fee8f5c
Compare
|
Duckduckgo integration test is failing on this branch, even after a rebase against |
| pub fn pushEntry(self: *Navigation, _url: ?[]const u8, state: ?[]const u8, page: *Page, dispatch: bool) !*NavigationHistoryEntry { | ||
| const arena = page.session.arena; | ||
|
|
||
| const url = if (_url) |u| try arena.dupe(u8, u) else null; |
There was a problem hiding this comment.
Does it make sense to accept a null url here?
I didn't see a place where pushEntry is called with a nullable url. Maybe we could remove that from the signature but also from the entry?
| if (is_same_document) { | ||
| page.url = new_url; | ||
|
|
||
| try committed.resolve({}); |
There was a problem hiding this comment.
Is it ok to return on error here w/o calling finished.resolve?
| }; | ||
|
|
||
| pub fn _traverseTo(self: *Navigation, key: []const u8, _: ?TraverseToOptions, page: *Page) !NavigationReturn { | ||
| // const opts = _opts orelse TraverseToOptions{}; |
There was a problem hiding this comment.
Consider to add a not implemented debug log
| typ: []const u8, | ||
| listener: EventHandler.Listener, | ||
| ) !?js.Function { | ||
| const target = @as(*parser.EventTarget, @ptrCast(self)); |
There was a problem hiding this comment.
Can you see if:
parser.toEventTarget(NavigationEventTarget, self),works? I think that's the safer version, but I know we don't use it everywhere we ought to.
|
|
||
| pub fn set_oncurrententrychange(self: *NavigationEventTarget, listener: ?EventHandler.Listener, page: *Page) !void { | ||
| if (self.oncurrententrychange_cbk) |cbk| try self.unregister("currententrychange", cbk.id); | ||
| if (listener) |listen| { |
There was a problem hiding this comment.
I think we make this mistake in other places. If listener is null, we never set oncurrententrychange_cbk back to null!
| .@"enum" => { | ||
| const T = @TypeOf(value); | ||
| if (@hasDecl(T, "toString")) { | ||
| // This should be deprecated in favor of the ENUM_JS_USE_TAG. |
There was a problem hiding this comment.
That could work, but the JS values often require escaping in Zig, e.g. largest-contentful-paint. Who likes typing @"largest-contentful-paint" ? On the flip side, these are pretty rare and they tend to be isolated.
3 participants
This is still pretty rough around the edges for now (and is incomplete)
This implements the
NavigationWebAPI.Historyis now a compatibility layer overNavigation, allowing them to share the same underlying entries.testinghas been changed, usingfetchWaitand loading a secondary testing html file that checks for the appropriate conditions when we need to load a secondary page (such as withHistoryorNavigation. thenavigateFromWebAPIhas also been changed, adding a field, that allows it so serve as the central way to navigate with proper hooks intoNavigation.There are a few Events that need to be completed so far, such as
NavigateEvent, and a lot of conditions that need extra testing (such as reloading).Looking to get this merged soon, just to prevent this from drifting off too far. There is still a good chunk of work left to be fully WPT compliant (such as firing events at the proper time), but a lot of these WPTs depend on other browser features (like
pageshow) that have to be implemented separately. I'm going to continue working on Navigation, specifically on getting most of the tests to pass.