Only create DebugModule where necessary.

[jellefoks]

Create a DebugModule only for the MainWebModule, so that there is no unnecessary waste observing messages which are then always discarded.

Add a command-line switch '--disable_web_debugger' that disables web debugger support completely, including the debug server.

b/287658692

[jellefoks]

Friendly ping?

[kaidokert]

Can you clarify what's the use case for this ? When would i use the added commandline flag ?

To be honest, i think we want to go in the opposite direction where most devtools are expected to be always available and included.

[jellefoks]

Can you clarify what's the use case for this ? When would i use the added commandline flag ?

To be honest, i think we want to go in the opposite direction where most devtools are expected to be always available and included.

Before this CL, the DebugModule is otherwise instantiated for the MainWebModule, the SplashScreen, and the DebugModule, while it's actually only functional for the MainWebModule. That means that it's currently instantiating idle and non-functional ScriptDebugger delegate objects, including for example the LogAgent that itself registers to intercept all logging output (and which then in turn does nothing with the log callbacks besides wasting CPU and memory resources).

This became apparent when implementing the functionality to allow reception of Cobalt console log in the devtools, specifically where agents_state (from the DebuggerState, owned by the BrowserModule, so there is a single shared instance for the entire app) would be modified from different threads, resulting in unexpected state and potentially crashes.

(in progress, need a little cleanup): eb247fb

[codecov]

Codecov Report

Merging #654 (a5fb789) into main (605a223) will decrease coverage by 0.37%.
Report is 1 commits behind head on main.
The diff coverage is 18.51%.

@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
- Coverage   56.88%   56.51%   -0.37%     
==========================================
  Files        1898     1898              
  Lines       94090    94096       +6     
==========================================
- Hits        53522    53181     -341     
- Misses      40568    40915     +347     

Files Changed	Coverage Δ
cobalt/browser/switches.cc	0.00% <0.00%> (ø)
cobalt/browser/web_module.h	100.00% <ø> (ø)
cobalt/renderer/backend/egl/graphics_system.cc	58.87% <ø> (-0.76%)	⬇️
cobalt/script/v8c/v8c_script_debugger.cc	0.00% <ø> (-41.75%)	⬇️
cobalt/script/v8c/v8c_script_debugger.h	0.00% <0.00%> (-66.67%)	⬇️
cobalt/browser/web_module.cc	38.68% <20.83%> (-0.75%)	⬇️

... and 26 files with indirect coverage changes

[kaidokert]

This became apparent when implementing the functionality to allow reception of Cobalt console log in the devtools, specifically where agents_state (from the DebuggerState, owned by the BrowserModule, so there is a single shared instance for the entire app) would be modified from different threads, resulting in unexpected state and potentially crashes.

Yep, okay - if it gets in the way and causes more instability in QA builds that are used for most testing, and doesn't work in other modules anyway, it should indeed be disabled.

But still - does a new commandline flag add any extra value here ? Unsure why we'd want that. Otherwise changes look okay - but please have @sherryzy or @aee-google do a bit more closer look as well.

[jellefoks]

This became apparent when implementing the functionality to allow reception of Cobalt console log in the devtools, specifically where agents_state (from the DebuggerState, owned by the BrowserModule, so there is a single shared instance for the entire app) would be modified from different threads, resulting in unexpected state and potentially crashes.

Yep, okay - if it gets in the way and causes more instability in QA builds that are used for most testing, and doesn't work in other modules anyway, it should indeed be disabled.

But still - does a new commandline flag add any extra value here ? Unsure why we'd want that. Otherwise changes look okay - but please have @sherryzy or @aee-google do a bit more closer look as well.

I added the command-line flag because 1) it was a simple change, and 2) in the past I have manually disabled the debug module often by editing code to be able to debug things happening in the MainWebModule without being distracted by the same code running for the DebugConsoleWebModule thread.

Chrome has a similar '--disable-dev-tools' switch.

[sherryzy]

Well, adding this extra command line switch does bring some extra complexity to the code, but I don't know if there a better way to do it. I trust your judgement here. Overall, the code looks good to me.