Web Inspector: Debugger: don't allow the domain to be disabled

• Component: Web Inspector
• Status: RESOLVED WONTFIX

[Devin Rousso]

.

[Yury Semikhatsky]

I wonder about the direction of removing enable/disable methods from protocol domains in general, what is the rationale for that? Does it mean that a client will now be receiving all events upon connection?

In the past we stuck to the principle of radio silence in the protocol by default, i.e. no events were sent over the wire until a domain was explicitly enabled. This was useful for low overhead instrumentation, e.g. when recording CPU profile it would make sense to disable things like debugger, DOM etc which could have non-trivial performance impact. What is the story for such use cases going forward?

[Devin Rousso]

Created attachment 376111 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 376112 [details]
Patch

[Devin Rousso]

Created attachment 376114 [details]
Patch

[Devin Rousso]

Created attachment 376119 [details]
Patch

[Devin Rousso]

Created attachment 376120 [details]
Patch

[EWS Watchlist]

Comment on attachment 376120 [details]
Patch

Attachment 376120 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12901137

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 376124 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 376120 [details]
Patch

Attachment 376120 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12901160

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 376128 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Joseph Pecoraro]

(In reply to Yury Semikhatsky from comment #1)
> I wonder about the direction of removing enable/disable methods from
> protocol domains in general, what is the rationale for that? Does it mean
> that a client will now be receiving all events upon connection?
> 
> In the past we stuck to the principle of radio silence in the protocol by
> default, i.e. no events were sent over the wire until a domain was
> explicitly enabled. This was useful for low overhead instrumentation, e.g.
> when recording CPU profile it would make sense to disable things like
> debugger, DOM etc which could have non-trivial performance impact. What is
> the story for such use cases going forward?

That is my understanding as well. I know that Devin wanted to remove some of these for Agents that we expect to always be "required", otherwise a bunch of other domains won't work.

Devin, what is the end goal here? Just cut down on a few back/forth messages? This seems to be rather low priority / low advantage. Particularly for things like Debugger, which now forces clients to hear about every script ever evaluated without a choice.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #12)
> (In reply to Yury Semikhatsky from comment #1)
> > I wonder about the direction of removing enable/disable methods from protocol domains in general, what is the rationale for that? Does it mean that a client will now be receiving all events upon connection?
> > 
> > In the past we stuck to the principle of radio silence in the protocol by default, i.e. no events were sent over the wire until a domain was explicitly enabled. This was useful for low overhead instrumentation, e.g. when recording CPU profile it would make sense to disable things like debugger, DOM etc which could have non-trivial performance impact. What is the story for such use cases going forward?
> 
> That is my understanding as well. I know that Devin wanted to remove some of these for Agents that we expect to always be "required", otherwise a bunch of other domains won't work.
That's one part of it, yes. Many types for other domains use `Network.FrameId`, which is ultimately created/controlled by `Page`, so all of those domains effectively require that the `PageAgent` be created/enabled in order for those bits to function.

> Devin, what is the end goal here? Just cut down on a few back/forth messages? This seems to be rather low priority / low advantage. Particularly for things like Debugger, which now forces clients to hear about every script ever evaluated without a choice.
Part of it is removing an extra message (and the latency that comes with it), as Web Inspector immediately `enable`s most "non-contained" agents (e.g. those that aren't limited to a specific Tab, and can find their associated data/commands in multiple places in the UI).

Another part of it is that no client ever seems to call `disable` on the majority of these agents.  In fact, AFAIK, the only times we call `disable` are the changes I've made when certain tabs are closed (e.g. Canvas, Storage, Timelines).

It seems like we're preserving these based on a possibility that some client will want this, and yet we haven't seen any evidence of this (other than those examples) in +10 years.

Given how pervasive some of these agents are throughout Web Inspector's UI, it makes no sense to me to even allow the idea for them to be enabled/disabled, as they're almost "expected" to exist for so much of the UI.  Even if the Elements tab is closed, we can still see DOM nodes in the Search tab and when logging nodes in the Console Tab (or split view), so we can't have a world where we don't have DOM node information (unless we're willing to allow the Console/Search to not show a node representation/tree if the Elements tab is closed), meaning that the `DOMAgent` always has to exist and send the frontend updates. Given that, it seems "wrong" to `enable` something that we're expecting to always exist.

Part of this may be putting data/events/commands in the "wrong" domain (e.g. I'd personally have put `Debugger.scriptParsed` in the `Runtime` domain, as that really has nothing to do with the debugger).  Fixing a few of those cases would actually let us `enable`/`disable` more domains when their associated tab(s) are closed.

[Devin Rousso]

(In reply to Devin Rousso from comment #13)
(I accidentally hit submit before I was done T.T)

I'd say the end goal is to do one of the following:
 1. remove `enable`/`disable` for all "non-contained" domains or those that are expected to be alive always
 2. move some events from domain A to domain B so that A is more "contained" and can be realistically `disable`d when the corresponding tab is closed
 3. create some sort of dependency system such that agent A is enabled by the backend (the frontend would be notified of this, probably via an `enabled` command) when a different agent B needs something from it

I'm most in favor of options 1 and 2, but even then I still think that would only really have any benefit for a few agents.

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #14)
> (In reply to Devin Rousso from comment #13)
> (I accidentally hit submit before I was done T.T)
> 
> I'd say the end goal is to do one of the following:
>  1. remove `enable`/`disable` for all "non-contained" domains or those that
> are expected to be alive always
>  2. move some events from domain A to domain B so that A is more "contained"
> and can be realistically `disable`d when the corresponding tab is closed
>  3. create some sort of dependency system such that agent A is enabled by
> the backend (the frontend would be notified of this, probably via an
> `enabled` command) when a different agent B needs something from it
> 
> I'm most in favor of options 1 and 2, but even then I still think that would
> only really have any benefit for a few agents.

Let's discuss this in person. The Protocol may benefit from a dependency system / restructuring but I don't think that wins us much.

  - as is we currently align with other developer tools protocols that have the same issues, and changing this will make us different and perhaps not in a good way
  - there are no obvious user benefits from removing the enable/disable other than potentially getting events a tiny bit earlier (unmeasured). Making use of `enable` / `disable` in new places (as you did for Timelines) sounds like the right idea, but removing the ability to disable / unsubscribe from events is probably the opposite of what we want.

[Devin Rousso]

See <https://webkit.org/b/200854> for explanation.