Skip to content

Conversation

@krichprollsch
Copy link
Member

No description provided.

@krichprollsch krichprollsch self-assigned this Apr 30, 2025
const parent_node = try bc.node_registry.register(p);
const node = bc.node_registry.lookup_by_node.get(n) orelse unreachable;
// Should-we return one DOM.setChildNodes event per parentId
// containing all its children in the nodes array?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

If it's something we'll need to do often, we could expand the NodeWriter. It already takes a placeholder option, so it could be something like:

.nodes = bc.nodeWriter(parent_node, .{.mode = .child_list})},

@krichprollsch krichprollsch marked this pull request as ready for review May 1, 2025 17:44
@krichprollsch
Copy link
Member Author

We have to dispatch the setChildNodes event for the whole hierarchy, starting from the root until the nodes.
I tried an implementation but I have a leak error with the unit tests.

@karlseguin
Copy link
Collaborator

We have to dispatch the setChildNodes event for the whole hierarchy, starting from the root until the nodes. I tried an implementation but I have a leak error with the unit tests.

Really? I was pretty sure it was just the node's sibling, or, maybe more accurately, the node's parent children.

I don't see a leak, both in the CI and locally, it's an unreachable which is being hit.

const parent_node = bc.node_registry.lookup_by_node.get(p) orelse unreachable;

I don't think there's any reason to assume the parent has been seen by the registry before. Always better to use register, which does a getOrPut

const node = try bc.node_registry.register(parser.documentToNode(doc));

After that, you'll get a crash because the session_id isn't set. I would do 2 things here:

1 -
Be defensive and add a check after loading `bc.

var bc = ...
const session_id = bc.session_id orelse return error.SessionIdNotLoaded;

2 - Change the cdp/testing.zig loadBrowserContext and set a default session_id if there's html:

// existing if statement
if (opts.html) |html| {
   // add
   if (bc.session_id == null) bc.session_id = "SID-X";
   ..
}

@krichprollsch
Copy link
Member Author

The issue is fixed. I added a bool into cdp.Node to know if a node has been already dispatched.

Really? I was pretty sure it was just the node's sibling, or, maybe more accurately, the node's parent children.

Yes it is as far as I can see. ..

Co-authored-by: Karl Seguin <karlseguin@users.noreply.github.com>
@krichprollsch krichprollsch merged commit 8eadccd into main May 5, 2025
12 checks passed
@krichprollsch krichprollsch deleted the dom-setchildnodes branch May 5, 2025 06:56
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants