1607560 - Implement DOM.describeNode

Status: RESOLVED FIXED

(Remote Protocol :: CDP, task, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Another API which is used a lot at least in Puppeteer unit tests, and is causing a lot of those to fail.

Puppeteer itself only uses the objectId for referencing the node. Every other optional argument is not in use. The result is a node description, whereby only its backendNodeId and frameId properties are used within Puppeteer.

Comment 1

[Andreas Tolfsen ❲:ato❳]

This is according to whimboo one of the currently highest ranking failures in Puppeteer unit tests.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I'm struggling in what backendNodeId actually should be. It is very hard to find something in Chromium's code about that.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So we actually have to implement an element store similar to the one from Marionette. With that we have our own mirror of DOM nodes as present in the various targets. Once an element is requested it needs to be added to that store, and at the same time it will get a unique id. Given that we don't have to diverge between the front-end and backend it looks like that nodeId and backgroundNodeId can be the same.

As temporary solution (which even makes some Puppeteer tests pass) I simply return the node's data. And by that I got nearly all of the available properties filled with data:

puppeteer:protocol SEND ► {"sessionId":1,"method":"DOM.describeNode","params":{"objectId":"0b2954a7-6584-7a49-88f6-08d4e221540d"},"id":19} +1ms
puppeteer:protocol ◀ RECV {"sessionId":1,"id":19,"result":{"node":{"nodeType":1,"nodeName":"IFRAME","localName":"iframe","nodeValue":"","childNodeCount":0,"attributes":["id","foo","src","<div>a</div>"],"frameId":"11"}}} +2ms

Some still have a wrong value (nodeId, childNodeCount), which needs to be fixed. But most of the work is still the implementation of the element store.

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Does it make sense to land your temporary solution now and follow up with the element store implementation later? Or rather do all of it in this bug?

Looking at Puppeteer source, the minimum needed the objectId parameter and the backendNodeId property on the return value (which can be the same as the nodeId, as you say.)

I expect the next method we'll need is DOM.resolveNode.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The backendNodeId is a custom id we internally have to set. We cannot return a random number without being able to store the correlation between this id and the element reference. Otherwise different ids would be returned each time the method gets called.

So we clearly need the element store, which indeed will be worked on all on this bug.

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The way gutenberg depends on this is through Puppeteer's Page.waitForSelector -- the call ends up in https://github.com/puppeteer/puppeteer/blob/14b236965000c2821aece8deae72fb927ab33930/lib/ExecutionContext.js#L189-L201

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f7473e06b1e76619b3ad0f7fa0854acbfaead33

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The last try build had some problems that are fixed now. Lets see how many Puppeteer tests unexpectedly start failing or passing now. Locally I can see a couple more passing but I assume it's the same race, which I saw on bug 1612174.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e58f4a087cabbc5cd659a33623252a80c658631

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1607560 - [remote] Basic implementation for DOM.describeNode.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31d3fb769ac3
[remote] Basic implementation for DOM.describeNode. r=remote-protocol-reviewers,maja_zf

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/31d3fb769ac3