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

Gh 1208 #1262

Merged
merged 5 commits into from Feb 26, 2014

Conversation

Projects
None yet
3 participants
@jdragojevic
Contributor

jdragojevic commented Feb 25, 2014

multiple-tab playback issues are resolved for me.

@bendk

This comment has been minimized.

Show comment
Hide comment
@bendk

bendk Feb 25, 2014

Member

I'm a little worried about this fix since it's not guaranteed that we can add a query param and get the same video back. I'm pretty sure that this will work on at least 99.9% of all videos out there, but I wonder if some servers will return something different. There's also a small chance that the URL already includes a noise param.

If there's no other way to fix the problem, then I guess we have no choice but to go with the hack. But maybe we should limit it to only chrome, and also change the query param to amaranoise or something like that to lower the chance of a name collision.

Member

bendk commented Feb 25, 2014

I'm a little worried about this fix since it's not guaranteed that we can add a query param and get the same video back. I'm pretty sure that this will work on at least 99.9% of all videos out there, but I wonder if some servers will return something different. There's also a small chance that the URL already includes a noise param.

If there's no other way to fix the problem, then I guess we have no choice but to go with the hack. But maybe we should limit it to only chrome, and also change the query param to amaranoise or something like that to lower the chance of a name collision.

@syl22-00

This comment has been minimized.

Show comment
Hide comment
@syl22-00

syl22-00 Feb 25, 2014

Member

@bendk you're right for the parameter name, I'll change it.

I am not very happy with the fix either, it also might prevent caching the video. The problem is that the issue itself is not clear, and it's more a hack around that unclear browser issue.

I'll also try to limit its scope (only html5 videos on chrome).

Member

syl22-00 commented Feb 25, 2014

@bendk you're right for the parameter name, I'll change it.

I am not very happy with the fix either, it also might prevent caching the video. The problem is that the issue itself is not clear, and it's more a hack around that unclear browser issue.

I'll also try to limit its scope (only html5 videos on chrome).

@syl22-00

This comment has been minimized.

Show comment
Hide comment
@syl22-00

syl22-00 Feb 25, 2014

Member

@bendk I made the few changes we discussed above, do you think that's OK/better? @jdragojevic would you mind giving it another try, trying to cover most of our possible URLs?

Member

syl22-00 commented Feb 25, 2014

@bendk I made the few changes we discussed above, do you think that's OK/better? @jdragojevic would you mind giving it another try, trying to cover most of our possible URLs?

@bendk

This comment has been minimized.

Show comment
Hide comment
@bendk

bendk Feb 25, 2014

Member

The changes look good to me.

Member

bendk commented Feb 25, 2014

The changes look good to me.

bendk added a commit that referenced this pull request Feb 26, 2014

@bendk bendk merged commit d403fe7 into staging Feb 26, 2014

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