MessageHandler currently supports two types of events:

• internal events
• protocol events

The behave similarly for the most part. For instance they both bubble up to the root message handler. But there are differences:

• internal events are both emitted under the name "message-handler-event" and under their own names
• protocol events are sent to the consumer

But this design lacks flexibility and prevents from easily reusing protocol events from other modules. Basically, as soon as we subscribe to a protocol event, it will be sent to the consumer. It doesn't matter if the subscription comes from an actual call to session.subscribe or if it's done internally, if the protocol event is emitted it will reach the consumer.

In practice this means you need to work around this, and if you need to reuse an existing protocol event for some internal logic, you will end up creating an intermediate internal event. This seems like unnecessary complexity which doesn't really bring much added value.

I propose to stop differentiating between protocol and internal events, we would just have events, period. But by default those events do not bubble to the consumer. Instead, the root session module would be responsible for sending the monitored events to the consumer.

Overall I think this makes sense, because the session module should already have complete knowledge of which events are monitored for which contexts, so it can very easily listen to all events from the RootMessageHandler and simply filter out the ones which are not currently monitored by the consumer.

I faced this issue while prototyping on HAR, and even if there are workarounds with the current design, I think we will keep facing this issue and we should address it.

To be discussed again next week.

(In reply to Julian Descottes [:jdescottes] from comment #0)

I propose to stop differentiating between protocol and internal events, we would just have events, period. But by default those events do not bubble to the consumer. Instead, the root session module would be responsible for sending the monitored events to the consumer.

Overall I think this makes sense, because the session module should already have complete knowledge of which events are monitored for which contexts, so it can very easily listen to all events from the RootMessageHandler and simply filter out the ones which are not currently monitored by the consumer.

This sounds fine to me and would match to what I proposed very early in the BiDi project. In such a case we have to create internal event structures which contain all the information that are needed by various consumers - and not only for external clients. It would make the events even more generic and re-usable between different protocols.

Further it would allow protocol commands to actually use events for their internal logic even those events haven't been subscribed to. As an example I could give the navigation command, that has to wait for some events before it returns.

One thing I'm not in par with is the fact that the session module should handle everything. I think it's fine for filtering out events that haven't been subscribed globally or for a given browsing context, but to actually build the event payload the session module would still have to forward into the actual protocol module in the parent process. Only that one defines what the event actually has to contain and the session doesn't know about.

As discussed yesterday we most likely want to already implement this in M4 to not being blocked when M5 starts.

We no longer distinguish between internal and protocol events within the framework.
All events now have the same behavior:

• emitted both under their own name and under the name message-handler-event
• bubble up to the root message handler
• the session which owns the RootMessageHandler does not listen for those events

The emitProtocolEvent has been moved to the RootMessageHandler only and can be used to emit events under a specific name "message-handler-protocol-event".
Only those events will be sent over to the actual client.

Internally the session module now keeps track of which events have already been subscribed to, to avoid forwarding duplicated events.
In a followup this will be centralized over in the EventsDispatcher, which can now play a bigger role.

Note that in the same way as for commands, there is still an implicit notion of "internal event", as in "an event which can not be subscribed to by consumers".
For commands this is done by having a "_" before the command name.
For events this is done by listing or not the event in the "supportedEvents" static array of the Module.

Depends on D155025
The current version returns the return value of the command (which can be a broadcast, depending on the provided descriptor).
This makes it impossible to know what to consistently expect as a return value.

It seems it was only used for a test assertion, so rewrite the test and cleanup the return value

Depends on D155059

In order for all events to really use similar codepaths, they should not use different subscription mechanism.

Setting an event in SessionData should be the entry point for all modules to decide to update their listeners or not.
Therefore we will also call _applySessionData on root modules which support this.

Next patch will start using this for the browsingContext.contextCreated event.

Depends on D155060

The _subscribe/_unsubscribeEvent can be removed in the next patch

Depends on D155061

All events now only react to SessionData and the session module can directly add/remove session data instead of using internal commands.
In the next patch, it will start relying on EventsDispatcher directly.

Depends on D155062

Some additional corrections had to be made on the EventsDispatcher module at the same time.

Comment on attachment 9290611 [details]
Bug 1783177 - [messagehandler] RootMessageHandler _applySessionData should return the session data update

Revision D155059 was moved to bug 1786255. Setting attachment 9290611 [details] to obsolete.

Comment on attachment 9290614 [details]
Bug 1783177 - [messagehandler] Apply session data to Root layer modules

Revision D155060 was moved to bug 1786255. Setting attachment 9290614 [details] to obsolete.

Comment on attachment 9290618 [details]
Bug 1783177 - [messagehandler] Use session data to subscribe to browsingContext.contextCreated

Revision D155061 was moved to bug 1786255. Setting attachment 9290618 [details] to obsolete.

Comment on attachment 9290619 [details]
Bug 1783177 - [messagehandler] Remove _subscribe/_unsubscribeEvent commands in favor of SessionData

Revision D155062 was moved to bug 1762334. Setting attachment 9290619 [details] to obsolete.

Comment on attachment 9290643 [details]
Bug 1783177 - [messagehandler] Use EventsDispatcher in session module

Revision D155070 was moved to bug 1762334. Setting attachment 9290643 [details] to obsolete.

https://hg.mozilla.org/mozilla-central/rev/4ef810265c8f