Skip to content

Update component.js#1334

Closed
sethborg wants to merge 3 commits intovideojs:masterfrom
sethborg:master
Closed

Update component.js#1334
sethborg wants to merge 3 commits intovideojs:masterfrom
sethborg:master

Conversation

@sethborg
Copy link
Copy Markdown
Contributor

@sethborg sethborg commented Jul 8, 2014

Removed the html tag so firefox would not continue to download the movie file in the background.

Removed the html tag so firefox would not continue to download the movie file in the background.
@heff
Copy link
Copy Markdown
Member

heff commented Jul 8, 2014

Thanks for helping out. Instead of component.js, this should go in the html5 tech's dispose method.
https://github.com/videojs/video.js/blob/v4.6.3/src/js/media/html5.js#L64

sethborg added 2 commits July 10, 2014 09:03
If the HTML tag is not removed, the video will still download in the background in Firefox after the player is disposed.
Moved my fix to src/js/media/html5.js
@sethborg
Copy link
Copy Markdown
Contributor Author

OK what's the status now?

@heff
Copy link
Copy Markdown
Member

heff commented Jul 10, 2014

Here's what I think needs to happen there:

vjs.Html5.disposeMediaElement(this.el_);
vjs.MediaTechController.prototype.dispose.call(this);

We should be referring the the tech's element here instead of the player's tag. They're one in the same except on platforms where we throw away the original tag and create a new element. When that happens we already dispose the original tag correctly, but without this change we'd miss disposing the newly created element.

Can you test if this does what you need it to?

@sethborg
Copy link
Copy Markdown
Contributor Author

Tested. Yes that works.

@heff
Copy link
Copy Markdown
Member

heff commented Jul 11, 2014

Great, thanks. Will merge in soon.
On July 10, 2014 at 12:42:06 PM, sethborg (notifications@github.com) wrote:

Tested. Yes that works.


Reply to this email directly or view it on GitHub.

@sethborg
Copy link
Copy Markdown
Contributor Author

Disappointed to see 4.6.4 does not contain this fix.

@mmcc
Copy link
Copy Markdown
Member

mmcc commented Jul 15, 2014

@sethborg Sorry about that, 4.6.4 had some pretty crucial fixes that needed to go out as soon as possible. Since we were releasing on a Friday afternoon we opted to keep it as simple as possible to avoid breaking things for everyone over a weekend :) This will get pulled in soon.

@heff
Copy link
Copy Markdown
Member

heff commented Jul 29, 2014

Made some updates and waiting on @sethborg's approval: sethborg#1

@heff heff closed this in f00f354 Aug 1, 2014
@heff
Copy link
Copy Markdown
Member

heff commented Aug 1, 2014

@sethborg Thanks for the help! This will go out with the next minor release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants