From 96f64c992e01c66a24fcc338df97e39ee5876499 Mon Sep 17 00:00:00 2001
From: brandonocasey <brandonocasey@gmail.com>
Date: Thu, 18 Jul 2019 13:43:54 -0400
Subject: [PATCH 1/6] perf: Use weakmap for dom data

---
 src/js/utils/dom-data.js              | 51 ++++-----------------------
 test/unit/utils/dom-data.test.js      | 40 ---------------------
 test/unit/videojs-integration.test.js | 44 -----------------------
 3 files changed, 7 insertions(+), 128 deletions(-)

diff --git a/src/js/utils/dom-data.js b/src/js/utils/dom-data.js
index de39dae602..a5eb2f8999 100644
--- a/src/js/utils/dom-data.js
+++ b/src/js/utils/dom-data.js
@@ -2,8 +2,6 @@
  * @file dom-data.js
  * @module dom-data
  */
-import * as Guid from './guid.js';
-import window from 'global/window';
 
 /**
  * Element Data Store.
@@ -15,16 +13,7 @@ import window from 'global/window';
  * @type {Object}
  * @private
  */
-export const elData = {};
-
-/*
- * Unique attribute name to store an element's guid in
- *
- * @type {String}
- * @constant
- * @private
- */
-const elIdAttr = 'vdata' + Math.floor(window.performance && window.performance.now() || Date.now());
+export const elData = new WeakMap();
 
 /**
  * Returns the cache object where data for an element is stored
@@ -36,17 +25,11 @@ const elIdAttr = 'vdata' + Math.floor(window.performance && window.performance.n
  *         The cache object for that el that was passed in.
  */
 export function getData(el) {
-  let id = el[elIdAttr];
-
-  if (!id) {
-    id = el[elIdAttr] = Guid.newGUID();
-  }
-
-  if (!elData[id]) {
-    elData[id] = {};
+  if (!elData.has(el)) {
+    elData.set(el, {});
   }
 
-  return elData[id];
+  return elData.get(el);
 }
 
 /**
@@ -60,13 +43,11 @@ export function getData(el) {
  *         - False otherwise.
  */
 export function hasData(el) {
-  const id = el[elIdAttr];
-
-  if (!id) {
+  if (!elData.has(el)) {
     return false;
   }
 
-  return !!Object.getOwnPropertyNames(elData[id]).length;
+  return !!Object.getOwnPropertyNames(elData.get(el)).length;
 }
 
 /**
@@ -76,24 +57,6 @@ export function hasData(el) {
  *        Remove cached data for this element.
  */
 export function removeData(el) {
-  const id = el[elIdAttr];
-
-  if (!id) {
-    return;
-  }
-
   // Remove all stored data
-  delete elData[id];
-
-  // Remove the elIdAttr property from the DOM node
-  try {
-    delete el[elIdAttr];
-  } catch (e) {
-    if (el.removeAttribute) {
-      el.removeAttribute(elIdAttr);
-    } else {
-      // IE doesn't appear to support removeAttribute on the document element
-      el[elIdAttr] = null;
-    }
-  }
+  delete elData.delete(el);
 }
diff --git a/test/unit/utils/dom-data.test.js b/test/unit/utils/dom-data.test.js
index e1a0d3e95f..4077006e98 100644
--- a/test/unit/utils/dom-data.test.js
+++ b/test/unit/utils/dom-data.test.js
@@ -1,8 +1,6 @@
 /* eslint-env qunit */
 import document from 'global/document';
 import * as DomData from '../../../src/js/utils/dom-data';
-import videojs from '../../../src/js/video.js';
-import window from 'global/window';
 
 QUnit.module('dom-data');
 
@@ -23,41 +21,3 @@ QUnit.test('should get and remove data from an element', function(assert) {
 
   assert.notOk(DomData.hasData(el), 'cached item emptied');
 });
-
-let memoryTestRun = false;
-
-QUnit.done(function(details) {
-  // don't run the extra dom data test on failures, there will likely be
-  // memory leaks
-  if (details.failed || memoryTestRun) {
-    return;
-  }
-
-  memoryTestRun = true;
-
-  QUnit.module('dom-data memory');
-
-  /**
-   * If this test fails you will want to add a debug statement
-   * in DomData.getData with the `id`. For instance if DomData.elData
-   * had 2 objects in it {5: {...}, 2003: {...} you would add:
-   *
-   * ```js
-   * if (id === 5) {
-   *   debugger;
-   * }
-   * ```
-   * to the tests to see what test. Then re-run the tests to see
-   * what leaking and where.
-   *
-   * > Note that the id can be off by 1-2 in either direction
-   *   for larger guids, so you may have to account for that.
-   */
-  QUnit.test('Memory is not leaking', function(assert) {
-    if (Object.keys(DomData.elData).length > 0) {
-      videojs.domData = DomData;
-      window.videojs = videojs;
-    }
-    assert.equal(Object.keys(DomData.elData).length, 0, 'no leaks, check videojs.domData.elData if failure');
-  });
-});
diff --git a/test/unit/videojs-integration.test.js b/test/unit/videojs-integration.test.js
index 2a36a57e05..7a2315ec16 100644
--- a/test/unit/videojs-integration.test.js
+++ b/test/unit/videojs-integration.test.js
@@ -2,7 +2,6 @@
 import videojs from '../../src/js/video.js';
 import window from 'global/window';
 import document from 'global/document';
-import * as DomData from '../../src/js/utils/dom-data';
 import * as Fn from '../../src/js/utils/fn';
 
 /**
@@ -70,54 +69,11 @@ QUnit.test('create a real player and dispose', function(assert) {
 
   player.muted(true);
 
-  const checkDomData = function() {
-    Object.keys(DomData.elData).forEach(function(elId) {
-      const data = DomData.elData[elId] || {};
-
-      Object.keys(data.handlers || {}).forEach(function(eventName) {
-        const listeners = data.handlers[eventName];
-        const uniqueList = [];
-
-        (listeners || []).forEach(function(listener) {
-          let add = true;
-
-          for (let i = 0; i < uniqueList.length; i++) {
-            const obj = uniqueList[i];
-
-            if (listener.og_ && listener.cx_ && obj.fn === listener.og_ && obj.cx === listener.cx_) {
-              add = false;
-              break;
-            }
-
-            if (listener.og_ && !listener.cx_ && obj.fn === listener.og_) {
-              add = false;
-              break;
-            }
-
-            if (!listener.og_ && !listener.cx_ && obj.fn === listener) {
-              add = false;
-              break;
-            }
-          }
-          const obj = {fn: listener.og_ || listener, cx: listener.cx_};
-
-          if (add) {
-            uniqueList.push(obj);
-            assert.ok(true, `${elId}/${eventName}/${obj.fn.name} is unique`);
-          } else {
-            assert.ok(false, `${elId}/${eventName}/${obj.fn.name} is not unique`);
-          }
-        });
-      });
-    });
-  };
-
   player.addTextTrack('captions', 'foo', 'en');
   player.ready(function() {
     assert.ok(player.tech_, 'tech exists');
     assert.equal(player.textTracks().length, 1, 'should have one text track');
 
-    checkDomData();
     player.dispose();
 
     Object.keys(old).forEach(function(k) {

From 13d46758f243c99d5b8a34957a42386500522f9c Mon Sep 17 00:00:00 2001
From: brandonocasey <brandonocasey@gmail.com>
Date: Thu, 18 Jul 2019 14:58:55 -0400
Subject: [PATCH 2/6] use it like a weakmap

---
 src/js/component.js              |  2 --
 src/js/utils/dom-data.js         | 48 ++------------------------------
 src/js/utils/events.js           | 26 +++++++++++------
 src/js/video.js                  |  3 ++
 test/unit/component.test.js      | 10 +++----
 test/unit/menu.test.js           |  4 +--
 test/unit/utils/dom-data.test.js | 23 ---------------
 7 files changed, 30 insertions(+), 86 deletions(-)
 delete mode 100644 test/unit/utils/dom-data.test.js

diff --git a/src/js/component.js b/src/js/component.js
index f2c9fb77c1..e583e96b84 100644
--- a/src/js/component.js
+++ b/src/js/component.js
@@ -7,7 +7,6 @@ import window from 'global/window';
 import evented from './mixins/evented';
 import stateful from './mixins/stateful';
 import * as Dom from './utils/dom.js';
-import * as DomData from './utils/dom-data';
 import * as Fn from './utils/fn.js';
 import * as Guid from './utils/guid.js';
 import toTitleCase from './utils/to-title-case.js';
@@ -152,7 +151,6 @@ class Component {
         this.el_.parentNode.removeChild(this.el_);
       }
 
-      DomData.removeData(this.el_);
       this.el_ = null;
     }
 
diff --git a/src/js/utils/dom-data.js b/src/js/utils/dom-data.js
index a5eb2f8999..f09e5e05f2 100644
--- a/src/js/utils/dom-data.js
+++ b/src/js/utils/dom-data.js
@@ -13,50 +13,6 @@
  * @type {Object}
  * @private
  */
-export const elData = new WeakMap();
+const elData = new WeakMap();
 
-/**
- * Returns the cache object where data for an element is stored
- *
- * @param {Element} el
- *        Element to store data for.
- *
- * @return {Object}
- *         The cache object for that el that was passed in.
- */
-export function getData(el) {
-  if (!elData.has(el)) {
-    elData.set(el, {});
-  }
-
-  return elData.get(el);
-}
-
-/**
- * Returns whether or not an element has cached data
- *
- * @param {Element} el
- *        Check if this element has cached data.
- *
- * @return {boolean}
- *         - True if the DOM element has cached data.
- *         - False otherwise.
- */
-export function hasData(el) {
-  if (!elData.has(el)) {
-    return false;
-  }
-
-  return !!Object.getOwnPropertyNames(elData.get(el)).length;
-}
-
-/**
- * Delete data for the element from the cache and the guid attr from getElementById
- *
- * @param {Element} el
- *        Remove cached data for this element.
- */
-export function removeData(el) {
-  // Remove all stored data
-  delete elData.delete(el);
-}
+export default elData;
diff --git a/src/js/utils/events.js b/src/js/utils/events.js
index f5ff79e800..296c24073b 100644
--- a/src/js/utils/events.js
+++ b/src/js/utils/events.js
@@ -7,7 +7,7 @@
  * @file events.js
  * @module events
  */
-import * as DomData from './dom-data';
+import DomData from './dom-data';
 import * as Guid from './guid.js';
 import log from './log.js';
 import window from 'global/window';
@@ -23,7 +23,10 @@ import document from 'global/document';
  *        Type of event to clean up
  */
 function _cleanUpEvents(elem, type) {
-  const data = DomData.getData(elem);
+  if (!DomData.has(elem)) {
+    return;
+  }
+  const data = DomData.get(elem);
 
   // Remove the events of a particular type if there are none left
   if (data.handlers[type].length === 0) {
@@ -48,7 +51,7 @@ function _cleanUpEvents(elem, type) {
 
   // Finally remove the element data if there is no data left
   if (Object.getOwnPropertyNames(data).length === 0) {
-    DomData.removeData(elem);
+    DomData.delete(elem);
   }
 }
 
@@ -250,7 +253,11 @@ export function on(elem, type, fn) {
     return _handleMultipleEvents(on, elem, type, fn);
   }
 
-  const data = DomData.getData(elem);
+  if (!DomData.has(elem)) {
+    DomData.set(elem, {});
+  }
+
+  const data = DomData.get(elem);
 
   // We need a place to store all our handler data
   if (!data.handlers) {
@@ -329,11 +336,11 @@ export function on(elem, type, fn) {
  */
 export function off(elem, type, fn) {
   // Don't want to add a cache object through getElData if not needed
-  if (!DomData.hasData(elem)) {
+  if (!DomData.has(elem)) {
     return;
   }
 
-  const data = DomData.getData(elem);
+  const data = DomData.get(elem);
 
   // If no events exist, nothing to unbind
   if (!data.handlers) {
@@ -405,7 +412,7 @@ export function trigger(elem, event, hash) {
   // Fetches element data and a reference to the parent (for bubbling).
   // Don't want to add a data object to cache for every parent,
   // so checking hasElData first.
-  const elemData = (DomData.hasData(elem)) ? DomData.getData(elem) : {};
+  const elemData = DomData.has(elem) ? DomData.get(elem) : {};
   const parent = elem.parentNode || elem.ownerDocument;
   // type = event.type || event,
   // handler;
@@ -432,7 +439,10 @@ export function trigger(elem, event, hash) {
 
   // If at the top of the DOM, triggers the default action unless disabled.
   } else if (!parent && !event.defaultPrevented && event.target && event.target[event.type]) {
-    const targetData = DomData.getData(event.target);
+    if (!DomData.has(event.target)) {
+      DomData.set(event.target, {});
+    }
+    const targetData = DomData.get(event.target);
 
     // Checks if the target has a default action for this event.
     if (event.target[event.type]) {
diff --git a/src/js/video.js b/src/js/video.js
index 41044ce438..f010d0b3f1 100644
--- a/src/js/video.js
+++ b/src/js/video.js
@@ -21,6 +21,7 @@ import { createTimeRanges } from './utils/time-ranges.js';
 import formatTime, { setFormatTime, resetFormatTime } from './utils/format-time.js';
 import log, { createLogger } from './utils/log.js';
 import * as Dom from './utils/dom.js';
+import DomData from './utils/dom-data.js';
 import * as browser from './utils/browser.js';
 import * as Url from './utils/url.js';
 import {isObject} from './utils/obj';
@@ -558,6 +559,8 @@ videojs.computedStyle = computedStyle;
  */
 videojs.dom = Dom;
 
+videojs.DomData = DomData;
+
 /**
  * A reference to the {@link module:url|URL utility module} as an object.
  *
diff --git a/test/unit/component.test.js b/test/unit/component.test.js
index f1dffa1c60..0c842328bc 100644
--- a/test/unit/component.test.js
+++ b/test/unit/component.test.js
@@ -2,7 +2,7 @@
 import window from 'global/window';
 import Component from '../../src/js/component.js';
 import * as Dom from '../../src/js/utils/dom.js';
-import * as DomData from '../../src/js/utils/dom-data';
+import DomData from '../../src/js/utils/dom-data';
 import * as Events from '../../src/js/utils/events.js';
 import * as Obj from '../../src/js/utils/obj';
 import * as browser from '../../src/js/utils/browser.js';
@@ -346,7 +346,7 @@ QUnit.test('should dispose of component and children', function(assert) {
     return true;
   });
   const el = comp.el();
-  const data = DomData.getData(el);
+  const data = DomData.get(el);
 
   let hasDisposed = false;
   let bubbles = null;
@@ -365,7 +365,7 @@ QUnit.test('should dispose of component and children', function(assert) {
   assert.ok(!comp.el(), 'component element was deleted');
   assert.ok(!child.children(), 'child children were deleted');
   assert.ok(!child.el(), 'child element was deleted');
-  assert.ok(!DomData.hasData(el), 'listener data nulled');
+  assert.ok(!DomData.has(el), 'listener data nulled');
   assert.ok(
     !Object.getOwnPropertyNames(data).length,
     'original listener data object was emptied'
@@ -979,7 +979,7 @@ QUnit.test('*AnimationFrame methods fall back to timers if rAF not supported', f
 QUnit.test('setTimeout should remove dispose handler on trigger', function(assert) {
   const comp = new Component(getFakePlayer());
   const el = comp.el();
-  const data = DomData.getData(el);
+  const data = DomData.get(el);
 
   comp.setTimeout(() => {}, 1);
 
@@ -996,7 +996,7 @@ QUnit.test('setTimeout should remove dispose handler on trigger', function(asser
 QUnit.test('requestAnimationFrame should remove dispose handler on trigger', function(assert) {
   const comp = new Component(getFakePlayer());
   const el = comp.el();
-  const data = DomData.getData(el);
+  const data = DomData.get(el);
   const oldRAF = window.requestAnimationFrame;
   const oldCAF = window.cancelAnimationFrame;
 
diff --git a/test/unit/menu.test.js b/test/unit/menu.test.js
index 857830f20b..7f07e8b0ee 100644
--- a/test/unit/menu.test.js
+++ b/test/unit/menu.test.js
@@ -1,5 +1,5 @@
 /* eslint-env qunit */
-import * as DomData from '../../src/js/utils/dom-data';
+import DomData from '../../src/js/utils/dom-data';
 import MenuButton from '../../src/js/menu/menu-button.js';
 import Menu from '../../src/js/menu/menu.js';
 import CaptionSettingsMenuItem from '../../src/js/control-bar/text-track-controls/caption-settings-menu-item';
@@ -137,7 +137,7 @@ QUnit.test('should remove old event listeners when the menu item adds to the new
    * A reusable collection of assertions.
    */
   function validateMenuEventListeners(watchedMenu) {
-    const eventData = DomData.getData(menuItem.eventBusEl_);
+    const eventData = DomData.get(menuItem.eventBusEl_);
     // `MenuButton`.`unpressButton` will be called when triggering click event on the menu item.
     const unpressButtonSpy = sinon.spy(menuButton, 'unpressButton');
     // `MenuButton`.`focus` will be called when triggering click event on the menu item.
diff --git a/test/unit/utils/dom-data.test.js b/test/unit/utils/dom-data.test.js
deleted file mode 100644
index 4077006e98..0000000000
--- a/test/unit/utils/dom-data.test.js
+++ /dev/null
@@ -1,23 +0,0 @@
-/* eslint-env qunit */
-import document from 'global/document';
-import * as DomData from '../../../src/js/utils/dom-data';
-
-QUnit.module('dom-data');
-
-QUnit.test('should get and remove data from an element', function(assert) {
-  const el = document.createElement('div');
-  const data = DomData.getData(el);
-
-  assert.strictEqual(typeof data, 'object', 'data object created');
-
-  // Add data
-  const testData = {asdf: 'fdsa'};
-
-  data.test = testData;
-  assert.strictEqual(DomData.getData(el).test, testData, 'data added');
-
-  // Remove all data
-  DomData.removeData(el);
-
-  assert.notOk(DomData.hasData(el), 'cached item emptied');
-});

From 0d1b594def6a4e257eacbb5631ada70363dec330 Mon Sep 17 00:00:00 2001
From: brandonocasey <brandonocasey@gmail.com>
Date: Thu, 18 Jul 2019 15:01:28 -0400
Subject: [PATCH 3/6] remove from vjs

---
 src/js/video.js | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/js/video.js b/src/js/video.js
index f010d0b3f1..41044ce438 100644
--- a/src/js/video.js
+++ b/src/js/video.js
@@ -21,7 +21,6 @@ import { createTimeRanges } from './utils/time-ranges.js';
 import formatTime, { setFormatTime, resetFormatTime } from './utils/format-time.js';
 import log, { createLogger } from './utils/log.js';
 import * as Dom from './utils/dom.js';
-import DomData from './utils/dom-data.js';
 import * as browser from './utils/browser.js';
 import * as Url from './utils/url.js';
 import {isObject} from './utils/obj';
@@ -559,8 +558,6 @@ videojs.computedStyle = computedStyle;
  */
 videojs.dom = Dom;
 
-videojs.DomData = DomData;
-
 /**
  * A reference to the {@link module:url|URL utility module} as an object.
  *

From 1bd13fd7e81fc849ee98bd4e199eca77d7e816ed Mon Sep 17 00:00:00 2001
From: brandonocasey <brandonocasey@gmail.com>
Date: Thu, 18 Jul 2019 15:01:41 -0400
Subject: [PATCH 4/6] only a weakmap

---
 src/js/utils/dom-data.js | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/src/js/utils/dom-data.js b/src/js/utils/dom-data.js
index f09e5e05f2..2c759349a0 100644
--- a/src/js/utils/dom-data.js
+++ b/src/js/utils/dom-data.js
@@ -3,16 +3,4 @@
  * @module dom-data
  */
 
-/**
- * Element Data Store.
- *
- * Allows for binding data to an element without putting it directly on the
- * element. Ex. Event listeners are stored here.
- * (also from jsninja.com, slightly modified and updated for closure compiler)
- *
- * @type {Object}
- * @private
- */
-const elData = new WeakMap();
-
-export default elData;
+export default new WeakMap();

From 32cd08fadee3818506f207af74f785b62de0ad1d Mon Sep 17 00:00:00 2001
From: brandonocasey <brandonocasey@gmail.com>
Date: Thu, 18 Jul 2019 15:02:46 -0400
Subject: [PATCH 5/6] bring the comment back

---
 src/js/utils/dom-data.js | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/js/utils/dom-data.js b/src/js/utils/dom-data.js
index 2c759349a0..43cf1a915e 100644
--- a/src/js/utils/dom-data.js
+++ b/src/js/utils/dom-data.js
@@ -3,4 +3,14 @@
  * @module dom-data
  */
 
+/**
+ * Element Data Store.
+ *
+ * Allows for binding data to an element without putting it directly on the
+ * element. Ex. Event listeners are stored here.
+ * (also from jsninja.com, slightly modified and updated for closure compiler)
+ *
+ * @type {Object}
+ * @private
+ */
 export default new WeakMap();

From 2f623f64554a214831a7fa14534245b4ab08f05a Mon Sep 17 00:00:00 2001
From: brandonocasey <brandonocasey@gmail.com>
Date: Thu, 18 Jul 2019 16:25:20 -0400
Subject: [PATCH 6/6] cr

---
 src/js/component.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/js/component.js b/src/js/component.js
index e583e96b84..1f197a7cf0 100644
--- a/src/js/component.js
+++ b/src/js/component.js
@@ -7,6 +7,7 @@ import window from 'global/window';
 import evented from './mixins/evented';
 import stateful from './mixins/stateful';
 import * as Dom from './utils/dom.js';
+import DomData from './utils/dom-data';
 import * as Fn from './utils/fn.js';
 import * as Guid from './utils/guid.js';
 import toTitleCase from './utils/to-title-case.js';
@@ -151,6 +152,9 @@ class Component {
         this.el_.parentNode.removeChild(this.el_);
       }
 
+      if (DomData.has(this.el_)) {
+        DomData.delete(this.el_);
+      }
       this.el_ = null;
     }