963724 - media clips are not loading if togetherjs is being used

Status: RESOLVED FIXED

(Webmaker Graveyard :: Popcorn Maker, defect)

Description

[Scott [:thecount] Downe]

No idea yet what's happening, but it's pretty busted.

Feels like we're not sending all the track event data to togetherjs.

Comment 1

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/popcorn-js/pull/373

I highly suspect together.js may be loading youtube's script after us, and they redefine the window. ready function.

I have a change where we define it exactly when we need it, and not some time before, giving there no moment where this can break.

Comment 2

[Jon Buckley [:jbuck]]

Comment on attachment 8365299 [details] [review]
https://github.com/mozilla/popcorn-js/pull/373

Why even clobber the YT API? Why not just use it as-is if it's loaded?

Comment 3

[Scott [:thecount] Downe]

Adding the script doesn't mean it is ready, it becomes ready some time later. Otherwise a onYouTubeIframeAPIReady would not be necessary.

So, if window.YT is true, and we instead don't clobber it, and just call the ready function, it could error.

By removing the script, and resetting the function, we get to retrigger a ready function.

I'm not putting this back into review, because I kinda want to look for other solutions and there is no rush, I'll think on it over the weekend. I might even file this on YouTube's end asking for a YT.isYouTUbeIframeReady or something. Then, a way to add multiple ready functions in case youtube is not ready, but a onYouTUbeIframeAPIReady is already defined.

It should be a YT.listen( "youTUbeIframeAPIReady ", function ); or something.

Comment 4

[Jon Buckley [:jbuck]]

Comment on attachment 8365299 [details] [review]
https://github.com/mozilla/popcorn-js/pull/373

r+ with nits noted in the PR

Comment 5

[Scott [:thecount] Downe]

Comment on attachment 8365299 [details] [review]
https://github.com/mozilla/popcorn-js/pull/373

Updated.

Was actually quite a few changes.

Comment 6

[Jon Buckley [:jbuck]]

Comment on attachment 8365299 [details] [review]
https://github.com/mozilla/popcorn-js/pull/373

Didn't the version before rely on window.YT.loaded or something instead of clobbering the YT API? Did you decide against using that?

Comment 7

[Scott [:thecount] Downe]

Yes ut did... not sure where it went. Going to find it and merge it in.

Comment 8

[Scott [:thecount] Downe]

Comment on attachment 8365299 [details] [review]
https://github.com/mozilla/popcorn-js/pull/373

I had two branches locally, and one remote.

I forgot about that, and fixed up the first one and not the second, which happened to have all the the same issues, so it didn't look out of place.

I have removed the wrong branch and only have one locally now.

Should be good.

Comment 9

[Jon Buckley [:jbuck]]

Comment on attachment 8365299 [details] [review]
https://github.com/mozilla/popcorn-js/pull/373

Looks great! r+ with one nit noted in the PR: https://github.com/mozilla/popcorn-js/pull/373/files#r9759297

I'm not sure what criteria you used to choose a 1000ms timeout, but maybe it can be lowered? Up to you

Comment 10

[Scott [:thecount] Downe]

Staged: https://github.com/mozilla/popcorn-js/commit/4fe23b3d94e7b8f9e4a1948894a86753ff7f6644

Needs verification.

I lowered it to 250. I had no criteria other than I needed something. 250 is also fine.

Comment 11

[Scott [:thecount] Downe]

Also staged: https://github.com/mozilla/popcorn.webmaker.org/commit/81971a81fd542e7db89aa477c206ecdb04858366