891679 - Only include type fields in notification packets, not reply packets

Status: RESOLVED WONTFIX

(DevTools :: Debugger, defect, P3)

Description

[Heather Arthur [:harth]]

Though it's not specified by the protocol, most times a packet from the server with a "type" is an event. It's a lot easier if whatever's handling requests and replies from an actor can rely on "type" signifying an event, to differentiate it from a reply to the previous request.

Right now I have to special case for the tab actor in my client. It seems like there's no need for the "type" in the attach() and detach() responses, but correct me if I'm wrong.

Comment 1

[Heather Arthur [:harth]]

Panos, you authored this and you probably have an opinion. Is there any reason not to remove the 'type's?

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

They are explicitly mentioned in the protocol spec, both for the tab and thread actors. I believe the rationale was to give the client more insight into the actor's state:

https://wiki.mozilla.org/Remote_Debugging_Protocol

It seems to me that we could easily remove "type":"detached" and "type":"tabAttached" and rely on the implicit convention that responses indicating success do not carry extraneous information.

I also think we could probably substitute an "error":"wrongState" response for "type":"exited", since we don't really use it in the front-end. Its purpose is I believe to inform the client of a rare situation in which a tab was exited between "listTabs" and "attach" (or a thread was exited between attaching to the tab and attaching to the thread), but I can't think of a meaningful difference in the client's treatment of an "exited" response versus a "wrongState" one.

Jim, as the spec author, may know of some reasons to keep the "type" property in those response packets though, so I'll pass the buck to him.

Comment 3

[Heather Arthur [:harth]]

Jim, any word? Could we just use 'status' instead of 'type'?

Comment 4

[Jim Blandy :jimb]

Our client doesn't keep as much information about the state of each actor it's talking to on the server side as I'd expected. I'd imagined that we'd have a distinct 'front' object on the client side for each actor we had decided to talk to on the server side. I expected the front object would be a natural place to track the state of each actor, and its methods could be expected to know which kinds of packets were what. But that's not how it went; and perhaps there's less code to take care of this way.

Sure, we could have packets that aren't replies to a specific request have a property "notification":<kind> where <kind> is a string identifying the particular kind of notification. That'd be kind of a big change, and a breaking one; what would it take to make the client cope with older servers, while changing our server?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

I think backwards compatibility could be preserved by adding a FeatureCompatibilityShim to the client that adds a "notification" property to any packet whose type is in UnsolicitedNotifications or UnsolicitedPauses. We can manipulate packets at will before handing them over to DC_onPacket.

Comment 6

[Eddy Bruel [:ejpbruel]]

Instead of doing this for just attach/detach, I think it would make sense to just adopt an convention such as:

1. All packets sent from the server to the client have the form { from: <actor>, ... }
2. If and only if a packet is a notification, it contains a type field with the name of the notification.

Or something similar to that. I realize that doing it exactly like above would be a breaking change that might require us to change too many things at once, but I would like to avoid a scenario where we have different protocol conventions for different actors.

Comment 7

[Jason Laster [:jlast]]

I'm closing as this is old and likely not needed. Feel free to comment if there is something relevant.