1612528 - Rename DebuggerServer, DebuggerServerConnection, DebuggerClient to DevToolsServer, DevToolsServerConnection, DevToolsClient

Status: ASSIGNED

(DevTools :: Framework, task, P3)

Description

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Based on today's discussion during the server architecture presentation, it looks like there is a agreement on renaming these classes.

Comment 1

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Attachment: Bug 1612528 - Rename DebuggerServer to DevToolsServer.

$ sed -ie "s/DebuggerServer/DevToolsServer/g" $(git grep -l DebuggerServer)

Comment 2

[Alexandre Poirot [:ochameau] PTO, back May 4th]

Attachment: Bug 1612528 - part2.

$ sed -ie "s//debugger-server"//devtools-server"/g" $(git grep -l /debugger-server)

Comment 3

[Patrick Brosset <:pbro>]

Maybe this should be another bug, maybe not, but the other thing that came out of the discussion today is that DevToolsServer, being the name of the singleton in charge of DevToolsServerConnections which really are the ones communicating with the clients felt odd.
If DevToolsClients are paired with DevToolsServerConnections, and DevToolsServer are just the global things that manage them, I would be tempted to do this:

• Rename DevToolsServer to DevToolsServerManager (or equivalent)
• Rename DevToolsServerConnection to DevToolsServer.

But, perhaps I'm missing a few nuances.

Comment 4

[Alexandre Poirot [:ochameau] PTO, back May 4th]

A try run, it looks like the rename is quite straightforward:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd52c170e946bd0c5d07ee3f4ed0f91eb2be4fef

Regarding the name, I don't think DebuggerServerConnection can be considered to be a "server".
A server typically connects to multiple incoming client connections. And DebuggerServerConnection is the class managing one "connection" with one client.
The Socket classes may actually be considered as servers. These are the ones receiving all client incoming connections and indirectly spawn the DebuggerServerConnection.

I think that the confusion isn't so much about DebuggerServerConnection, but rather with DebuggerServer.
And it isn't quite about its name (outside of replacing Debugger with DevTools), but rather about the fact that it is a singleton.

It surely is confusing because we spawn many instances of it. I think the real issue is here.
If only, we were spawning one DebuggerServer per process, it might be OK as-is.
But in some edge cases, we do spawn two servers in the same process. We do that by spawning two loaders: the common one and a custom one using insibileToDebugger/freshCompartment.
There may be ways to make this class instanciated, but it sounds like another quest.

Comment 5

[Julian Descottes [:jdescottes]]

I think the Server vs ServerConnection topic requires a bit more discussion indeed.

Initially, the name DebuggerServerConnection made me think it was purely about "connecting" things.
It felt lower level than debugger-server and less interesting to look at. But as it turns out, this is where the actors for a given "client" live, so it's quite important.

What I'm trying to say, is that I wish DebuggerServerConnection had a more compelling name, maybe I wouldn't have ignored it for so long :)
But naming is hard, and documentation can fill the gap. Maybe here that's the best option. And thinking more about it, I think I agree about the fact that DebuggerServerConnection is not a server.

Having a real singleton rather than a "singleton per loader" for the DevToolsServer would be nice. But here again, having a few examples that explain how many servers we end up creating would be enough.

(filed bug 1612681 to move my presentation as a documentation in tree)

Comment 6

[Phabricator Automation]

Comment on attachment 9123808 [details]
Bug 1612528 - Rename DebuggerServer to DevToolsServer.

Revision D61363 was moved to bug 1614791. Setting attachment 9123808 [details] to obsolete.

Comment 7

[Phabricator Automation]

Comment on attachment 9123809 [details]
Bug 1612528 - part2.

Revision D61364 was moved to bug 1614791. Setting attachment 9123809 [details] to obsolete.