From 3171115085b6bfad31fe793b8d0bd82fa3e8d692 Mon Sep 17 00:00:00 2001 From: Alex Rudenko Date: Thu, 12 Oct 2023 12:15:06 +0200 Subject: [PATCH] refactor: move target checks to the api surface (#11137) --- packages/puppeteer-core/src/api/Browser.ts | 7 ------- packages/puppeteer-core/src/cdp/Browser.ts | 11 +++++----- .../src/cdp/ChromeTargetManager.ts | 21 ++++--------------- .../src/cdp/FirefoxTargetManager.ts | 2 +- packages/puppeteer-core/src/cdp/Target.ts | 10 ++++++++- .../puppeteer-core/src/cdp/TargetManager.ts | 2 +- test/src/cdp/TargetManager.spec.ts | 14 ++++++------- 7 files changed, 27 insertions(+), 40 deletions(-) diff --git a/packages/puppeteer-core/src/api/Browser.ts b/packages/puppeteer-core/src/api/Browser.ts index 22e6ad58..07853142 100644 --- a/packages/puppeteer-core/src/api/Browser.ts +++ b/packages/puppeteer-core/src/api/Browser.ts @@ -259,13 +259,6 @@ export abstract class Browser extends EventEmitter { throw new Error('Not implemented'); } - /** - * @internal - */ - get _targets(): Map { - throw new Error('Not implemented'); - } - /** * Gets the associated * {@link https://nodejs.org/api/child_process.html#class-childprocess | ChildProcess}. diff --git a/packages/puppeteer-core/src/cdp/Browser.ts b/packages/puppeteer-core/src/cdp/Browser.ts index 3240c6eb..068fe374 100644 --- a/packages/puppeteer-core/src/cdp/Browser.ts +++ b/packages/puppeteer-core/src/cdp/Browser.ts @@ -91,10 +91,6 @@ export class CdpBrowser extends BrowserBase { #contexts = new Map(); #targetManager: TargetManager; - override get _targets(): Map { - return this.#targetManager.getAvailableTargets(); - } - constructor( product: 'chrome' | 'firefox' | undefined, connection: Connection, @@ -314,8 +310,9 @@ export class CdpBrowser extends BrowserBase { #onAttachedToTarget = async (target: CdpTarget) => { if ( + target._isTargetExposed() && (await target._initializedDeferred.valueOrThrow()) === - InitializationStatus.SUCCESS + InitializationStatus.SUCCESS ) { this.emit(BrowserEvent.TargetCreated, target); target.browserContext().emit(BrowserContextEvent.TargetCreated, target); @@ -326,8 +323,9 @@ export class CdpBrowser extends BrowserBase { target._initializedDeferred.resolve(InitializationStatus.ABORTED); target._isClosedDeferred.resolve(); if ( + target._isTargetExposed() && (await target._initializedDeferred.valueOrThrow()) === - InitializationStatus.SUCCESS + InitializationStatus.SUCCESS ) { this.emit(BrowserEvent.TargetDestroyed, target); target.browserContext().emit(BrowserContextEvent.TargetDestroyed, target); @@ -382,6 +380,7 @@ export class CdpBrowser extends BrowserBase { this.#targetManager.getAvailableTargets().values() ).filter(target => { return ( + target._isTargetExposed() && target._initializedDeferred.value() === InitializationStatus.SUCCESS ); }); diff --git a/packages/puppeteer-core/src/cdp/ChromeTargetManager.ts b/packages/puppeteer-core/src/cdp/ChromeTargetManager.ts index 5f8843c2..19c2b47e 100644 --- a/packages/puppeteer-core/src/cdp/ChromeTargetManager.ts +++ b/packages/puppeteer-core/src/cdp/ChromeTargetManager.ts @@ -18,7 +18,6 @@ import type {Protocol} from 'devtools-protocol'; import type {TargetFilterCallback} from '../api/Browser.js'; import {CDPSession, CDPSessionEvent} from '../api/CDPSession.js'; -import {TargetType} from '../api/Target.js'; import {EventEmitter} from '../common/EventEmitter.js'; import {debugError} from '../common/util.js'; import {assert} from '../util/assert.js'; @@ -34,10 +33,6 @@ import { type TargetManagerEvents, } from './TargetManager.js'; -function isTargetExposed(target: CdpTarget): boolean { - return target.type() !== TargetType.TAB && !target._subtype(); -} - function isPageTargetBecomingPrimary( target: CdpTarget, newTargetInfo: Protocol.Target.TargetInfo @@ -183,14 +178,8 @@ export class ChromeTargetManager this.#removeAttachmentListeners(this.#connection); } - getAvailableTargets(): Map { - const result = new Map(); - for (const [id, target] of this.#attachedTargetsByTargetId.entries()) { - if (isTargetExposed(target)) { - result.set(id, target); - } - } - return result; + getAvailableTargets(): ReadonlyMap { + return this.#attachedTargetsByTargetId; } #setupAttachmentListeners(session: CDPSession | Connection): void { @@ -402,7 +391,7 @@ export class ChromeTargetManager } this.#targetsIdsForInit.delete(target._targetId); - if (!isExistingTarget && isTargetExposed(target)) { + if (!isExistingTarget) { this.emit(TargetManagerEvent.TargetAvailable, target); } this.#finishInitializationIfReady(); @@ -440,8 +429,6 @@ export class ChromeTargetManager } this.#attachedTargetsByTargetId.delete(target._targetId); - if (isTargetExposed(target)) { - this.emit(TargetManagerEvent.TargetGone, target); - } + this.emit(TargetManagerEvent.TargetGone, target); }; } diff --git a/packages/puppeteer-core/src/cdp/FirefoxTargetManager.ts b/packages/puppeteer-core/src/cdp/FirefoxTargetManager.ts index f5063dc3..548e7ffc 100644 --- a/packages/puppeteer-core/src/cdp/FirefoxTargetManager.ts +++ b/packages/puppeteer-core/src/cdp/FirefoxTargetManager.ts @@ -131,7 +131,7 @@ export class FirefoxTargetManager } } - getAvailableTargets(): Map { + getAvailableTargets(): ReadonlyMap { return this.#availableTargetsByTargetId; } diff --git a/packages/puppeteer-core/src/cdp/Target.ts b/packages/puppeteer-core/src/cdp/Target.ts index 0d09d654..936137a4 100644 --- a/packages/puppeteer-core/src/cdp/Target.ts +++ b/packages/puppeteer-core/src/cdp/Target.ts @@ -163,7 +163,11 @@ export class CdpTarget extends Target { if (!openerId) { return; } - return this.browser()._targets.get(openerId); + return this.browser() + .targets() + .find(target => { + return (target as CdpTarget)._targetId === openerId; + }); } _targetInfoChanged(targetInfo: Protocol.Target.TargetInfo): void { @@ -175,6 +179,10 @@ export class CdpTarget extends Target { this._initializedDeferred.resolve(InitializationStatus.SUCCESS); } + _isTargetExposed(): boolean { + return this.type() !== TargetType.TAB && !this._subtype(); + } + protected _checkIfInitialized(): void { if (!this._initializedDeferred.resolved()) { this._initializedDeferred.resolve(InitializationStatus.SUCCESS); diff --git a/packages/puppeteer-core/src/cdp/TargetManager.ts b/packages/puppeteer-core/src/cdp/TargetManager.ts index 8a90073b..64935efa 100644 --- a/packages/puppeteer-core/src/cdp/TargetManager.ts +++ b/packages/puppeteer-core/src/cdp/TargetManager.ts @@ -69,7 +69,7 @@ export interface TargetManagerEvents extends Record { * @internal */ export interface TargetManager extends EventEmitter { - getAvailableTargets(): Map; + getAvailableTargets(): ReadonlyMap; initialize(): Promise; dispose(): void; } diff --git a/test/src/cdp/TargetManager.spec.ts b/test/src/cdp/TargetManager.spec.ts index 91ec840e..45a20658 100644 --- a/test/src/cdp/TargetManager.spec.ts +++ b/test/src/cdp/TargetManager.spec.ts @@ -53,18 +53,18 @@ describe('TargetManager', () => { const {server, context, browser} = state; const targetManager = browser._targetManager(); - expect(targetManager.getAvailableTargets().size).toBe(2); + expect(targetManager.getAvailableTargets().size).toBe(3); expect(await context.pages()).toHaveLength(0); - expect(targetManager.getAvailableTargets().size).toBe(2); + expect(targetManager.getAvailableTargets().size).toBe(3); const page = await context.newPage(); expect(await context.pages()).toHaveLength(1); - expect(targetManager.getAvailableTargets().size).toBe(3); + expect(targetManager.getAvailableTargets().size).toBe(5); await page.goto(server.EMPTY_PAGE); expect(await context.pages()).toHaveLength(1); - expect(targetManager.getAvailableTargets().size).toBe(3); + expect(targetManager.getAvailableTargets().size).toBe(5); // attach a local iframe. let framePromise = page.waitForFrame(frame => { @@ -73,7 +73,7 @@ describe('TargetManager', () => { await attachFrame(page, 'frame1', server.EMPTY_PAGE); await framePromise; expect(await context.pages()).toHaveLength(1); - expect(targetManager.getAvailableTargets().size).toBe(3); + expect(targetManager.getAvailableTargets().size).toBe(5); expect(page.frames()).toHaveLength(2); // // attach a remote frame iframe. @@ -87,7 +87,7 @@ describe('TargetManager', () => { ); await framePromise; expect(await context.pages()).toHaveLength(1); - expect(targetManager.getAvailableTargets().size).toBe(4); + expect(targetManager.getAvailableTargets().size).toBe(6); expect(page.frames()).toHaveLength(3); framePromise = page.waitForFrame(frame => { @@ -100,7 +100,7 @@ describe('TargetManager', () => { ); await framePromise; expect(await context.pages()).toHaveLength(1); - expect(targetManager.getAvailableTargets().size).toBe(5); + expect(targetManager.getAvailableTargets().size).toBe(7); expect(page.frames()).toHaveLength(4); }); });