refactor redundant/verbose code in player.js #3597

brandonocasey · 2016-09-01T19:57:06Z

Description

refactor all event handling logic around tech
remove unused code
Save another 454 bytes on top of the previous PR (0.7%)
This PR depends upon refactor redundant code in html5 tech #3593

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

ToDo

Find a way to generate documentation for all the methods that are now dynamic, without compromising on the size of video.js

gkatsev · 2016-09-02T05:52:54Z

I don't think the changes to player.js here are worth the limited savings. We lose an easy way to document the functions, plus, there's a performance and memory hit here since we're creating a new functions each time a player is created. The player is the most important class in the project so making sure that it's well documented is essential.

brandonocasey · 2016-09-02T19:58:53Z

we should still be able to document the methods, and I can add all of this to the prototype rather than in the constructor

pam · 2016-09-02T20:12:16Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 8599123d7af103dc2f90d8ee2be31556d6556be2
Build details: https://travis-ci.org/pam/video.js/builds/157191006

(Please note that this is a fully automated comment.)

brandonocasey · 2016-09-02T20:55:43Z

src/js/player.js

- *
- * @event ended
- */
-Player.prototype.handleTechEnded_; // eslint-disable-line


this function actually exists

Are we not losing the @event ended here with this removal?

We are but I think this is the same or similar to: #3630 as @event just attached the event to the global scope in the docs. I can put @triggers on the method if we want though

I think we should keep it but make sure it gets updated as part of #3630.

pam · 2016-09-02T21:04:04Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 15cc4c0ef4da33471562a4f3ee34e8dd90dce4f2
Build details: https://travis-ci.org/pam/video.js/builds/157203479

(Please note that this is a fully automated comment.)

pam · 2016-09-02T21:08:12Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 37cd7780179df233f5a76d161bd8eb440eb497d2
Build details: https://travis-ci.org/pam/video.js/builds/157204402

(Please note that this is a fully automated comment.)

pam · 2016-09-02T21:41:17Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: e1eaf4bdc7ec9310f81daf3b45473c1db48e69f5
Build details: https://travis-ci.org/pam/video.js/builds/157211626

(Please note that this is a fully automated comment.)

misteroneill · 2016-09-08T17:53:30Z

src/js/player.js

 /**
 * Fired when the player has initial duration and dimension information
 *
 * @event loadedmetadata
+ * Player.prototype.handleLoadedMetaData_


Missing @method here?

yeah good catch

We probably don't want to have @method here. We don't want these to show up as documentation. These stubs specifically are used to document the events that the player listens to but are otherwise private methods. Adding a @private, I think, will then prevent the event from being displayed in the docs.

going to add @Private

misteroneill · 2016-09-09T14:00:39Z

@gkatsev Made a good point. We should make sure that anything with a trailing underscore is marked as @private because we don't want it showing up in docs.

misteroneill · 2016-09-12T14:19:33Z

src/js/tech/html5.js

+      elTracks.addEventListener('removetrack', this[`handle${capitalType}TrackRemove_`]);
+      this[`removeOld${capitalType}Tracks_`] = (e) => this.removeOldTracks_(techTracks, elTracks);
+
+      // Remove (native) trackts that are not used anymore


As long as we're in here, the "trackts" typo could be fixed.

This still needs an update.

this was changed in the other PR #3593, and I will rebase it when it is merged

Ah cool. Forgot that it included the other PR.

gkatsev

A few questions.

gkatsev · 2016-09-20T18:22:59Z

src/js/player.js

@@ -60,7 +140,6 @@ import './tech/html5.js';
 * @param {Element} tag        The original video tag used for configuring options
 * @param {Object=} options    Object of option names and values
 * @param {Function=} ready    Ready callback function
- * @extends Component


does this get picked up automatically?

yeah from my testing with jsdoc it does

gkatsev · 2016-09-20T18:26:52Z

src/js/player.js

 */
-Player.prototype.handleLoadedMetaData_; // eslint-disable-line


you don't need the method stub for jsdoc to pick this up?

no you don't, jsdoc assumes that it is a dynamic method

gkatsev · 2016-09-20T18:27:10Z

src/js/player.js

 /**
 * Fired when the player has initial duration and dimension information
 *
 * @event loadedmetadata
+ * @private


does @private still keep the events in the doc output?

let me double check

no id does not keep the events in the docs, and it seems like our event format may even be off http://usejsdoc.org/tags-event.html

Might be the old format or something? I would vote for removing the @private and filing a separate issue for fixing the @event doclets.

yeah it seems like @event should be it's own seperate tag, it does not show up even with @private removed I think that we just need lots of documentation updates at this point

this seems to be format that we would want it in, although it still looks a bit strange when I generate the docs, I think we need to do any more documentation changes/updates in another PR

/** * Fires loadedmetadata from the tech * * @fires Player#loadedmetadata * @method Player.prototype.handleLoadedMetaData_ * @private */ /** * Fired when the player has initial duration and dimension information * * @event Player#loadedmetadata * @type {Event} */

Sounds good. Want to open a new issue with that?

gkatsev

LGTM.
Test fails because of the firefox issue.

fixing doc block (should not have ended in ?)

brandonocasey reviewed Sep 2, 2016
View reviewed changes

misteroneill reviewed Sep 8, 2016
View reviewed changes

misteroneill reviewed Sep 12, 2016
View reviewed changes

gkatsev requested changes Sep 20, 2016

View reviewed changes

brandonocasey mentioned this pull request Sep 20, 2016

jsdoc comments are out of date, and there is not a good way to build them #3630

Closed

3 tasks

gkatsev approved these changes Sep 27, 2016

View reviewed changes

gkatsev modified the milestones: 5.13, 5.12 Sep 27, 2016

brandonocasey added 2 commits September 29, 2016 14:37

removing duplicate constructor docs

2b8174b

fixing doc block (should not have ended in ?)

adding back event doc comments

af584d7

gkatsev merged commit ae3e277 into videojs:master Sep 29, 2016

gkatsev mentioned this pull request Oct 5, 2016

chore: update CHANGELOG automation to use conventional-changelog #3669

Merged

brandonocasey deleted the chore/refactor-player-js branch December 6, 2016 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor redundant/verbose code in player.js #3597

refactor redundant/verbose code in player.js #3597

brandonocasey commented Sep 1, 2016

gkatsev commented Sep 2, 2016

brandonocasey commented Sep 2, 2016

pam commented Sep 2, 2016

brandonocasey Sep 2, 2016

gkatsev Sep 29, 2016

brandonocasey Sep 29, 2016

gkatsev Sep 29, 2016

pam commented Sep 2, 2016

pam commented Sep 2, 2016

pam commented Sep 2, 2016

misteroneill Sep 8, 2016

brandonocasey Sep 8, 2016

gkatsev Sep 9, 2016

brandonocasey Sep 9, 2016

misteroneill commented Sep 9, 2016

misteroneill Sep 12, 2016

brandonocasey Sep 12, 2016

gkatsev Sep 20, 2016

brandonocasey Sep 20, 2016

gkatsev Sep 20, 2016

gkatsev left a comment

gkatsev Sep 20, 2016

brandonocasey Sep 20, 2016

gkatsev Sep 20, 2016

brandonocasey Sep 20, 2016

gkatsev Sep 20, 2016

brandonocasey Sep 20, 2016

brandonocasey Sep 20, 2016

gkatsev Sep 20, 2016

brandonocasey Sep 20, 2016 •

edited

Loading

brandonocasey Sep 20, 2016

gkatsev Sep 20, 2016

brandonocasey Sep 20, 2016

gkatsev left a comment

		*/
		Player.prototype.handleLoadedMetaData_; // eslint-disable-line

refactor redundant/verbose code in player.js #3597

refactor redundant/verbose code in player.js #3597

Conversation

brandonocasey commented Sep 1, 2016

Description

Requirements Checklist

ToDo

gkatsev commented Sep 2, 2016

brandonocasey commented Sep 2, 2016

pam commented Sep 2, 2016

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

pam commented Sep 2, 2016

pam commented Sep 2, 2016

pam commented Sep 2, 2016

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

misteroneill commented Sep 9, 2016

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

gkatsev left a 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

brandonocasey Sep 20, 2016 • edited Loading

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

gkatsev left a comment

Choose a reason for hiding this comment

brandonocasey Sep 20, 2016 •

edited

Loading