core(driver): do not use target manager in legacy mode #14079
Conversation
| /** | ||
| * @param {LH.Crdp.Target.AttachedToTargetEvent} event | ||
| */ | ||
| async _onTargetAttachedLegacy(event) { |
There was a problem hiding this comment.
i looked a this compared to my old solution:
and see that you're still ensuring that networkmonitor calls network.enable for every new target. and my thing wouldn't have done that.
so.. 👍 !
There was a problem hiding this comment.
actually we don't need to do any of this. driver is already enabling the network domain for us.
|
Pretty certain the failing smokes are unrelated, given they are FR and this PR only impact legacy. |
1 similar comment
|
Pretty certain the failing smokes are unrelated, given they are FR and this PR only impact legacy. |
|
|
||
| // Legacy driver does its own target management. | ||
| // @ts-expect-error | ||
| const isLegacyRunner = Boolean(this._session._domainEnabledCounts); |
There was a problem hiding this comment.
Are you planning to remove this hack as part of #14078, or once the legacy runner is removed?
There was a problem hiding this comment.
IMO the latter. (once the legacy runner is removed)
There was a problem hiding this comment.
uh, whatever comes first?
Related to #14078. In the legacy driver, we are already handling targets via the event handlers added in the constructor. Let's not complicate things in legacy mode by doing target management in a second place (only the NetworkMonitor is what creates a TargetManager in legacy mode).