core(network-recorder): use findNetworkQuietPeriods for networkIdle #4102
Conversation
patrickhulce
changed the title
| @@ -53,22 +130,17 @@ class NetworkRecorder extends EventEmitter { | |||
| */ | |||
| onRequestStarted(request) { | |||
| this.startedRequestCount++; | |||
| request.data._observedNodeStartTime = Date.now(); | |||
There was a problem hiding this comment.
what are we doing with these two timestamps?
There was a problem hiding this comment.
we actually aren't anymore but it was useful for debugging delta between trace and epoch timestamps :)
|
findNetworkQuietPeriods is largely untouched other than 1 new filter that's arguably unnecessary, the meat of the PR is that we use these periods to determine isIdle and isIdle2 instead of the old counts |
| if (activeCount === 1) { | ||
| this.emit('networkbusy'); | ||
| } | ||
| log.verbose( |
There was a problem hiding this comment.
do we want to keep these lines attached to activeRequestCount now? since our idling logic isn't related it seems better to drop the logs or have them log out the networkStatus
There was a problem hiding this comment.
yeah good point I just removed all other active request count tracking since we don't really need it, moved log to network status
| * @param {!Array<!WebInspector.NetworkRequest>} networkRecords | ||
| * @param {number} allowedConcurrentRequests | ||
| * @param {number=} endTime | ||
| * @return {!Array<{start: number, end: number, ongoing: boolean|undefined}>} |
There was a problem hiding this comment.
naming bikeshed! is ongoing === "in flight"? if so i'd prefer that as a name, but i suspect there's a difference already..... :)
There was a problem hiding this comment.
yeah they're different haha :)
it's a flag to signal that this period had no end, basically that it's Infinity even though we mark it at endTime, suggestions welcome I just nuked it instead :)
|
@patrickhulce before landing, should we tweak this commit message any? i feel like there's a decent size gap between the actual change and the fix. how about something like this:
land at will. |
|
yeah name change sgtm 👍 |
patrickhulce
changed the title
fixes #3734