Skip to content

fix video module issue with require.js - #1337

Merged
singingwolfboy merged 7 commits into
masterfrom
db/fix-video-requirejs
Oct 16, 2013
Merged

fix video module issue with require.js#1337
singingwolfboy merged 7 commits into
masterfrom
db/fix-video-requirejs

Conversation

@singingwolfboy

Copy link
Copy Markdown
Contributor

Previously, the YouTube API and the Video Xmodule had a race condition -- Xmodule was waiting for the www.youtube.com/player_api script to load before executing the video scripts, but that file is just a loader that handles loading the real YouTube API. As a result, the real YouTube API and the Xmodule videoplayer were loaded simultaneously, and if Xmodule loaded before YouTube, the video would not display on the page. Fortunately, the YouTube API loader exposes a YT.ready function, that will execute its argument when the YouTube API is fully loaded, so it wasn't too difficult to make Xmodule wait until the YouTube API is fully loaded.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

On Friday, I was able to reliably reproduce the bug with the video module loading without any content. Today, I put this branch up on a server on AWS and tested in exactly the same configuration where I was able to reliably reproduce the bug on Friday. I wasn't able to reproduce the bug at all on this branch; the video loaded successfully three times in a row.

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@cahrens @valera-rozuvan Can you two review this pull request, please?

Comment thread cms/templates/base.html Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm surprised to see this deletion in the file. I imagine a rebase is in order....

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

I had to disable two Jasmine tests in this pull request, because they were failing for (what appear to be) reasons unrelated to the change in this pull request. Jasmine runs tests in a deterministic order, but that order is not the order that the tests were defined in in; I suspect that the modifications I made to the tests caused the tests to run in a different order than before, and that these test failures are due to issues of test isolation. (In theory, reordering tests shouldn't cause them to break -- in practice, sometimes it does.)

@valera-rozuvan, when you review this PR, can you investigate these test failures, and see if you can figure out what's causing them to fail and how to fix them? If we could get these tests running and passing again before the branch is merged, that would be excellent.

@valera-rozuvan

Copy link
Copy Markdown
Contributor

@singingwolfboy I have rebased against master. This will get my latest PR. Also I have updated Jasmine describe text messages. All tests are passing locally. I have also tested the Video player in Studio with network packet loss set to 10%. All works well.

Now will try to identify the cause for the failing of the 2 Jasmine tests that you have commented out.

@valera-rozuvan

Copy link
Copy Markdown
Contributor

@singingwolfboy Enabled and fixed 1 of your disabled tests. Also enabled and fixed 1 unrelated test.

@valera-rozuvan

Copy link
Copy Markdown
Contributor

@singingwolfboy Fixed second disabled test.

@valera-rozuvan

Copy link
Copy Markdown
Contributor

👍 Good to merge.

@cahrens

cahrens commented Oct 16, 2013

Copy link
Copy Markdown

👍 Once tests pass.

singingwolfboy added a commit that referenced this pull request Oct 16, 2013
fix video module issue with require.js
@singingwolfboy
singingwolfboy merged commit e363e5a into master Oct 16, 2013
@singingwolfboy
singingwolfboy deleted the db/fix-video-requirejs branch October 16, 2013 16:40
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Oct 21, 2016
Agrendalath pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 2, 2019
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.

3 participants