Add TreeWalker #664
Add TreeWalker #664
Conversation
karlseguin
left a comment
karlseguin
left a comment
There was a problem hiding this comment.
This looks ok, but it seems like the implementation is missing a lot, e.g. the filter and whatToShow fields aren't being used. That's fine, but I just wanted to make sure that was the intention.
src/browser/dom/document.zig
Outdated
| return Node.replaceChildren(parser.documentToNode(self), nodes); | ||
| } | ||
|
|
||
| pub fn _createTreeWalker(_: *parser.Document, root: *parser.Node, whatToShow: ?u32, filter: ?Env.Callback) TreeWalker { |
There was a problem hiding this comment.
| pub fn _createTreeWalker(_: *parser.Document, root: *parser.Node, whatToShow: ?u32, filter: ?Env.Callback) TreeWalker { | |
| pub fn _createTreeWalker(_: *parser.Document, root: *parser.Node, what_to_show: ?u32, filter: ?Env.Callback) TreeWalker { |
src/browser/dom/tree_walker.zig
Outdated
| whatToShow: u32, | ||
| filter: ?Env.Callback, | ||
|
|
||
| pub fn init(node: *parser.Node, whatToShow: ?u32, filter: ?Env.Callback) TreeWalker { |
There was a problem hiding this comment.
| pub fn init(node: *parser.Node, whatToShow: ?u32, filter: ?Env.Callback) TreeWalker { | |
| pub fn init(node: *parser.Node, what_to_show: ?u32, filter: ?Env.Callback) TreeWalker { |
src/browser/dom/tree_walker.zig
Outdated
| pub const TreeWalker = struct { | ||
| root: *parser.Node, | ||
| currentNode: *parser.Node, | ||
| whatToShow: u32, |
There was a problem hiding this comment.
| whatToShow: u32, | |
| what_to_show: u32, |
src/browser/dom/tree_walker.zig
Outdated
| // https://developer.mozilla.org/en-US/docs/Web/API/TreeWalker | ||
| pub const TreeWalker = struct { | ||
| root: *parser.Node, | ||
| currentNode: *parser.Node, |
There was a problem hiding this comment.
| currentNode: *parser.Node, | |
| current_node: *parser.Node, |
src/browser/dom/tree_walker.zig
Outdated
| return .{ | ||
| .root = node, | ||
| .currentNode = node, | ||
| .whatToShow = whatToShow orelse std.math.maxInt(u32), |
There was a problem hiding this comment.
Would improve readability to use NodeFilter._SHOW_ALL, or at least a comment saying the orelse is show all.
src/browser/dom/tree_walker.zig
Outdated
| } | ||
|
|
||
| pub fn _firstChild(self: *TreeWalker) ?*parser.Node { | ||
| const first_child = parser.nodeFirstChild(self.currentNode) catch return null; |
There was a problem hiding this comment.
Hmm dom_node_get_first_child increments the ref_count, walker.zig says we should be deiniting these Nodes.
Though I don't see us doing that in the rest of the code.
Worst case they would be cleaned up when the page is removed.
There was a problem hiding this comment.
Because everything is in an mimalloc arena. Wonder if we took out all the refcnt++ and refcnt-- from libdom if we'd shave a few µs.
There was a problem hiding this comment.
Yea, may want to remove to comments from walker.zig.
Or actually maybe this PR can replace the whole thing, maybe.
mookums
force-pushed
the
treewalker
branch
2 times, most recently
from
59a9cf8 to
8bbe69d
Compare
Because jsValueToStruct is now used in union probing, it shouldn't fail on a mismatch, but rather return null. It's up to the caller to decide whether that's an error or not.
src/browser/dom/tree_walker.zig
Outdated
| const child = (try parser.nodeListItem(children, index)) orelse return null; | ||
|
|
||
| if (!try self.verify_what_to_show(child)) continue; | ||
| if (!try self.verify_filter(child)) continue; |
There was a problem hiding this comment.
since you always call verify_filter and verify_what_to_show together, I wonder combining them, or having a helper that calls both is worthwhile.
There was a problem hiding this comment.
Yeah, I collapsed these into a single verify function.
src/browser/dom/tree_walker.zig
Outdated
| } | ||
|
|
||
| pub fn _previousNode(self: *TreeWalker) !?*parser.Node { | ||
| return self._parentNode(); |
There was a problem hiding this comment.
same as nextNode..is it really always the parent? It can't be a previous sibling?
There was a problem hiding this comment.
Fixed this too, moving in document order.
src/browser/dom/tree_walker.zig
Outdated
| if (!try self.verify_what_to_show(child)) continue; | ||
| if (!try self.verify_filter(child)) continue; | ||
|
|
||
| self.depth += 1; |
There was a problem hiding this comment.
does every node you visit necessarily increase the depth? I'm thinking if child is a sibling of current_node, it shouldn't increase the depth.
Maybe I don't understand..but if I'm right, it seems like depth is going to get tricky to keep track of. If depth is only used in _parentNode to protect against going outside the tree, maybe there's a better way (like comparing against the root node?)
There was a problem hiding this comment.
Ditched depth in favor of just comparing against the self.root when we are moving up the tree.
|
Just as a note, some newly passing WPT tests: |
|
LGTM |
This introduces the
TreeWalkerand relatedNodeFilter.