1563689 - Manage TabObserver from Targets

Status: RESOLVED FIXED

(Remote Protocol :: Agent, enhancement)

Description

[Alexandre Poirot [:ochameau]]

For now, we were observing the tabs opening/closing from RemoteAgent, and making the glue between TabObserver and Targets from RemoteAgent class.
But this is a bit misleading as the MainProcessTarget is managed by Targets and someday, we will have some other target kinds, like workers.
I believe it will be easier to follow if Targets was the component responsible to hold the list of all the targets of any kind, and maintain it by all means. That, without depending on some external glue code.

Also the connect and disconnect event being emitted by Targets are misleading. It would be great to rename them to no longer use the connection meaning as no connection is being made or shut down during these events.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1563689 - Revamp how targets are watched and reported.

• TabObserver is rather an helper class of Targets rather than RemoteAgent.
  Targets is the class which holds all the targets and reports about their
  creation and destructor. It feels legitimate to have it directly integrate
  with TabObserver.
• To better sort of the files. i.e. avoid having "random files" in /remote/
  I'm renaming and moving TabObserver according to its usage.
• We were emitting "connect" and "disconnect" event when a target was created
  or destroyed. But this is misleading as there is no connection to anything
  being made. Only later, a CDP client might connect to a target HTTP endpoint
  and initiate a connection. These events are making this hard to understand
  that the connection actually happens when Target.handle is called.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1563689 - Ensure removing listeners set by TabObserver.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1563689 - Release DOM event listeners set on top level windows.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b351d73797d
Revamp how targets are watched and reported. r=remote-protocol-reviewers,ato,jdescottes
https://hg.mozilla.org/integration/autoland/rev/e3611aa630dc
Ensure removing listeners set by TabObserver. r=remote-protocol-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/6697f16a02ef
Release DOM event listeners set on top level windows. r=remote-protocol-reviewers,jdescottes

Comment 5

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1b351d73797d
https://hg.mozilla.org/mozilla-central/rev/e3611aa630dc
https://hg.mozilla.org/mozilla-central/rev/6697f16a02ef