core: single network monitor kept on Driver #15055
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
connorjclark
commented
May 9, 2023
| @@ -50,6 +51,7 @@ declare module Gatherer { | |||
| on(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void | |||
| off(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void | |||
| }; | |||
| networkMonitor: NetworkMonitor; | |||
There was a problem hiding this comment.
I know I should type just the public interface here, but then we get this error...
How can we resolve this?
There was a problem hiding this comment.
Should we pull waitForFullyLoaded (and whatever else...) into NetworkMonitor? expose as NetworkMonitor.waitForFullyLoaded(session, opts)?
There was a problem hiding this comment.
Eh, waitForFullyLoaded does other non-network stuff like CPU idle. I think it's fine this way personally.
connorjclark
force-pushed
the
one-network-monitor
branch
from
9e05018 to
f81206c
Compare
connorjclark
force-pushed
the
one-network-monitor
branch
from
f81206c to
a60e6bf
Compare
adamraine
approved these changes
Jul 19, 2023
|
Looks like you have to do the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref #14078
Moves
NetworkMonitortoDriver, so that the navigation runner and FPS gatherer can use the same instance.Without changing the lifecycle of the monitor: the navigation runner enables/disables it for
goToURLonly, and the FPS does the same ingetArtifact(so after the monitor has been disabled in navigation mode - but for the first and only time in other modes). If a gatherer wanted to use the monitor in earlier gathering stages, we need to modify how the monitor is enabled. One option is to track a counter of enables/disables (like we do for protocol domains). Another is to just enable unconditionally inDriver.connect, so it's on constantly for all modes and gatherers. The second commit in the PR tries this approach.