Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add several debugging aides #5854

Closed
wants to merge 1 commit into from
Closed

feat: add several debugging aides #5854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/js/mixins/evented.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ const EventedMixin = {
// Use the same function ID as the listener so we can remove it later
// it using the ID of the original listener.
wrapper.guid = listener.guid;
wrapper.original_ = listener.original_ || listener;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'd want to make sure to null these out so that we're not leaking references.

listen(target, 'one', type, wrapper);
}
},
Expand Down
2 changes: 1 addition & 1 deletion src/js/utils/dom-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import * as Guid from './guid.js';
* @type {Object}
* @private
*/
const elData = {};
export const elData = {};

/*
* Unique attribute name to store an element's guid in
Expand Down
7 changes: 7 additions & 0 deletions src/js/utils/fn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ export const bind = function(context, fn, uid) {
// currently used in text tracks
bound.guid = (uid) ? uid + '_' + fn.guid : fn.guid;

bound.original_ = fn.original_ || fn;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make sure to null these out when they're no longer needed.

bound.context_ = fn.context_ || context;

return bound;
};

Expand Down Expand Up @@ -72,6 +75,8 @@ export const throttle = function(fn, wait) {
}
};

throttled.original_ = fn.original_ || fn;

return throttled;
};

Expand Down Expand Up @@ -133,5 +138,7 @@ export const debounce = function(func, wait, immediate, context = window) {

debounced.cancel = cancel;

debounced.original_ = func.original_ || func;

return debounced;
};
3 changes: 3 additions & 0 deletions src/js/video.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,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 * as 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';
Expand Down Expand Up @@ -553,6 +554,8 @@ videojs.computedStyle = computedStyle;
*/
videojs.dom = Dom;

videojs.domData = DomData;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against this being in a production build of Video.js, though, I do see the usefulness of it when debugging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can have this be here during local dev but not when releasing, that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For local development we can just import DomData (as long as we let it export elData). I think that plugins could also benefit from being able to see what elData they may be leaking, but I am not quite sure how to do that without exposing it here.

Copy link
Contributor Author

@brandonocasey brandonocasey Mar 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can expose it somewhere else? or put elData on the player somehow so that the player can remove it all on dispose?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we can safely expose it and not deal with consequences down the line or have issues with people fiddling with it and causing issues.


/**
* A reference to the {@link module:url|URL utility module} as an object.
*
Expand Down