chore: remove various watchers, use FrameTask directly (#1460)

src/firefox/ffPage.ts

@@ -157,8 +157,8 @@ export class FFPage implements PageDelegate {
 
   _onNavigationAborted(params: Protocol.Page.navigationAbortedPayload) {
     const frame = this._page._frameManager.frame(params.frameId)!;
-    for (const watcher of frame._documentWatchers)
+    for (const task of frame._frameTasks)
-      watcher(params.navigationId, new Error(params.errorText));
+      task.onNewDocument(params.navigationId, new Error(params.errorText));
   }
 
   _onNavigationCommitted(params: Protocol.Page.navigationCommittedPayload) {

src/frames.ts

@@ -47,7 +47,6 @@ export class FrameManager {
   private _page: Page;
   private _frames = new Map<string, Frame>();
   private _mainFrame: Frame;
-  readonly _lifecycleWatchers = new Set<() => void>();
   readonly _consoleMessageTags = new Map<string, ConsoleTagHandler>();
   private _pendingNavigationBarriers = new Set<PendingNavigationBarrier>();
 
@@ -141,8 +140,8 @@ export class FrameManager {
     frame._url = url;
     frame._name = name;
     frame._lastDocumentId = documentId;
-    for (const watcher of frame._documentWatchers)
+    for (const task of frame._frameTasks)
-      watcher(documentId);
+      task.onNewDocument(documentId);
     this.clearFrameLifecycle(frame);
     if (!initial)
       this._page.emit(Events.Page.FrameNavigated, frame);
@@ -153,8 +152,8 @@ export class FrameManager {
     if (!frame)
       return;
     frame._url = url;
-    for (const watcher of frame._sameDocumentNavigationWatchers)
+    for (const task of frame._frameTasks)
-      watcher();
+      task.onSameDocument();
     this._page.emit(Events.Page.FrameNavigated, frame);
   }
 
@@ -172,8 +171,7 @@ export class FrameManager {
     const hasLoad = frame._firedLifecycleEvents.has('load');
     frame._firedLifecycleEvents.add('domcontentloaded');
     frame._firedLifecycleEvents.add('load');
-    for (const watcher of this._lifecycleWatchers)
+    this._notifyLifecycle(frame);
-      watcher();
     if (frame === this.mainFrame() && !hasDOMContentLoaded)
       this._page.emit(Events.Page.DOMContentLoaded);
     if (frame === this.mainFrame() && !hasLoad)
@@ -185,8 +183,7 @@ export class FrameManager {
     if (!frame)
       return;
     frame._firedLifecycleEvents.add(event);
-    for (const watcher of this._lifecycleWatchers)
+    this._notifyLifecycle(frame);
-      watcher();
     if (frame === this._mainFrame && event === 'load')
       this._page.emit(Events.Page.Load);
     if (frame === this._mainFrame && event === 'domcontentloaded')
@@ -207,8 +204,8 @@ export class FrameManager {
 
   requestStarted(request: network.Request) {
     this._inflightRequestStarted(request);
-    for (const watcher of request.frame()._requestWatchers)
+    for (const task of request.frame()._frameTasks)
-      watcher(request);
+      task.onRequest(request);
     if (!request._isFavicon)
       this._page._requestStarted(request);
   }
@@ -232,8 +229,8 @@ export class FrameManager {
         let errorText = request.failure()!.errorText;
         if (canceled)
           errorText += '; maybe frame was detached?';
-        for (const watcher of request.frame()._documentWatchers)
+        for (const task of request.frame()._frameTasks)
-          watcher(request._documentId, new Error(errorText));
+          task.onNewDocument(request._documentId, new Error(errorText));
       }
     }
     if (!request._isFavicon)
@@ -241,8 +238,15 @@ export class FrameManager {
   }
 
   provisionalLoadFailed(frame: Frame, documentId: string, error: string) {
-    for (const watcher of frame._documentWatchers)
+    for (const task of frame._frameTasks)
-      watcher(documentId, new Error(error));
+      task.onNewDocument(documentId, new Error(error));
+  }
+
+  private _notifyLifecycle(frame: Frame) {
+    for (let parent: Frame | null = frame; parent; parent = parent.parentFrame()) {
+      for (const frameTask of parent._frameTasks)
+        frameTask.onLifecycle();
+    }
   }
 
   private _removeFramesRecursively(frame: Frame) {
@@ -310,9 +314,7 @@ export class Frame {
   _id: string;
   readonly _firedLifecycleEvents: Set<types.LifecycleEvent>;
   _lastDocumentId = '';
-  _requestWatchers = new Set<(request: network.Request) => void>();
+  _frameTasks = new Set<FrameTask>();
-  _documentWatchers = new Set<(documentId: string, error?: Error) => void>();
-  _sameDocumentNavigationWatchers = new Set<() => void>();
   readonly _page: Page;
   private _parentFrame: Frame | null;
   _url = '';
@@ -983,9 +985,13 @@ export class FrameTask {
   private _frame: Frame;
   private _failurePromise: Promise<Error>;
   private _requestMap = new Map<string, network.Request>();
-  private _disposables: (() => void)[] = [];
+  private _timer?: NodeJS.Timer;
   private _url: string | undefined;
 
+  onNewDocument: (documentId: string, error?: Error) => void = () => {};
+  onSameDocument = () => {};
+  onLifecycle = () => {};
+
   constructor(frame: Frame, options: types.TimeoutOptions, url?: string) {
     this._frame = frame;
     this._url = url;
@@ -995,10 +1001,8 @@ export class FrameTask {
     const { timeout = frame._page._timeoutSettings.navigationTimeout() } = options;
     if (timeout) {
       const errorMessage = 'Navigation timeout of ' + timeout + ' ms exceeded';
-      let timer: NodeJS.Timer;
+      timeoutPromise = new Promise(fulfill => this._timer = setTimeout(fulfill, timeout))
-      timeoutPromise = new Promise(fulfill => timer = setTimeout(fulfill, timeout))
           .then(() => { throw new TimeoutError(errorMessage); });
-      this._disposables.push(() => clearTimeout(timer));
     }
 
     // Process detached frames
@@ -1008,14 +1012,13 @@ export class FrameTask {
       this._frame._detachedPromise.then(() => { throw new Error('Navigating frame was detached!'); }),
     ]);
 
-    // Collect requests during the task.
+    frame._frameTasks.add(this);
-    const watcher = (request: network.Request) => {
+  }
-      if (!request._documentId || request.redirectedFrom())
+
-        return;
+  onRequest(request: network.Request) {
-      this._requestMap.set(request._documentId, request);
+    if (!request._documentId || request.redirectedFrom())
-    };
+      return;
-    this._disposables.push(() => this._frame._requestWatchers.delete(watcher));
+    this._requestMap.set(request._documentId, request);
-    this._frame._requestWatchers.add(watcher);
   }
 
   async raceAgainstFailures<T>(promise: Promise<T>): Promise<T> {
@@ -1034,74 +1037,58 @@ export class FrameTask {
     throw error;
   }
 
-  request(id: string): network.Request | undefined {
+  request(documentId: string): network.Request | undefined {
-    return this._requestMap.get(id);
+    return this._requestMap.get(documentId);
   }
 
-  async waitForSameDocumentNavigation(url?: types.URLMatch): Promise<void> {
+  waitForSameDocumentNavigation(url?: types.URLMatch): Promise<void> {
-    let resolve: () => void;
+    return this.raceAgainstFailures(new Promise((resolve, reject) => {
-    const promise = new Promise<void>(x => resolve = x);
+      this.onSameDocument = () => {
-    const watch = () => {
+        if (helper.urlMatches(this._frame.url(), url))
-      if (helper.urlMatches(this._frame.url(), url))
-        resolve();
-    };
-    this._disposables.push(() => this._frame._sameDocumentNavigationWatchers.delete(watch));
-    this._frame._sameDocumentNavigationWatchers.add(watch);
-    return this.raceAgainstFailures(promise);
-  }
-
-  async waitForSpecificDocument(expectedDocumentId: string): Promise<void> {
-    let resolve: () => void;
-    let reject: (error: Error) => void;
-    const promise = new Promise((res, rej) => { resolve = res; reject = rej; });
-    const watch = (documentId: string, error?: Error) => {
-      if (documentId === expectedDocumentId) {
-        if (!error)
           resolve();
-        else
+      };
-          reject(error);
+    }));
-      } else if (!error) {
+  }
-        reject(new Error('Navigation interrupted by another one'));
+
-      }
+  waitForSpecificDocument(expectedDocumentId: string): Promise<void> {
-    };
+    return this.raceAgainstFailures(new Promise((resolve, reject) => {
-    this._disposables.push(() => this._frame._documentWatchers.delete(watch));
+      this.onNewDocument = (documentId: string, error?: Error) => {
-    this._frame._documentWatchers.add(watch);
+        if (documentId === expectedDocumentId) {
-    await this.raceAgainstFailures(promise);
+          if (!error)
+            resolve();
+          else
+            reject(error);
+        } else if (!error) {
+          reject(new Error('Navigation interrupted by another one'));
+        }
+      };
+    }));
   }
 
   waitForNewDocument(url?: types.URLMatch): Promise<string> {
-    let resolve: (documentId: string) => void;
+    return this.raceAgainstFailures(new Promise((resolve, reject) => {
-    let reject: (error: Error) => void;
+      this.onNewDocument = (documentId: string, error?: Error) => {
-    const promise = new Promise<string>((res, rej) => { resolve = res; reject = rej; });
+        if (!error && !helper.urlMatches(this._frame.url(), url))
-    const watch = (documentId: string, error?: Error) => {
+          return;
-      if (!error && !helper.urlMatches(this._frame.url(), url))
+        if (error)
-        return;
+          reject(error);
-      if (error)
+        else
-        reject(error);
+          resolve(documentId);
-      else
+      };
-        resolve(documentId);
+    }));
-    };
-    this._disposables.push(() => this._frame._documentWatchers.delete(watch));
-    this._frame._documentWatchers.add(watch);
-    return this.raceAgainstFailures(promise);
   }
 
   waitForLifecycle(waitUntil: types.LifecycleEvent = 'load'): Promise<void> {
-    let resolve: () => void;
     if (!types.kLifecycleEvents.has(waitUntil))
       throw new Error(`Unsupported waitUntil option ${String(waitUntil)}`);
-
+    return this.raceAgainstFailures(new Promise((resolve, reject) => {
-    const checkLifecycleComplete = () => {
+      this.onLifecycle = () => {
-      if (!checkLifecycleRecursively(this._frame))
+        if (!checkLifecycleRecursively(this._frame))
-        return;
+          return;
-      resolve();
+        resolve();
-    };
+      };
-
+      this.onLifecycle();
-    const promise = new Promise<void>(x => resolve = x);
+    }));
-    this._disposables.push(() => this._frame._page._frameManager._lifecycleWatchers.delete(checkLifecycleComplete));
-    this._frame._page._frameManager._lifecycleWatchers.add(checkLifecycleComplete);
-    checkLifecycleComplete();
-    return this.raceAgainstFailures(promise);
 
     function checkLifecycleRecursively(frame: Frame): boolean {
       if (!frame._firedLifecycleEvents.has(waitUntil))
@@ -1115,9 +1102,9 @@ export class FrameTask {
   }
 
   done() {
+    this._frame._frameTasks.delete(this);
+    if (this._timer)
+      clearTimeout(this._timer);
     this._failurePromise.catch(e => {});
-    for (const disposable of this._disposables)
-      disposable();
-    this._disposables = [];
   }
 }

test/geolocation.spec.js

@@ -119,6 +119,7 @@ module.exports.describe = function ({ testRunner, expect, FFOX, WEBKIT }) {
         page.waitForEvent('popup'),
         page.evaluate(url => window._popup = window.open(url), server.PREFIX + '/geolocation.html'),
       ]);
+      await popup.waitForLoadState();
       const geolocation = await popup.evaluate(() => window.geolocationPromise);
       expect(geolocation).toEqual({ longitude: 10, latitude: 10 });
     });