feat(rpc): merge DispatcherScope and Dispatcher (#2918)

dgozman · web-flow · commit fc6861410b51 · 2020-07-10T16:24:11.000-07:00
diff --git a/src/rpc/server.ts b/src/rpc/server.ts
@@ -25,4 +25,4 @@ transport.onmessage = message => dispatcherConnection.dispatch(message);
 dispatcherConnection.onmessage = message => transport.send(message);
 
 const playwright = new Playwright(__dirname, require('../../browsers.json')['browsers']);
-new PlaywrightDispatcher(dispatcherConnection.rootScope(), playwright);
+new PlaywrightDispatcher(dispatcherConnection.rootDispatcher(), playwright);
diff --git a/src/rpc/server/browserContextDispatcher.ts b/src/rpc/server/browserContextDispatcher.ts
@@ -48,7 +48,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, Browser
     context.on(Events.BrowserContext.Page, page => this._dispatchEvent('page', new PageDispatcher(this._scope, page)));
     context.on(Events.BrowserContext.Close, () => {
       this._dispatchEvent('close');
-      this._scope.dispose();
+      this._dispose();
     });
   }
 
diff --git a/src/rpc/server/browserDispatcher.ts b/src/rpc/server/browserDispatcher.ts
@@ -30,7 +30,7 @@ export class BrowserDispatcher extends Dispatcher<Browser, BrowserInitializer> i
     super(scope, browser, 'browser', {}, true);
     browser.on(Events.Browser.Disconnected, () => {
       this._dispatchEvent('close');
-      this._scope.dispose();
+      this._dispose();
     });
   }
 
diff --git a/src/rpc/server/cdpSessionDispatcher.ts b/src/rpc/server/cdpSessionDispatcher.ts
@@ -24,7 +24,7 @@ export class CDPSessionDispatcher extends Dispatcher<CRSession, CDPSessionInitia
     crSession._eventListener = (method, params) => this._dispatchEvent('event', { method, params });
     crSession.on(CRSessionEvents.Disconnected, () => {
       this._dispatchEvent('disconnected');
-      this._scope.dispose();
+      this._dispose();
     });
   }
 
diff --git a/src/rpc/server/dispatcher.ts b/src/rpc/server/dispatcher.ts
@@ -36,99 +36,95 @@ export function lookupNullableDispatcher<DispatcherType>(object: any | null): Di
 }
 
 export class Dispatcher<Type, Initializer> extends EventEmitter implements Channel {
+  private _connection: DispatcherConnection;
+  private _isScope: boolean;
+  // Parent is always "isScope".
+  private _parent: Dispatcher<any, any> | undefined;
+  // Only "isScope" channel owners have registered dispatchers inside.
+  private _dispatchers = new Map<string, Dispatcher<any, any>>();
+
   readonly _guid: string;
   readonly _type: string;
-  protected _scope: DispatcherScope;
+  readonly _scope: Dispatcher<any, any>;
   _object: Type;
 
-  constructor(scope: DispatcherScope, object: Type, type: string, initializer: Initializer, isScope?: boolean, guid = type + '@' + helper.guid()) {
+  constructor(parent: Dispatcher<any, any> | DispatcherConnection, object: Type, type: string, initializer: Initializer, isScope?: boolean, guid = type + '@' + helper.guid()) {
     super();
+
+    this._connection = parent instanceof DispatcherConnection ? parent : parent._connection;
+    this._isScope = !!isScope;
+    this._parent = parent instanceof DispatcherConnection ? undefined : parent;
+    this._scope = isScope ? this : this._parent!;
+
+    assert(!this._connection._dispatchers.has(guid));
+    this._connection._dispatchers.set(guid, this);
+    if (this._parent) {
+      assert(!this._parent._dispatchers.has(guid));
+      this._parent._dispatchers.set(guid, this);
+    }
+
     this._type = type;
     this._guid = guid;
     this._object = object;
-    this._scope = isScope ? scope.createChild(guid) : scope;
-    scope.bind(this._guid, this);
+
     (object as any)[dispatcherSymbol] = this;
-    this._scope.sendMessageToClient(scope.guid, '__create__', { type, initializer, guid });
+    if (this._parent)
+      this._connection.sendMessageToClient(this._parent._guid, '__create__', { type, initializer, guid });
   }
 
   _dispatchEvent(method: string, params: Dispatcher<any, any> | any = {}) {
-    this._scope.sendMessageToClient(this._guid, method, params);
-  }
-}
-
-export class DispatcherScope {
-  private _connection: DispatcherConnection;
-  private _dispatchers = new Map<string, Dispatcher<any, any>>();
-  private _parent: DispatcherScope | undefined;
-  readonly _children = new Set<DispatcherScope>();
-  readonly guid: string;
-
-  constructor(connection: DispatcherConnection, guid: string, parent?: DispatcherScope) {
-    this._connection = connection;
-    this._parent = parent;
-    this.guid = guid;
-    if (parent)
-      parent._children.add(this);
-  }
-
-  createChild(guid: string): DispatcherScope {
-    return new DispatcherScope(this._connection, guid, this);
-  }
-
-  bind(guid: string, arg: Dispatcher<any, any>) {
-    assert(!this._dispatchers.has(guid));
-    this._dispatchers.set(guid, arg);
-    this._connection._dispatchers.set(guid, arg);
+    this._connection.sendMessageToClient(this._guid, method, params);
   }
 
-  dispose() {
-    // Take care of hierarchy.
-    for (const child of [...this._children])
-      child.dispose();
-    this._children.clear();
+  _dispose() {
+    assert(this._isScope);
 
-    // Delete self from scopes and objects.
-    this._connection._dispatchers.delete(this.guid);
+    // Clean up from parent and connection.
+    if (this._parent)
+      this._parent._dispatchers.delete(this._guid);
+    this._connection._dispatchers.delete(this._guid);
 
-    // Delete all of the objects from connection.
-    for (const guid of this._dispatchers.keys())
-      this._connection._dispatchers.delete(guid);
-
-    if (this._parent) {
-      this._parent._children.delete(this);
-      this._parent._dispatchers.delete(this.guid);
+    // Dispose all children.
+    for (const [guid, dispatcher] of [...this._dispatchers]) {
+      if (dispatcher._isScope)
+        dispatcher._dispose();
+      else
+        this._connection._dispatchers.delete(guid);
     }
+    this._dispatchers.clear();
   }
 
-  async sendMessageToClient(guid: string, method: string, params: any): Promise<any> {
-    this._connection._sendMessageToClient(guid, method, params);
+  _debugScopeState(): any {
+    return {
+      _guid: this._guid,
+      objects: this._isScope ? Array.from(this._dispatchers.values()).map(o => o._debugScopeState()) : undefined,
+    };
   }
+}
+
+export type DispatcherScope = Dispatcher<any, any>;
 
-  _dumpScopeState(scopes: any[]): any {
-    const scopeState: any = { _guid: this.guid };
-    scopeState.objects = [...this._dispatchers.keys()];
-    scopes.push(scopeState);
-    [...this._children].map(c => c._dumpScopeState(scopes));
-    return scopeState;
+class Root extends Dispatcher<{}, {}> {
+  constructor(connection: DispatcherConnection) {
+    super(connection, {}, '', {}, true, '');
   }
 }
 
 export class DispatcherConnection {
   readonly _dispatchers = new Map<string, Dispatcher<any, any>>();
-  private _rootScope: DispatcherScope;
+  private _rootDispatcher: Root;
   onmessage = (message: string) => {};
 
-  async _sendMessageToClient(guid: string, method: string, params: any): Promise<any> {
+  async sendMessageToClient(guid: string, method: string, params: any): Promise<any> {
     this.onmessage(JSON.stringify({ guid, method, params: this._replaceDispatchersWithGuids(params) }));
   }
 
   constructor() {
-    this._rootScope = new DispatcherScope(this, '');
+    this._rootDispatcher = new Root(this);
   }
 
-  rootScope(): DispatcherScope {
-    return this._rootScope;
+  rootDispatcher(): Dispatcher<any, any> {
+    return this._rootDispatcher;
   }
 
   async dispatch(message: string) {
@@ -140,11 +136,7 @@ export class DispatcherConnection {
       return;
     }
     if (method === 'debugScopeState') {
-      const dispatcherState: any = {};
-      dispatcherState.objects = [...this._dispatchers.keys()];
-      dispatcherState.scopes = [];
-      this._rootScope._dumpScopeState(dispatcherState.scopes);
-      this.onmessage(JSON.stringify({ id, result: dispatcherState }));
+      this.onmessage(JSON.stringify({ id, result: this._rootDispatcher._debugScopeState() }));
       return;
     }
     try {
@@ -155,7 +147,7 @@ export class DispatcherConnection {
     }
   }
 
-  _replaceDispatchersWithGuids(payload: any): any {
+  private _replaceDispatchersWithGuids(payload: any): any {
     if (!payload)
       return payload;
     if (payload instanceof Dispatcher)
diff --git a/test/channels.spec.js b/test/channels.spec.js
@@ -24,10 +24,13 @@ describe.skip(!CHANNEL)('Channels', function() {
 
   it('should scope context handles', async({browser, server}) => {
     const GOLDEN_PRECONDITION = {
-      objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ],
-      scopes: [
-        { _guid: '', objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ] },
-        { _guid: 'browser', objects: [] }
+      _guid: '',
+      objects: [
+        { _guid: 'chromium' },
+        { _guid: 'firefox' },
+        { _guid: 'webkit' },
+        { _guid: 'playwright' },
+        { _guid: 'browser', objects: [] },
       ]
     };
     await expectScopeState(browser, GOLDEN_PRECONDITION);
@@ -36,11 +39,20 @@ describe.skip(!CHANNEL)('Channels', function() {
     const page = await context.newPage();
     await page.goto(server.EMPTY_PAGE);
     await expectScopeState(browser, {
-      objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser', 'context', 'frame', 'page', 'request', 'response' ],
-      scopes: [
-        { _guid: '', objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ] },
-        { _guid: 'browser', objects: ['context'] },
-        { _guid: 'context', objects: ['frame', 'page', 'request', 'response'] }
+      _guid: '',
+      objects: [
+        { _guid: 'chromium' },
+        { _guid: 'firefox' },
+        { _guid: 'webkit' },
+        { _guid: 'playwright' },
+        { _guid: 'browser', objects: [
+          { _guid: 'context', objects: [
+            { _guid: 'frame' },
+            { _guid: 'page' },
+            { _guid: 'request' },
+            { _guid: 'response' },
+          ]},
+        ] },
       ]
     });
 
@@ -50,21 +62,28 @@ describe.skip(!CHANNEL)('Channels', function() {
 
   it.skip(!CHROMIUM)('should scope CDPSession handles', async({browserType, browser, server}) => {
     const GOLDEN_PRECONDITION = {
-      objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ],
-      scopes: [
-        { _guid: '', objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ] },
-        { _guid: 'browser', objects: [] }
+      _guid: '',
+      objects: [
+        { _guid: 'chromium' },
+        { _guid: 'firefox' },
+        { _guid: 'webkit' },
+        { _guid: 'playwright' },
+        { _guid: 'browser', objects: [] },
       ]
     };
     await expectScopeState(browserType, GOLDEN_PRECONDITION);
 
     const session = await browser.newBrowserCDPSession();
     await expectScopeState(browserType, {
-      objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser', 'cdpSession' ],
-      scopes: [
-        { _guid: '', objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ] },
-        { _guid: 'browser', objects: ['cdpSession'] },
-        { _guid: 'cdpSession', objects: [] },
+      _guid: '',
+      objects: [
+        { _guid: 'chromium' },
+        { _guid: 'firefox' },
+        { _guid: 'webkit' },
+        { _guid: 'playwright' },
+        { _guid: 'browser', objects: [
+          { _guid: 'cdpSession', objects: [] },
+        ] },
       ]
     });
 
@@ -74,23 +93,30 @@ describe.skip(!CHANNEL)('Channels', function() {
 
   it('should scope browser handles', async({browserType, defaultBrowserOptions}) => {
     const GOLDEN_PRECONDITION = {
-      objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ],
-      scopes: [
-        { _guid: '', objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser' ] },
-        { _guid: 'browser', objects: [] }
+      _guid: '',
+      objects: [
+        { _guid: 'chromium' },
+        { _guid: 'firefox' },
+        { _guid: 'webkit' },
+        { _guid: 'playwright' },
+        { _guid: 'browser', objects: [] },
       ]
     };
     await expectScopeState(browserType, GOLDEN_PRECONDITION);
 
     const browser = await browserType.launch(defaultBrowserOptions);
     await browser.newContext();
     await expectScopeState(browserType, {
-      objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser', 'browser', 'context' ],
-      scopes: [
-        { _guid: '', objects: [ 'chromium', 'firefox', 'webkit', 'playwright', 'browser', 'browser' ] },
+      _guid: '',
+      objects: [
+        { _guid: 'chromium' },
+        { _guid: 'firefox' },
+        { _guid: 'webkit' },
+        { _guid: 'playwright' },
+        { _guid: 'browser', objects: [
+          { _guid: 'context', objects: [] },
+        ] },
         { _guid: 'browser', objects: [] },
-        { _guid: 'browser', objects: ['context'] },
-        { _guid: 'context', objects: [] },
       ]
     });
 
@@ -100,15 +126,28 @@ describe.skip(!CHANNEL)('Channels', function() {
 });
 
 async function expectScopeState(object, golden) {
+  golden = trimGuids(golden);
   const remoteState = trimGuids(await object._channel.debugScopeState());
   const localState = trimGuids(object._connection._debugScopeState());
-  expect(processLocalState(localState)).toEqual(golden);
+  expect(localState).toEqual(golden);
   expect(remoteState).toEqual(golden);
 }
 
+function compareObjects(a, b) {
+  if (a._guid !== b._guid)
+    return a._guid.localeCompare(b._guid);
+  if (a.objects && !b.objects)
+    return -1;
+  if (!a.objects && b.objects)
+    return 1;
+  if (!a.objects && !b.objects)
+    return 0;
+  return a.objects.length - b.objects.length;
+}
+
 function trimGuids(object) {
   if (Array.isArray(object))
-    return object.map(trimGuids);
+    return object.map(trimGuids).sort(compareObjects);
   if (typeof object === 'object') {
     const result = {};
     for (const key in object)
@@ -119,21 +158,3 @@ function trimGuids(object) {
     return object ? object.match(/[^@]+/)[0] : '';
   return object;
 }
-
-function processLocalState(root) {
-  const objects = [];
-  const scopes = [];
-  const visit = (object, scope) => {
-    if (object._guid !== '')
-      objects.push(object._guid);
-    scope.push(object._guid);
-    if (object.objects) {
-      scope = [];
-      scopes.push({ _guid: object._guid, objects: scope });
-      for (const child of object.objects)
-        visit(child, scope);
-    }
-  };
-  visit(root, []);
-  return { objects, scopes };
-}
diff --git a/test/electron/electron.spec.js b/test/electron/electron.spec.js
diff --git a/test/environments.js b/test/environments.js

Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, Browser`
`48`	`48`	`context.on(Events.BrowserContext.Page, page => this._dispatchEvent('page', new PageDispatcher(this._scope, page)));`
`49`	`49`	`context.on(Events.BrowserContext.Close, () => {`
`50`	`50`	`this._dispatchEvent('close');`
`51`		`- this._scope.dispose();`
	`51`	`+ this._dispose();`
`52`	`52`	`});`
`53`	`53`	`}`
`54`	`54`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ export class BrowserDispatcher extends Dispatcher<Browser, BrowserInitializer> i`
`30`	`30`	`super(scope, browser, 'browser', {}, true);`
`31`	`31`	`browser.on(Events.Browser.Disconnected, () => {`
`32`	`32`	`this._dispatchEvent('close');`
`33`		`- this._scope.dispose();`
	`33`	`+ this._dispose();`
`34`	`34`	`});`
`35`	`35`	`}`
`36`	`36`
Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ export class CDPSessionDispatcher extends Dispatcher<CRSession, CDPSessionInitia`
`24`	`24`	`crSession._eventListener = (method, params) => this._dispatchEvent('event', { method, params });`
`25`	`25`	`crSession.on(CRSessionEvents.Disconnected, () => {`
`26`	`26`	`this._dispatchEvent('disconnected');`
`27`		`- this._scope.dispose();`
	`27`	`+ this._dispose();`
`28`	`28`	`});`
`29`	`29`	`}`
`30`	`30`