fix(FEC-11037): multiple decorator exist after destroy plugin with de…

…corator (#544)

Issue: when the plugin has the decorator, every re-create of player will register again to static that contains the decorator and multiple instances will be created for next time.
Solution: change the decorator registration from static to instance to make sure every player has his own decorator manager.

flow-typed/interfaces/engine-decorator-provider.js → flow-typed/types/engine-decorator-provider.js

@@ -3,4 +3,5 @@ import type {IEngine} from './engine';
 
 declare interface IEngineDecoratorProvider {
   getEngineDecorator(engine: IEngine, dispatchEventHandler: Function): IEngineDecorator;
+  getName(): string;
 }

src/engines/engine-decorator-manager.js

@@ -0,0 +1,33 @@
+// @flow
+import getLogger from '../utils/logger';
+
+/**
+ * Engine decorator manager for plugins.
+ * @class EngineDecoratorManager
+ */
+class EngineDecoratorManager {
+  _decoratorProviders: Map<string, IEngineDecoratorProvider> = new Map();
+  _logger: any = getLogger('EngineDecoratorManager');
+
+  register(engineDecoratorProvider: IEngineDecoratorProvider): void {
+    if (!this._decoratorProviders.has(engineDecoratorProvider.getName())) {
+      this._decoratorProviders.set(engineDecoratorProvider.getName(), engineDecoratorProvider);
+    } else {
+      this._logger.warn(`decorator already registered for ${engineDecoratorProvider.getName()}`);
+    }
+  }
+
+  createDecorators(engine: IEngine, dispatchEvent: Function): Array<IEngineDecorator> {
+    this._logger.debug(`decorators created for ${Array.from(this._decoratorProviders.keys()).toString()}`);
+    return Array.from(this._decoratorProviders.values(), engineDecoratorProvider =>
+      engineDecoratorProvider.getEngineDecorator(engine, dispatchEvent)
+    );
+  }
+
+  destroy(): void {
+    this._logger.debug(`decorators destroyed`);
+    this._decoratorProviders.clear();
+  }
+}
+
+export {EngineDecoratorManager};

src/engines/engine-decorator-provider.js

@@ -0,0 +1,27 @@
+// @flow
+
+/**
+ * Engine decorator provider.
+ * @class EngineDecoratorProvider
+ * @param {IEngineDecoratorProvider} plugin - The plugin which have implemented decorator.
+ * @implements {IEngineDecorator}
+ */
+class EngineDecoratorProvider implements IEngineDecoratorProvider {
+  _name: string;
+  _getEngineDecorator: (engine: IEngine, dispatchEventHandler: Function) => IEngineDecorator;
+
+  constructor(plugin: IEngineDecoratorProvider) {
+    this._name = plugin.getName();
+    this._getEngineDecorator = plugin.getEngineDecorator.bind(plugin);
+  }
+
+  getEngineDecorator(engine: IEngine, dispatchEventHandler: Function): IEngineDecorator {
+    return this._getEngineDecorator(engine, dispatchEventHandler);
+  }
+
+  getName(): string {
+    return this._name;
+  }
+}
+
+export {EngineDecoratorProvider};

src/engines/engine-decorator.js

@@ -3,6 +3,7 @@ import FakeEvent from '../event/fake-event';
 import {EventType} from '../event/event-type';
 import EventManager from '../event/event-manager';
 import FakeEventTarget from '../event/fake-event-target';
+import {EngineDecoratorManager} from './engine-decorator-manager';
 
 /**
  * Engine decorator for plugin.
@@ -11,26 +12,13 @@ import FakeEventTarget from '../event/fake-event-target';
  * @implements {IEngineDecorator}
  */
 class EngineDecorator extends FakeEventTarget implements IEngineDecorator {
-  static _decoratorProviders: Array<IEngineDecoratorProvider> = [];
   _pluginDecorators: Array<IEngineDecorator>;
   _eventManager: EventManager;
 
-  static register(decoratorProvider: IEngineDecoratorProvider): void {
-    if (decoratorProvider) {
-      if (!EngineDecorator._decoratorProviders.includes(decoratorProvider)) {
-        EngineDecorator._decoratorProviders.push(decoratorProvider);
-      }
-    }
-  }
-
-  static getDecorator(engine: IEngine): ?IEngine {
-    return EngineDecorator._decoratorProviders.length ? new this(engine) : null;
-  }
-
-  constructor(engine: IEngine) {
+  constructor(engine: IEngine, decoratorManager: EngineDecoratorManager) {
     super();
     this._eventManager = new EventManager();
-    this._pluginDecorators = EngineDecorator._decoratorProviders.map(provider => provider.getEngineDecorator(engine, super.dispatchEvent.bind(this)));
+    this._pluginDecorators = decoratorManager.createDecorators(engine, super.dispatchEvent.bind(this));
     const events: Array<string> = (Object.values(EventType): any);
     events.forEach(event => this._eventManager.listen(engine, event, (e: FakeEvent) => this.dispatchEvent(e)));
     return new Proxy(engine, {
@@ -69,5 +57,4 @@ class EngineDecorator extends FakeEventTarget implements IEngineDecorator {
   }
 }
 
-const registerEngineDecoratorProvider = EngineDecorator.register;
-export {EngineDecorator, registerEngineDecoratorProvider};
+export {EngineDecorator};

src/player.js

@@ -38,6 +38,7 @@ import {LabelOptions} from './track/label-options';
 import {AutoPlayType} from './enums/auto-play-type';
 import ImageTrack from './track/image-track';
 import {ThumbnailInfo} from './thumbnail/thumbnail-info';
+import {EngineDecoratorManager} from './engines/engine-decorator-manager';
 
 /**
  * The black cover class name.
@@ -390,6 +391,12 @@ export default class Player extends FakeEventTarget {
    * @private
    */
   _aspectRatio: ?string;
+  /**
+   * The engine decorator manager.
+   * @type {?EngineDecoratorManager}
+   * @private
+   */
+  _engineDecoratorManager: ?EngineDecoratorManager;
 
   /**
    * @param {Object} config - The configuration for the player instance.
@@ -627,6 +634,9 @@ export default class Player extends FakeEventTarget {
     if (this._engine) {
       this._engine.destroy();
     }
+    if (this._engineDecoratorManager) {
+      this._engineDecoratorManager.destroy();
+    }
     this._resizeWatcher.destroy();
     if (this._el) {
       Utils.Dom.removeChild(this._el.parentNode, this._el);
@@ -690,6 +700,21 @@ export default class Player extends FakeEventTarget {
     }
   }
 
+  /**
+   * detach the engine's media source
+   * @public
+   * @returns {void}
+   * @param {IEngineDecoratorProvider} engineDecoratorProvider - function to create the decorator
+   */
+  registerEngineDecoratorProvider(engineDecoratorProvider: IEngineDecoratorProvider): void {
+    if (!this._engineDecoratorManager) {
+      this._engineDecoratorManager = new EngineDecoratorManager();
+    }
+    if (engineDecoratorProvider) {
+      this._engineDecoratorManager.register(engineDecoratorProvider);
+    }
+  }
+
   /**
    * Get the first buffered range of the engine.
    * @returns {TimeRanges} - First buffered range of the engine in seconds.
@@ -1694,7 +1719,7 @@ export default class Player extends FakeEventTarget {
    */
   _createEngine(Engine: IEngineStatic, source: PKMediaSourceObject): void {
     const engine = Engine.createEngine(source, this._config, this._playerId);
-    this._engine = EngineDecorator.getDecorator(engine) || engine;
+    this._engine = this._engineDecoratorManager ? new EngineDecorator(engine, this._engineDecoratorManager) : engine;
   }
 
   /**

src/playkit.js

@@ -2,7 +2,7 @@
 import Player from './player';
 import BaseMediaSourceAdapter from './engines/html5/media-source/base-media-source-adapter';
 import {registerMediaSourceAdapter} from './engines/html5/media-source/media-source-provider';
-import {registerEngineDecoratorProvider} from './engines/engine-decorator';
+import {EngineDecoratorProvider} from './engines/engine-decorator-provider';
 import {registerEngine, unRegisterEngine} from './engines/engine-provider';
 import BaseMiddleware from './middleware/base-middleware';
 import State from './state/state';
@@ -54,9 +54,6 @@ export function loadPlayer(config: ?Object) {
 // Export the media source adapters necessary utils
 export {registerMediaSourceAdapter, BaseMediaSourceAdapter};
 
-// Export the engine decorator provider register method
-export {registerEngineDecoratorProvider};
-
 // Export the middleware framework
 export {BaseMiddleware};
 
@@ -90,6 +87,9 @@ const setCapabilities = Player.setCapabilities;
 // Export capabilities utils
 export {getCapabilities, setCapabilities};
 
+// Export engineDecoratorProvider
+export {EngineDecoratorProvider};
+
 // Export engine framework
 export {registerEngine, unRegisterEngine};
 

test/src/engines/engine-deorator.spec.js

@@ -0,0 +1,94 @@
+import {FakeDecoratorProvider, FakeDecoratorProviderActive, FakeHTML5Engine} from './test-engine-decorator-providers';
+import {EngineDecorator} from '../../../src/engines/engine-decorator';
+import Player from '../../../src/player';
+import {createElement, getConfigStructure} from '../utils/test-utils';
+import {EngineDecoratorManager} from '../../../src/engines/engine-decorator-manager';
+
+describe('EngineDecorator', () => {
+  let engine;
+  beforeEach(() => {
+    engine = new FakeHTML5Engine();
+  });
+  afterEach(() => {
+    engine = null;
+  });
+
+  it('should decorator be able to register once for same plugin name', () => {
+    const decoratorManager = new EngineDecoratorManager();
+    decoratorManager.register(FakeDecoratorProviderActive);
+    decoratorManager.register(FakeDecoratorProviderActive);
+    decoratorManager.register(FakeDecoratorProviderActive);
+    decoratorManager.createDecorators(null, null).length.should.equal(1);
+  });
+
+  it('should decorator use the engine for non implemented methods on active decorator', () => {
+    const decoratorManager = new EngineDecoratorManager();
+    decoratorManager.register(FakeDecoratorProviderActive);
+    const engineDecorator = new EngineDecorator(engine, decoratorManager);
+    engineDecorator.isAdaptiveBitrateEnabled().should.be.true;
+  });
+
+  it('should decorator use the engine for implemented methods on non active decorator', () => {
+    const decoratorManager = new EngineDecoratorManager();
+    decoratorManager.register(FakeDecoratorProvider);
+    const engineDecorator = new EngineDecorator(engine, decoratorManager);
+    engineDecorator.isLive().should.be.false;
+  });
+
+  it('should decorator use the decorator for implemented methods on active decorator', () => {
+    let engineDecorator, decoratorManager;
+    decoratorManager = new EngineDecoratorManager();
+    decoratorManager.register(FakeDecoratorProviderActive);
+    engineDecorator = new EngineDecorator(engine, decoratorManager);
+    engineDecorator.isLive().should.be.true;
+
+    decoratorManager = new EngineDecoratorManager();
+    decoratorManager.register(FakeDecoratorProvider);
+    engineDecorator = new EngineDecorator(engine, decoratorManager);
+    engineDecorator.isLive().should.be.false;
+  });
+
+  it('should decorator providers should destroy on destroy', () => {
+    const targetId = 'player-placeholder_engine-decorator.spec';
+    const playerContainer = createElement('DIV', targetId);
+
+    const player = new Player(getConfigStructure());
+    player.registerEngineDecoratorProvider(FakeDecoratorProviderActive);
+    player.registerEngineDecoratorProvider(FakeDecoratorProvider);
+    playerContainer.appendChild(player.getView());
+
+    player.destroy();
+    player._engineDecoratorManager.createDecorators(null, null).length.should.equal(0);
+  });
+
+  it.skip('should decorator use the decorator instance as context of the function', done => {
+    const decoratorManager = new EngineDecoratorManager();
+    const decoratorProvider = FakeDecoratorProviderActive;
+    decoratorManager.register(decoratorProvider);
+    const engineDecorator = new EngineDecorator(engine, decoratorManager);
+    const loadPromise = engineDecorator.load();
+    loadPromise.then(context => {
+      try {
+        (context === decoratorProvider.getEngineDecorator()).should.be.true;
+        done();
+      } catch (e) {
+        done(e);
+      }
+    });
+  });
+
+  it.skip('should decorator use the engine when decorator not active', done => {
+    const decoratorManager = new EngineDecoratorManager();
+    decoratorManager.register(FakeDecoratorProvider);
+    const engineDecorator = new EngineDecorator(engine, decoratorManager);
+    const loadPromise = engineDecorator.load();
+    loadPromise.then(context => {
+      try {
+        (context === engine).should.be.true;
+        done();
+      } catch (e) {
+        done(e);
+      }
+    });
+  });
+});

test/src/engines/test-engine-decorator-providers.js

@@ -0,0 +1,61 @@
+import type {IEngine, IEngineDecorator} from '../../../flow-typed/interfaces/engine';
+import FakeEventTarget from '../../../src/event/fake-event-target';
+
+class FakeHTML5Engine extends FakeEventTarget implements IEngine {
+  constructor() {
+    super();
+  }
+  load(): Promise<*> {
+    return Promise.resolve(this);
+  }
+  isAdaptiveBitrateEnabled() {
+    return true;
+  }
+
+  isLive(): boolean {
+    return false;
+  }
+
+  destroy() {}
+}
+
+const FakeDecoratorProvider = {
+  getEngineDecorator: () => {
+    return new (class EngineDecorator implements IEngineDecorator {
+      constructor() {}
+
+      get active(): boolean {
+        return false;
+      }
+    })();
+  },
+  getName: () => {
+    return 'fake';
+  }
+};
+
+const FakeDecoratorProviderActive = {
+  _decorator: new (class EngineDecorator implements IEngineDecorator {
+    constructor() {}
+
+    load(): Promise<*> {
+      return Promise.resolve(this);
+    }
+
+    isLive(): boolean {
+      return true;
+    }
+
+    get active(): boolean {
+      return true;
+    }
+  })(),
+  getEngineDecorator: () => {
+    return FakeDecoratorProviderActive._decorator;
+  },
+  getName: () => {
+    return 'fakeActive';
+  }
+};
+
+export {FakeDecoratorProvider, FakeDecoratorProviderActive, FakeHTML5Engine};