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.

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

Move manual events into tech controller #1415

Closed
wants to merge 3 commits into from

Conversation

dmlap
Copy link
Member

@dmlap dmlap commented Aug 14, 2014

Migrate the timers that manage creating timeupdate and progress events when the tech doesn't support them natively. Now, techs that extend MediaTechController will continue to automatically pick up synthetic playback and buffering events but they're scoped much more closely to the entity that needs them. In addition, time and progress tracking have been moved much earlier into the component initialization which fixes #1414.

I can rebase this PR off of stable if it should be part of a patch release.

Migrate the timers that manage creating timeupdate and progress events when the tech doesn't support them natively. Now, techs that extend MediaTechController will continue to automatically pick up synthetic playback and buffering events but they're scoped much more closely to the entity that needs them. In addition, time and progress tracking have been moved much earlier into the component initialization which fixes videojs#1414.
vjs.Component.prototype.dispose.call(this);
};

vjs.MediaTechController.prototype.setCurrentTime = function() {
Copy link
Member

Choose a reason for hiding this comment

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

What if we added a listener to the seeking event instead of having this in the setCurrentTime function? That would avoid needing this include in the sub-techs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thinking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not seeing seeking or seeked events coming from the Flash player so this won't work. Defer?

Copy link
Member

Choose a reason for hiding this comment

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

Seeking should be in there.
2f5c9b6f1d919220201f0c1f0bc113a2271a/src/com/videojs/providers/HTTPVideoProvider.as#L522

But I believe we're currently trigger it directly on the player in the case of flash, in case you were listening for it on the tech or the object itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

In practice, doesn't seem to be working. Filed videojs/video-js-swf#114 to track it.

@heff
Copy link
Member

heff commented Aug 14, 2014

We're gonna have a few conflicts in this with #1412.

This doesn't break any use cases I know of and could be considered a bug, and maybe urgent? I'm ok making it a patch if you want to rebase it on stable. In that case we update the in-production copy on the CDN, so you gotta make sure it's good and tested, including IE8.

Other than the inline suggestions, lgtm.

The major browsers that support HTML video now reliably support progress events, so don't worry about synthesizing them for HTML anymore.
@dmlap
Copy link
Member Author

dmlap commented Aug 14, 2014

Ran the automated test on the latest desktop browsers, IE8, an iPhone, an iPad, and an Android phone and tablet. Did sandbox testing against the latest desktop browsers, IE8, an iPhone, an iPad, and an Android phone and tablet.

@dmlap
Copy link
Member Author

dmlap commented Aug 14, 2014

The logging test fails on IE8 before the dev console is opened but this occurs in master, too:

No other issues.

Pick up updated flash compiler settings.
@heff
Copy link
Member

heff commented Aug 14, 2014

replaced by #1417

@heff heff closed this Aug 14, 2014
@dmlap dmlap deleted the hotfix/manual-timeupdates branch August 15, 2014 06:04
gkatsev added a commit to gkatsev/video.js that referenced this pull request Sep 2, 2014
Due to videojs#1415, various usage of features moved around. This makes sure
that features are now called directly on the tech everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manualtimeupdates should be turned on before triggering player ready
2 participants