Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: make target manager accessible on driver #14122

Merged
merged 7 commits into from
Jun 22, 2022
Merged

core: make target manager accessible on driver #14122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
06a655c
core: make targetManager accessible on driver
brendankenny Jun 10, 2022
ccc846f
feedback
brendankenny Jun 14, 2022
b3a55b2
target-manager-test
brendankenny Jun 16, 2022
f78707e
move protocolevent tests from session to target-manager
brendankenny Jun 16, 2022
a3bc0c7
network-monitor-test
brendankenny Jun 17, 2022
f4d4c8c
driver-test
brendankenny Jun 17, 2022
39b9d64
full-page-screenshot-test
brendankenny Jun 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions lighthouse-core/fraggle-rock/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
'use strict';

const log = require('lighthouse-logger');
const ProtocolSession = require('./session.js');
const ExecutionContext = require('../../gather/driver/execution-context.js');
const Fetcher = require('../../gather/fetcher.js');
const TargetManager = require('../../gather/driver/target-manager.js');

/** @return {*} */
const throwNotConnectedFn = () => {
Expand All @@ -24,8 +24,6 @@ const throwingSession = {
on: throwNotConnectedFn,
once: throwNotConnectedFn,
off: throwNotConnectedFn,
addProtocolMessageListener: throwNotConnectedFn,
removeProtocolMessageListener: throwNotConnectedFn,
sendCommand: throwNotConnectedFn,
dispose: throwNotConnectedFn,
};
Expand All @@ -37,6 +35,8 @@ class Driver {
*/
constructor(page) {
this._page = page;
/** @type {TargetManager|undefined} */
this._targetManager = undefined;
/** @type {ExecutionContext|undefined} */
this._executionContext = undefined;
/** @type {Fetcher|undefined} */
Expand All @@ -57,6 +57,11 @@ class Driver {
return this._fetcher;
}

get targetManager() {
if (!this._targetManager) return throwNotConnectedFn();
return this._targetManager;
}

/** @return {Promise<string>} */
async url() {
return this._page.url();
Expand All @@ -67,8 +72,10 @@ class Driver {
if (this.defaultSession !== throwingSession) return;
const status = {msg: 'Connecting to browser', id: 'lh:driver:connect'};
log.time(status);
const session = await this._page.target().createCDPSession();
this.defaultSession = new ProtocolSession(session);
const cdpSession = await this._page.target().createCDPSession();
this._targetManager = new TargetManager(cdpSession);
await this._targetManager.enable();
this.defaultSession = this._targetManager.rootSession();
Comment on lines +75 to +78
Copy link
Member Author

@brendankenny brendankenny Jun 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because TargetManager is in charge of creating FR ProtocolSessions from CDPSessions, and we want to eventually make those sessions available for gatherers to access directly, Driver now gets its driver.defaultSession from targetManager instead of making it itself. This is a little weird, but otherwise we have two different session objects pointing to the same thing. This seemed conceptually simpler (driver setup is a little more involved but it means the set of all active sessions includes defaultSession) but open to ideas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big fan of avoiding the double session object! (I was seeing that at the CDP traffic level. so weird.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd love to s/defaultSession/rootSession/g but fine with it happening in a followup.

this._executionContext = new ExecutionContext(this.defaultSession);
this._fetcher = new Fetcher(this.defaultSession);
log.timeEnd(status);
Expand All @@ -77,6 +84,7 @@ class Driver {
/** @return {Promise<void>} */
async disconnect() {
if (this.defaultSession === throwingSession) return;
this._targetManager?.disable();
await this.defaultSession.dispose();
}
}
Expand Down
37 changes: 2 additions & 35 deletions lighthouse-core/fraggle-rock/gather/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,10 @@ class ProtocolSession {
this._targetInfo = undefined;
/** @type {number|undefined} */
this._nextProtocolTimeout = undefined;
/** @type {WeakMap<any, any>} */
this._callbackMap = new WeakMap();
}

sessionId() {
return this._targetInfo && this._targetInfo.type === 'iframe' ?
// TODO: use this._session.id() for real session id.
this._targetInfo.targetId :
undefined;
id() {
return this._cdpSession.id();
}

/** @param {LH.Crdp.Target.TargetInfo} targetInfo */
Expand Down Expand Up @@ -78,34 +73,6 @@ class ProtocolSession {
this._cdpSession.once(eventName, /** @type {*} */ (callback));
}

/**
* Bind to puppeteer's '*' event that fires for *any* protocol event,
* and wrap it with data about the protocol message instead of just the event.
* @param {(payload: LH.Protocol.RawEventMessage) => void} callback
*/
addProtocolMessageListener(callback) {
/**
* @param {any} method
* @param {any} event
*/
const listener = (method, event) => callback({
method,
params: event,
sessionId: this.sessionId(),
});
this._callbackMap.set(callback, listener);
this._cdpSession.on('*', /** @type {*} */ (listener));
}

/**
* @param {(payload: LH.Protocol.RawEventMessage) => void} callback
*/
removeProtocolMessageListener(callback) {
const listener = this._callbackMap.get(callback);
if (!listener) return;
this._cdpSession.off('*', /** @type {*} */ (listener));
}

/**
* Bind listeners for protocol events.
* @template {keyof LH.CrdpEvents} E
Expand Down
40 changes: 24 additions & 16 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,30 @@ class Driver {
this.evaluate = this.executionContext.evaluate.bind(this.executionContext);
/** @private @deprecated Only available for plugin backcompat. */
this.evaluateAsync = this.executionContext.evaluateAsync.bind(this.executionContext);

// A shim for sufficient coverage of targetManager functionality. Exposes the target
// management that legacy driver already handles (see this._handleTargetAttached).
this.targetManager = {
rootSession: () => {
return this.defaultSession;
},
/**
* Bind to *any* protocol event.
* @param {'protocolevent'} event
* @param {(payload: LH.Protocol.RawEventMessage) => void} callback
*/
on: (event, callback) => {
this._connection.on('protocolevent', callback);
},
/**
* Unbind to *any* protocol event.
* @param {'protocolevent'} event
* @param {(payload: LH.Protocol.RawEventMessage) => void} callback
*/
off: (event, callback) => {
this._connection.off('protocolevent', callback);
},
};
}

/** @deprecated - Not available on Fraggle Rock driver. */
Expand Down Expand Up @@ -187,22 +211,6 @@ class Driver {
this._eventEmitter.removeListener(eventName, cb);
}

/**
* Bind to *any* protocol event.
* @param {(payload: LH.Protocol.RawEventMessage) => void} callback
*/
addProtocolMessageListener(callback) {
this._connection.on('protocolevent', callback);
}

/**
* Unbind to *any* protocol event.
* @param {(payload: LH.Protocol.RawEventMessage) => void} callback
*/
removeProtocolMessageListener(callback) {
this._connection.off('protocolevent', callback);
}

/** @param {LH.Crdp.Target.TargetInfo} targetInfo */
setTargetInfo(targetInfo) { // eslint-disable-line no-unused-vars
// OOPIF handling in legacy driver is implicit.
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/driver/navigation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ async function gotoURL(driver, requestor, options) {
log.time(status);

const session = driver.defaultSession;
const networkMonitor = new NetworkMonitor(driver.defaultSession);
const networkMonitor = new NetworkMonitor(driver.targetManager);

// Enable the events and network monitor needed to track navigation progress.
await networkMonitor.enable();
Expand Down
71 changes: 12 additions & 59 deletions lighthouse-core/gather/driver/network-monitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,38 +15,34 @@ const {EventEmitter} = require('events');
const NetworkRecorder = require('../../lib/network-recorder.js');
const NetworkRequest = require('../../lib/network-request.js');
const URL = require('../../lib/url-shim.js');
const TargetManager = require('./target-manager.js');

/** @typedef {import('../../lib/network-recorder.js').NetworkRecorderEvent} NetworkRecorderEvent */
/** @typedef {'network-2-idle'|'network-critical-idle'|'networkidle'|'networkbusy'|'network-critical-busy'|'network-2-busy'} NetworkMonitorEvent_ */
/** @typedef {NetworkRecorderEvent|NetworkMonitorEvent_} NetworkMonitorEvent */
/** @typedef {Record<NetworkMonitorEvent_, []> & Record<NetworkRecorderEvent, [NetworkRequest]> & {protocolmessage: [LH.Protocol.RawEventMessage]}} NetworkMonitorEventMap */
/** @typedef {Record<NetworkMonitorEvent_, []> & Record<NetworkRecorderEvent, [NetworkRequest]>} NetworkMonitorEventMap */
/** @typedef {LH.Protocol.StrictEventEmitter<NetworkMonitorEventMap>} NetworkMonitorEmitter */

/** @implements {NetworkMonitorEmitter} */
class NetworkMonitor {
/** @type {NetworkRecorder|undefined} */
_networkRecorder = undefined;
/** @type {TargetManager|undefined} */
_targetManager = undefined;
/** @type {Array<LH.Crdp.Page.Frame>} */
_frameNavigations = [];

/** @param {LH.Gatherer.FRProtocolSession} session */
constructor(session) {
this._session = session;
// TODO(FR-COMPAT): switch to real TargetManager when legacy removed.
/** @param {LH.Gatherer.FRTransitionalDriver['targetManager']} targetManager */
constructor(targetManager) {
/** @type {LH.Gatherer.FRTransitionalDriver['targetManager']} */
this._targetManager = targetManager;

this._onTargetAttached = this._onTargetAttached.bind(this);

/** @type {Map<string, LH.Gatherer.FRProtocolSession>} */
this._sessions = new Map();
/** @type {LH.Gatherer.FRProtocolSession} */
this._session = targetManager.rootSession();

/** @param {LH.Crdp.Page.FrameNavigatedEvent} event */
this._onFrameNavigated = event => this._frameNavigations.push(event.frame);

/** @param {LH.Protocol.RawEventMessage} event */
this._onProtocolMessage = event => {
this.emit('protocolmessage', event);
if (!this._networkRecorder) return;
this._networkRecorder.dispatch(event);
};
Expand All @@ -69,31 +65,14 @@ class NetworkMonitor {
this.removeAllListeners = emitter.removeAllListeners.bind(emitter);
}

/**
* @param {{target: {targetId: string}, session: LH.Gatherer.FRProtocolSession}} session
*/
async _onTargetAttached({session, target}) {
const targetId = target.targetId;

this._sessions.set(targetId, session);
session.addProtocolMessageListener(this._onProtocolMessage);

await session.sendCommand('Network.enable');
}

/**
* @return {Promise<void>}
*/
async enable() {
if (this._targetManager) return;
if (this._networkRecorder) return;

this._frameNavigations = [];
this._sessions = new Map();
this._networkRecorder = new NetworkRecorder();
/** @type {LH.Puppeteer.CDPSession} */
// @ts-expect-error - temporarily reach in to get CDPSession
const rootCdpSession = this._session._cdpSession;
this._targetManager = new TargetManager(rootCdpSession);

/**
* Reemit the same network recorder events.
Expand All @@ -109,46 +88,20 @@ class NetworkMonitor {
this._networkRecorder.on('requestfinished', reEmit('requestfinished'));

this._session.on('Page.frameNavigated', this._onFrameNavigated);
await this._session.sendCommand('Page.enable');

// Legacy driver does its own target management.
// @ts-expect-error
const isLegacyRunner = Boolean(this._session._domainEnabledCounts);
if (isLegacyRunner) {
this._session.addProtocolMessageListener(this._onProtocolMessage);
} else {
this._targetManager.addTargetAttachedListener(this._onTargetAttached);
await this._targetManager.enable();
}
this._targetManager.on('protocolevent', this._onProtocolMessage);
}

/**
* @return {Promise<void>}
*/
async disable() {
if (!this._targetManager) return;
if (!this._networkRecorder) return;

this._session.off('Page.frameNavigated', this._onFrameNavigated);

// Legacy driver does its own target management.
// @ts-expect-error
const isLegacyRunner = Boolean(this._session._domainEnabledCounts);
if (isLegacyRunner) {
this._session.removeProtocolMessageListener(this._onProtocolMessage);
} else {
this._targetManager.removeTargetAttachedListener(this._onTargetAttached);

for (const session of this._sessions.values()) {
session.removeProtocolMessageListener(this._onProtocolMessage);
}

await this._targetManager.disable();
}
this._targetManager.off('protocolevent', this._onProtocolMessage);

this._frameNavigations = [];
this._networkRecorder = undefined;
this._targetManager = undefined;
this._sessions = new Map();
}

/** @return {Promise<{requestedUrl?: string, mainDocumentUrl?: string}>} */
Expand Down
Loading