1610855 - Implement Target.targetInfoChanged

Status: RESOLVED WONTFIX

(Remote Protocol :: CDP, enhancement, P3)

Description

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

The Puppeteer unit test for this expects a Target.targetInfoChanged CDP event after Page.close in Puppeteer.

https://github.com/puppeteer/puppeteer/blob/14b236965000c2821aece8deae72fb927ab33930/test/launcher.spec.js#L382-L393

Puppeteer emits its own Events.Browser.TargetChanged when it sees Target.targetInfoChanged

Firefox Browser target events should work (launcher.spec.js:382:5)
  Message:
    expect.toEqual failed:
{
  "0": "CREATED",
  "1": "DESTROYED"
} ≈ {
  "0": "CREATED",
  "1": "CHANGED",
  "2": "DESTROYED"
}
  Stack:
    Error: expect.toEqual failed:
    {
      "0": "CREATED",
      "1": "DESTROYED"
    } ≈ {
      "0": "CREATED",
      "1": "CHANGED",
      "2": "DESTROYED"

Comment 1

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

This event doesn't need to be emitted for Page.close only, but for each instance a target changes. Note that we don't have implemented this event at all. Also there is even no meta bug filed yet. Could you do that?

Comment 2

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

It's actually for targetInfoChanged, for which we have a meta bug. (I updated my original comment as well to reflect this.)

Comment 3

[Oliver Medhurst [:canadahonk]]

As the spec does not specify, from looking at Chromium source, Target.targetInfoChanged is emitted when a target is:

• Navigated
• Attached
• Detached (not implemented)

As detached is not implemented, it might be good for this (or the meta) to depend on detach as otherwise Page.close, etc would not emit a Target.targetInfoChanged event.

Comment 4

[Oliver Medhurst [:canadahonk]]

Attachment: Bug 1610855 - [cdp] Implement Target.targetInfoChanged

Added new "target-changed" event to TargetList generally, which Target
domain now subscribes to with Target.setDiscoverTargets, and emits a
Target.targetInfoChanged event when recieved.

Target.targetInfoChanged is emitted when certain things happen to a target:

• Top frame navigation (via observing document-element-inserted in TargetList)
• Attached (via attachToTarget in Target domain)
• Detached (not implemented - see Bug 1549536)

These are not specified in the spec but you can find by searching Chromium source:
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/devtools/protocol/target_handler.cc?q=targetInfoChanged

Puppeteer tests look to depend on this event quite a bit as they
always expect it in event histories, hopefully should fix several
results (TODO: retest and check changes).

WIP, needs tests.

Comment 5

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

(In reply to CanadaHonk [:CanadaHonk] from comment #3)

As detached is not implemented, it might be good for this (or the meta) to depend on detach as otherwise Page.close, etc would not emit a Target.targetInfoChanged event.

I've added a comment to bug 1636979 to indicate that this event needs to be fired as well.

Comment 6

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

Oliver, as it looks like we forgot to continue on this PR. I assume you haven't had the time to check why the test is failing all the time? If there is no time on your side to finish the patch we might consider abandon it. What do you think? Thanks!

Comment 7

[Oliver Medhurst [:canadahonk]]

I could if it is wanted, but since CDP efforts are winding down it probably isn't worth either of our times.

Comment 8

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

Sure, but it's work you did and which was close to landing. So if only a single piece is needed it might still be worthwhile.

I've quickly rebased the patch locally and the test works fine for me. Lets see if try is happy as well these days and if some other patch from the last months actually fixed the problem.

Here a try build:
https://treeherder.mozilla.org/jobs?repo=try&revision=93a5aa3b4e56c402076c34109d08b0d6877bb854

Comment 9

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

Tests are still failing and this is most likely related to sending out the event too early in case of a navigation still not done. Finding and fixing the problem might take a while, so I would say lets not spend more time on it and just close it as wontfix.

Thank you Oliver anyway for trying to get this API added.