refactor: move target checks to the api surface (#11137)

packages/puppeteer-core/src/api/Browser.ts

@@ -259,13 +259,6 @@ export abstract class Browser extends EventEmitter<BrowserEvents> {
     throw new Error('Not implemented');
   }
 
-  /**
-   * @internal
-   */
-  get _targets(): Map<string, Target> {
-    throw new Error('Not implemented');
-  }
-
   /**
    * Gets the associated
    * {@link https://nodejs.org/api/child_process.html#class-childprocess | ChildProcess}.

packages/puppeteer-core/src/cdp/Browser.ts

@@ -91,10 +91,6 @@ export class CdpBrowser extends BrowserBase {
   #contexts = new Map<string, CdpBrowserContext>();
   #targetManager: TargetManager;
 
-  override get _targets(): Map<string, CdpTarget> {
-    return this.#targetManager.getAvailableTargets();
-  }
-
   constructor(
     product: 'chrome' | 'firefox' | undefined,
     connection: Connection,
@@ -314,6 +310,7 @@ export class CdpBrowser extends BrowserBase {
 
   #onAttachedToTarget = async (target: CdpTarget) => {
     if (
+      target._isTargetExposed() &&
       (await target._initializedDeferred.valueOrThrow()) ===
         InitializationStatus.SUCCESS
     ) {
@@ -326,6 +323,7 @@ export class CdpBrowser extends BrowserBase {
     target._initializedDeferred.resolve(InitializationStatus.ABORTED);
     target._isClosedDeferred.resolve();
     if (
+      target._isTargetExposed() &&
       (await target._initializedDeferred.valueOrThrow()) ===
         InitializationStatus.SUCCESS
     ) {
@@ -382,6 +380,7 @@ export class CdpBrowser extends BrowserBase {
       this.#targetManager.getAvailableTargets().values()
     ).filter(target => {
       return (
+        target._isTargetExposed() &&
         target._initializedDeferred.value() === InitializationStatus.SUCCESS
       );
     });

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<string, CdpTarget> {
-    const result = new Map<string, CdpTarget>();
-    for (const [id, target] of this.#attachedTargetsByTargetId.entries()) {
-      if (isTargetExposed(target)) {
-        result.set(id, target);
-      }
-    }
-    return result;
+  getAvailableTargets(): ReadonlyMap<string, CdpTarget> {
+    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);
-    }
   };
 }

packages/puppeteer-core/src/cdp/FirefoxTargetManager.ts

@@ -131,7 +131,7 @@ export class FirefoxTargetManager
     }
   }
 
-  getAvailableTargets(): Map<string, CdpTarget> {
+  getAvailableTargets(): ReadonlyMap<string, CdpTarget> {
     return this.#availableTargetsByTargetId;
   }
 

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);

packages/puppeteer-core/src/cdp/TargetManager.ts

@@ -69,7 +69,7 @@ export interface TargetManagerEvents extends Record<EventType, unknown> {
  * @internal
  */
 export interface TargetManager extends EventEmitter<TargetManagerEvents> {
-  getAvailableTargets(): Map<string, CdpTarget>;
+  getAvailableTargets(): ReadonlyMap<string, CdpTarget>;
   initialize(): Promise<void>;
   dispose(): void;
 }

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);
   });
 });