fix: clear event binding function references #38

odedhutzler · 2017-12-06T14:23:39Z

Description of the Changes

Created an object to hold all the event handlers, so we can unbind it properly.
Today we don't really unbind the handler because it's a new handler and not a reference to the previous one.

CheckLists

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
test are passing in local environment
Travis tests are passing (or test results are not worse than on master branch :))
Docs have been updated

OrenMe · 2017-12-06T21:52:55Z

src/dash-adapter.js

+   * @type {Object}
+   * @private
+   */
+  _adapterEventsBindings: Object = {};


flow type {[name: string]: function}

OrenMe · 2017-12-06T21:53:20Z

src/dash-adapter.js

@@ -202,19 +210,29 @@ export default class DashAdapter extends BaseMediaSourceAdapter {
    }
  }

+
+  _createAdapterFunctionBindings(): void{
+


why empty line?

there is no spoon (empty line).

OrenMe · 2017-12-06T21:54:07Z

src/dash-adapter.js

+
+  _createAdapterFunctionBindings(): void{
+
+      this._adapterEventsBindings['error'] =  this._onError.bind(this);


can we use arrow function?

also, need to clear these bounded methods on destroy or we will have memory leaks possibly

OrenMe · 2017-12-07T15:16:47Z

src/dash-adapter.js

    //TODO use events enum when available
-    this._videoElement.addEventListener('waiting', this._onWaiting.bind(this));
-    this._videoElement.addEventListener('playing', this._onPlaying.bind(this));
+    this._videoElement.addEventListener('waiting', this._adapterEventsBindings.waiting);


Use ENUM for HTML5 events

OrenMe · 2017-12-07T15:17:09Z

src/dash-adapter.js

-    this._shaka.addEventListener('error', this._onError.bind(this));
-    this._shaka.addEventListener('buffering', this._onBuffering.bind(this));
+    this._shaka.addEventListener('adaptation', this._adapterEventsBindings.adaption);
+    this._shaka.addEventListener('error', this._adapterEventsBindings.error);


Create local ENUM for Shaka event names

OrenMe · 2017-12-07T15:47:52Z

src/dash-adapter.js

+  ERROR: 'error' ,
+  ADAPTION: 'adaption',
+  BUFFERING: 'buffering',
+  WAITING: 'waiting',


WAITING and PLAYING are HTML5 events

dan-ziv · 2017-12-10T15:08:00Z

src/dash-adapter.js

@@ -7,6 +7,20 @@ import {Error} from 'playkit-js'
 import Widevine from './drm/widevine'
 import PlayReady from './drm/playready'

+
+type ShakaEventsType = { [event: string]: string };


change to ShakaEventType

dan-ziv · 2017-12-10T15:09:20Z

src/dash-adapter.js

+ * @type {Object}
+ * @const
+ */
+const SHAKA_EVENTS: ShakaEventsType = {


Enums convention are to lead with capitals and refer as singular (i.e. ShakaEvent)

dan-ziv · 2017-12-10T15:10:14Z

src/dash-adapter.js

@@ -202,19 +224,27 @@ export default class DashAdapter extends BaseMediaSourceAdapter {
    }
  }

+
+  _createAdapterFunctionBindings(): void {


We can do this logic statically when we declare the _adapterEventsBindings variable, no need to create method for it

@dan-ziv what do you mean statically? we want to have the this bindings

_adapterEventsBindings: { [name: string]: Function } = {
[ShakaEvent.ERROR] : (event) => this._onError(event),
...
};

dan-ziv · 2017-12-10T15:11:45Z

src/dash-adapter.js

@@ -202,19 +224,27 @@ export default class DashAdapter extends BaseMediaSourceAdapter {
    }
  }

+


remove redundant lines (!!!)

cleaning code

create an object for all the binding functions references.

2adaa99

odedhutzler added the Priority: Medium label Dec 6, 2017

odedhutzler self-assigned this Dec 6, 2017

odedhutzler requested review from OrenMe, yairans and dan-ziv December 6, 2017 14:23

OrenMe suggested changes Dec 6, 2017

View reviewed changes

.bind=>arrow functions, destroy, remove empty line

5b011b6

OrenMe changed the title ~~fix: create an object for all the binding functions references.~~ fix: clear event binding functions references Dec 7, 2017

OrenMe changed the title ~~fix: clear event binding functions references~~ fix: clear event binding function references Dec 7, 2017

OrenMe suggested changes Dec 7, 2017

View reviewed changes

adding types + enum shaka events

4f667f4

OrenMe suggested changes Dec 7, 2017

View reviewed changes

adding html5 events to html5 events. and shaka events to shaka

a0630ca

OrenMe previously approved these changes Dec 7, 2017

View reviewed changes

dan-ziv suggested changes Dec 10, 2017

View reviewed changes

removing the object intilization function.

ff26649

cleaning code

odedhutzler dismissed OrenMe’s stale review via ff26649 December 11, 2017 12:19

OrenMe approved these changes Dec 11, 2017

View reviewed changes

Merge branch 'master' into event_binding_handler

cdebe37

dan-ziv approved these changes Dec 12, 2017

View reviewed changes

dan-ziv merged commit 58beee8 into master Dec 12, 2017

dan-ziv deleted the event_binding_handler branch December 12, 2017 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: clear event binding function references #38

fix: clear event binding function references #38

odedhutzler commented Dec 6, 2017 •

edited

Loading

OrenMe Dec 6, 2017

OrenMe Dec 6, 2017

odedhutzler Dec 7, 2017

OrenMe Dec 6, 2017

OrenMe Dec 6, 2017

OrenMe Dec 7, 2017

OrenMe Dec 7, 2017

OrenMe Dec 7, 2017

dan-ziv Dec 10, 2017

dan-ziv Dec 10, 2017

dan-ziv Dec 10, 2017

OrenMe Dec 10, 2017

dan-ziv Dec 10, 2017

dan-ziv Dec 10, 2017


		_createAdapterFunctionBindings(): void{

		this._adapterEventsBindings['error'] = this._onError.bind(this);

		@@ -202,19 +224,27 @@ export default class DashAdapter extends BaseMediaSourceAdapter {
		}
		}

fix: clear event binding function references #38

fix: clear event binding function references #38

Conversation

odedhutzler commented Dec 6, 2017 • edited Loading

Description of the Changes

CheckLists

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odedhutzler commented Dec 6, 2017 •

edited

Loading