Refactor browser creation flow, add ownership model

extensions/vscode-api-tests/src/singlefolder-tests/browser.test.ts

@@ -73,6 +73,23 @@ import { assertNoRpc, closeAllEditors } from '../utils';
 		assert.strictEqual(window.browserTabs.length, countBefore - 1);
 	});
 
+	test('Closing via workbench.action.closeActiveEditor removes tab from browserTabs', async () => {
+		const tab = await window.openBrowserTab('about:blank');
+		assert.ok(window.browserTabs.includes(tab));
+
+		const closed = new Promise<vscode.BrowserTab>(resolve => {
+			const disposable = window.onDidCloseBrowserTab(t => {
+				disposable.dispose();
+				resolve(t);
+			});
+		});
+
+		await commands.executeCommand('workbench.action.closeActiveEditor');
+		const firedTab = await closed;
+		assert.ok(firedTab);
+		assert.ok(!window.browserTabs.includes(tab));
+	});
+
 	test('Can move a browser tab to a new group and close it successfully', async () => {
 		const tab = await window.openBrowserTab('about:blank');
 		assert.ok(window.browserTabs.includes(tab));

src/vs/platform/browserView/common/browserView.ts

@@ -5,7 +5,6 @@
 
 import { Event } from '../../../base/common/event.js';
 import { VSBuffer } from '../../../base/common/buffer.js';
-import { UriComponents } from '../../../base/common/uri.js';
 import { localize } from '../../../nls.js';
 
 const commandPrefix = 'workbench.action.browser';
@@ -81,6 +80,49 @@ export interface IBrowserViewCaptureScreenshotOptions {
 	pageRect?: { x: number; y: number; width: number; height: number };
 }
 
+/**
+ * Identifies who owns a browser view's lifecycle.
+ * The owner is set at creation time and never changes.
+ */
+export interface IBrowserViewOwner {
+	/** The main code window ID that owns this view's lifecycle. */
+	readonly mainWindowId: number;
+}
+
+/**
+ * Summary information about a browser view, including its current state and
+ * ownership. Returned by the main service when listing or creating views.
+ */
+export interface IBrowserViewInfo {
+	readonly id: string;
+	readonly owner: IBrowserViewOwner;
+	readonly state: IBrowserViewState;
+}
+
+/**
+ * Editor opening hints passed from the main process to the workbench.
+ */
+export interface IBrowserViewOpenOptions {
+	readonly preserveFocus?: boolean;
+	readonly background?: boolean;
+	readonly pinned?: boolean;
+	/** The parent view ID. Used by the workbench to place the new tab in the same editor group. */
+	readonly parentViewId?: string;
+	/** When set, open in an auxiliary (new) window with these bounds. */
+	readonly auxiliaryWindow?: { x?: number; y?: number; width?: number; height?: number };
+}
+
+export interface IBrowserViewCreatedEvent {
+	readonly info: IBrowserViewInfo;
+	readonly openOptions: IBrowserViewOpenOptions;
+}
+
+export interface IBrowserViewCreateOptions {
+	readonly owner: IBrowserViewOwner;
+	readonly scope: BrowserViewStorageScope;
+	readonly initialState?: Partial<IBrowserViewState>;
+}
+
 export interface IBrowserViewState {
 	url: string;
 	title: string;
@@ -161,19 +203,6 @@ export interface IBrowserViewFaviconChangeEvent {
 	favicon: string | undefined;
 }
 
-export enum BrowserNewPageLocation {
-	Foreground = 'foreground',
-	Background = 'background',
-	NewWindow = 'newWindow'
-}
-export interface IBrowserViewNewPageRequest {
-	resource: UriComponents;
-	url: string;
-	location: BrowserNewPageLocation;
-	// Only applicable if location is NewWindow
-	position?: { x?: number; y?: number; width?: number; height?: number };
-}
-
 export interface IBrowserViewFindInPageOptions {
 	recompute?: boolean;
 	forward?: boolean;
@@ -214,6 +243,11 @@ export function browserZoomAccessibilityLabel(zoomFactor: number): string {
 export const browserViewIsolatedWorldId = 999;
 
 export interface IBrowserViewService {
+	/**
+	 * Fires when a new browser view is created from an internal source (e.g. CDP or window.open).
+	 */
+	onDidCreateBrowserView: Event<IBrowserViewCreatedEvent>;
+
 	/**
 	 * Dynamic events that return an Event for a specific browser view ID.
 	 */
@@ -225,17 +259,21 @@ export interface IBrowserViewService {
 	onDynamicDidKeyCommand(id: string): Event<IBrowserViewKeyDownEvent>;
 	onDynamicDidChangeTitle(id: string): Event<IBrowserViewTitleChangeEvent>;
 	onDynamicDidChangeFavicon(id: string): Event<IBrowserViewFaviconChangeEvent>;
-	onDynamicDidRequestNewPage(id: string): Event<IBrowserViewNewPageRequest>;
 	onDynamicDidFindInPage(id: string): Event<IBrowserViewFindInPageResult>;
 	onDynamicDidClose(id: string): Event<void>;
 
 	/**
-	 * Get or create a browser view instance
+	 * Get all known browser views with their ownership and state information.
+	 */
+	getBrowserViews(windowId?: number): Promise<IBrowserViewInfo[]>;
+
+	/**
+	 * Get or create a browser view instance. Does not fire `onDidCreateBrowserView`.
+	 *
 	 * @param id The browser view identifier
-	 * @param scope The storage scope for the browser view. Ignored if the view already exists.
-	 * @param workspaceId Workspace identifier for session isolation. Only used if scope is 'workspace'.
+	 * @param options Creation options. If a view with the given ID already exists, these options are ignored.
 	 */
-	getOrCreateBrowserView(id: string, scope: BrowserViewStorageScope, workspaceId?: string): Promise<IBrowserViewState>;
+	getOrCreateBrowserView(id: string, options: IBrowserViewCreateOptions): Promise<IBrowserViewState>;
 
 	/**
 	 * Destroy a browser view instance

src/vs/platform/browserView/electron-main/browserView.ts

@@ -8,18 +8,24 @@ import { Disposable } from '../../../base/common/lifecycle.js';
 import { Emitter, Event } from '../../../base/common/event.js';
 import { VSBuffer } from '../../../base/common/buffer.js';
 import { CancellationToken } from '../../../base/common/cancellation.js';
-import { IBrowserViewBounds, IBrowserViewDevToolsStateEvent, IBrowserViewFocusEvent, IBrowserViewKeyDownEvent, IBrowserViewState, IBrowserViewNavigationEvent, IBrowserViewLoadingEvent, IBrowserViewLoadError, IBrowserViewTitleChangeEvent, IBrowserViewFaviconChangeEvent, IBrowserViewNewPageRequest, IBrowserViewCaptureScreenshotOptions, IBrowserViewFindInPageOptions, IBrowserViewFindInPageResult, IBrowserViewVisibilityEvent, BrowserNewPageLocation, browserViewIsolatedWorldId, browserZoomFactors, browserZoomDefaultIndex, IElementData } from '../common/browserView.js';
+import { IBrowserViewBounds, IBrowserViewDevToolsStateEvent, IBrowserViewFocusEvent, IBrowserViewKeyDownEvent, IBrowserViewState, IBrowserViewNavigationEvent, IBrowserViewLoadingEvent, IBrowserViewLoadError, IBrowserViewTitleChangeEvent, IBrowserViewFaviconChangeEvent, IBrowserViewCaptureScreenshotOptions, IBrowserViewFindInPageOptions, IBrowserViewFindInPageResult, IBrowserViewVisibilityEvent, browserViewIsolatedWorldId, browserZoomFactors, browserZoomDefaultIndex, IElementData, IBrowserViewOwner, IBrowserViewOpenOptions } from '../common/browserView.js';
 import { BrowserViewElementInspector } from './browserViewElementInspector.js';
 import { IWindowsMainService } from '../../windows/electron-main/windows.js';
 import { ICodeWindow } from '../../window/electron-main/window.js';
 import { IAuxiliaryWindowsMainService } from '../../auxiliaryWindow/electron-main/auxiliaryWindows.js';
-import { BrowserViewUri } from '../common/browserViewUri.js';
 import { BrowserViewDebugger } from './browserViewDebugger.js';
 import { ILogService } from '../../log/common/log.js';
 import { BrowserSession } from './browserSession.js';
 import { IAuxiliaryWindow } from '../../auxiliaryWindow/electron-main/auxiliaryWindow.js';
-import { hasKey } from '../../../base/common/types.js';
 import { SCAN_CODE_STR_TO_EVENT_KEY_CODE } from '../../../base/common/keyCodes.js';
+import { ITelemetryService } from '../../telemetry/common/telemetry.js';
+import { logBrowserOpen } from '../common/browserViewTelemetry.js';
+
+enum NewPageLocation {
+	Foreground = 'foreground',
+	Background = 'background',
+	NewWindow = 'newWindow'
+}
 
 /**
  * Represents a single browser view instance with its WebContentsView and all associated logic.
@@ -37,7 +43,9 @@ export class BrowserView extends Disposable {
 
 	readonly debugger: BrowserViewDebugger;
 	private readonly _inspector: BrowserViewElementInspector;
-	private _window: ICodeWindow | IAuxiliaryWindow | undefined;
+
+	private _ownerWindow: ICodeWindow;
+	private _currentWindow: ICodeWindow | IAuxiliaryWindow | undefined;
 	private _isDisposed = false;
 
 	private static readonly MAX_CONSOLE_LOG_ENTRIES = 1000;
@@ -67,9 +75,6 @@ export class BrowserView extends Disposable {
 	private readonly _onDidChangeFavicon = this._register(new Emitter<IBrowserViewFaviconChangeEvent>());
 	readonly onDidChangeFavicon: Event<IBrowserViewFaviconChangeEvent> = this._onDidChangeFavicon.event;
 
-	private readonly _onDidRequestNewPage = this._register(new Emitter<IBrowserViewNewPageRequest>());
-	readonly onDidRequestNewPage: Event<IBrowserViewNewPageRequest> = this._onDidRequestNewPage.event;
-
 	private readonly _onDidFindInPage = this._register(new Emitter<IBrowserViewFindInPageResult>());
 	readonly onDidFindInPage: Event<IBrowserViewFindInPageResult> = this._onDidFindInPage.event;
 
@@ -78,13 +83,15 @@ export class BrowserView extends Disposable {
 
 	constructor(
 		public readonly id: string,
+		public readonly owner: IBrowserViewOwner,
 		public readonly session: BrowserSession,
-		createChildView: (options?: Electron.WebContentsViewConstructorOptions) => BrowserView,
+		createChildView: (url: string, electronOptions: Electron.WebContentsViewConstructorOptions | undefined, openOptions: IBrowserViewOpenOptions) => BrowserView,
 		openContextMenu: (view: BrowserView, params: Electron.ContextMenuParams) => void,
 		options: Electron.WebContentsViewConstructorOptions | undefined,
 		@IWindowsMainService private readonly windowsMainService: IWindowsMainService,
 		@IAuxiliaryWindowsMainService private readonly auxiliaryWindowsMainService: IAuxiliaryWindowsMainService,
-		@ILogService private readonly logService: ILogService
+		@ILogService private readonly logService: ILogService,
+		@ITelemetryService private readonly telemetryService: ITelemetryService,
 	) {
 		super();
 
@@ -108,12 +115,18 @@ export class BrowserView extends Disposable {
 		});
 		this._view.setBackgroundColor('#FFFFFF');
 
+		this._ownerWindow = this.windowsMainService.getWindowById(owner.mainWindowId)!;
+		if (!this._ownerWindow) {
+			throw new Error(`Window with ID ${owner.mainWindowId} not found`);
+		}
+		this._register(this._ownerWindow.onDidClose(() => this.dispose()));
+
 		this._view.webContents.setWindowOpenHandler((details) => {
 			const location = (() => {
 				switch (details.disposition) {
-					case 'background-tab': return BrowserNewPageLocation.Background;
-					case 'foreground-tab': return BrowserNewPageLocation.Foreground;
-					case 'new-window': return BrowserNewPageLocation.NewWindow;
+					case 'background-tab': return NewPageLocation.Background;
+					case 'foreground-tab': return NewPageLocation.Foreground;
+					case 'new-window': return NewPageLocation.NewWindow;
 					default: return undefined;
 				}
 			})();
@@ -126,15 +139,21 @@ export class BrowserView extends Disposable {
 			return {
 				action: 'allow',
 				createWindow: (options) => {
-					const childView = createChildView(options);
-					const resource = BrowserViewUri.forId(childView.id);
-
-					// Fire event for the workbench to open this view
-					this._onDidRequestNewPage.fire({
-						resource,
-						url: details.url,
-						location,
-						position: { x: options.x, y: options.y, width: options.width, height: options.height }
+					logBrowserOpen(this.telemetryService, (() => {
+						switch (location) {
+							case NewPageLocation.NewWindow: return 'browserLinkNewWindow';
+							case NewPageLocation.Background: return 'browserLinkBackground';
+							case NewPageLocation.Foreground: return 'browserLinkForeground';
+						}
+					})());
+
+					const childView = createChildView(details.url, options, {
+						pinned: true,
+						background: location === NewPageLocation.Background,
+						parentViewId: id,
+						auxiliaryWindow: location === NewPageLocation.NewWindow
+							? { x: options.x, y: options.y, width: options.width, height: options.height }
+							: undefined,
 					});
 
 					// Return the webContents so Electron can complete the window.open() call
@@ -386,12 +405,12 @@ export class BrowserView extends Disposable {
 		});
 	}
 
-	private consumePopupPermission(location: BrowserNewPageLocation): boolean {
+	private consumePopupPermission(location: NewPageLocation): boolean {
 		switch (location) {
-			case BrowserNewPageLocation.Foreground:
-			case BrowserNewPageLocation.Background:
+			case NewPageLocation.Foreground:
+			case NewPageLocation.Background:
 				return true;
-			case BrowserNewPageLocation.NewWindow:
+			case NewPageLocation.NewWindow:
 				// Each user gesture allows one popup window within 1 second
 				if (this._lastUserGestureTimestamp > Date.now() - 1000) {
 					this._lastUserGestureTimestamp = -Infinity;
@@ -442,11 +461,11 @@ export class BrowserView extends Disposable {
 	 * Update the layout bounds of this view
 	 */
 	layout(bounds: IBrowserViewBounds): void {
-		if (this._window?.win?.id !== bounds.windowId) {
+		if (this._currentWindow?.win?.id !== bounds.windowId) {
 			const newWindow = this._windowById(bounds.windowId);
 			if (newWindow) {
-				this._window?.win?.contentView.removeChildView(this._view);
-				this._window = newWindow;
+				this._currentWindow?.win?.contentView.removeChildView(this._view);
+				this._currentWindow = newWindow;
 				newWindow.win?.contentView.addChildView(this._view);
 			}
 		}
@@ -476,7 +495,7 @@ export class BrowserView extends Disposable {
 
 		// If the view is focused, pass focus back to the window when hiding
 		if (!visible && this._view.webContents.isFocused()) {
-			this._window?.win?.webContents.focus();
+			this._currentWindow?.win?.webContents.focus();
 		}
 
 		this._view.setVisible(visible);
@@ -596,7 +615,7 @@ export class BrowserView extends Disposable {
 	 */
 	async focus(force?: boolean): Promise<void> {
 		// By default, only focus the view if its window is already focused.
-		if (!force && !this._window?.win?.isFocused()) {
+		if (!force && !this._currentWindow?.win?.isFocused()) {
 			return;
 		}
 		this._view.webContents.focus();
@@ -676,15 +695,7 @@ export class BrowserView extends Disposable {
 	 * This can be an auxiliary window, depending on where the view is currently hosted.
 	 */
 	getElectronWindow(): Electron.BrowserWindow | undefined {
-		return this._window?.win ?? undefined;
-	}
-
-	/**
-	 * Get the main code window hosting this browser view, if any. This is used for routing commands from the browser view to the correct window.
-	 * If the browser view is hosted in an auxiliary window, this will return the parent code window of that auxiliary window.
-	 */
-	getTopCodeWindow(): ICodeWindow | undefined {
-		return this._window && hasKey(this._window, { parentId: true }) ? this._codeWindowById(this._window.parentId) : undefined;
+		return this._currentWindow?.win ?? undefined;
 	}
 
 	override dispose(): void {
@@ -697,7 +708,7 @@ export class BrowserView extends Disposable {
 		this.debugger.dispose();
 
 		// Remove from parent window
-		this._window?.win?.contentView.removeChildView(this._view);
+		this._currentWindow?.win?.contentView.removeChildView(this._view);
 
 		// Fire close event BEFORE disposing emitters. This signals the view has been destroyed.
 		this._onDidClose.fire();

src/vs/platform/browserView/electron-main/browserViewGroup.ts

@@ -202,7 +202,7 @@ export class BrowserViewGroup extends Disposable implements ICDPBrowserTarget, I
 			throw new Error(`Unknown browser context ${browserContextId}`);
 		}
 
-		const target = await this.browserViewMainService.createTarget(url, browserContextId, windowId);
+		const target = await this.browserViewMainService.createTarget(url, windowId, browserContextId);
 		if (target instanceof BrowserView) {
 			await this.addView(target.id);
 			return this.viewTargets.get(target.id)!;