Tech 2.0 #2057

eXon · 2015-04-22T11:29:45Z

I'm starting a pull request to make sure I'm working in the right direction and that everyone agrees with that. (#2008)

By the way I couldn't test Flash because videojs.options doesn't exists anymore so I can't set the swf path.

Let me know what you guys think.

Roadmap

Trigger events on the tech instead of the player (Change techs to emit events instead of triggering them on the player #1485)
Player should be responsible for adding the DOM element of the tech on himself (using tech.el_)
Remove every dependencies of the player on
- tech (fix Captions: MediaTechController initializes some listeners for TextTrackDisplay #1863)
- html5
- flash
Remove the player from the tech constructor signature
Fix unit tests

gkatsev · 2015-04-22T15:36:40Z

You should be able to pass in the swf via an option to the player you're creating:

videojs('foo', {
  flash: {
    swf: "http://google.com"
  }
});

Which reminds me, we probably want a way to change these defaults globally so you won't be required to specify it for each player.

heff · 2015-04-23T00:00:30Z

src/js/tech/flash.js

@@ -321,7 +321,7 @@ Flash['checkReady'] = function(tech){
 // Trigger events from the swf on the player
 Flash['onEvent'] = function(swfID, eventName){
  let player = Lib.el(swfID)['player'];
-  player.trigger(eventName);
+  player.tech.trigger(eventName);


We should store a reference to the tech on the swf object, probably after it's created. Then we don't need to reference the player here.

Yeah, I think the placeholder was actually a remnant of using swfobject.js, which required you to replace an existing element. #1340 got us a little closer but I think the placeholder could be refactored out now.

This specific line though is the swf's reference to the tech, not the tech's reference to the swf. We need the swf to have a reference because the Flash events get triggered globally and we need to figure out which tech they apply to.

This is an out-dated diff (I think I misplaced my comment for the placeholder). It is now:

let tech = Lib.el(swfID)['tech']; tech.trigger(eventName);

and the tech is saved within the element instead of the player.

heff · 2015-04-23T00:05:02Z

Looking great so far!

eXon · 2015-04-23T01:08:09Z

@gkatsev The option is now working. However, it seems like Flash is never sending me the onReady event. Any clue?

I'm on Ubuntu / Chrome and Flash works fine.

…k on himself

eXon · 2015-04-23T12:13:02Z

src/js/tech/flash.js

@@ -26,9 +26,6 @@ class Flash extends Tech {

    let { source, parentEl } = options;

-    // Create a temporary element to be replaced by swf object
-    let placeHolder = this.el_ = Lib.createEl('div', { id: player.id() + '_temp_flash' });


I tried to guess why we were using this placeholder and I ended up looking at 2 years old commits. My only guess is that it was needed in the iframe mode. It seems to work perfectly fine without using it. @heff any clue?

I believe the placeholder used to be required for the flash fallback back in the day. I think it can be safely removed now.

Yeah, placeholder can be removed now.

heff · 2015-04-23T21:16:24Z

src/js/player.js

+   * Fires when the browser is intentionally not getting media data
+   * @event suspend
+   */
+  onTechSuspend() {


For these simple ones we could probably loop though a list of the events and create these dynamically. Though it is kind of nice to have an excuse to add the event jsdoc tag.

heff · 2015-04-23T21:53:31Z

So it looks like we previously broke Flash. The swf is still told to fire the videojs.Flash.onReady event, but we've changed that to just Flash.onReady, and it's no longer available globally. We'll need to re-export that to the window some how.

In videojs/video-js-swf#143 we hardcoded these functions to prevent security issues. So either we'll need to expose window.videojs everywhere again, or create new global functions and release a new version of the swf.

gkatsev · 2015-04-23T22:01:41Z

It would be awesome if we export a different flash-onready function per player on the window object :)

heff · 2015-04-23T22:02:57Z

Is there a way we can do that without opening the security hole back up?

gkatsev · 2015-04-24T03:55:33Z

Oh, right, completely forgot about that. Hm...

heff · 2015-04-24T04:15:21Z

src/js/player.js

+    this.on(this.tech, 'pause', this.onTechPause);
+    this.on(this.tech, 'progress', this.onTechProgress);
+    this.on(this.tech, 'durationchange', this.onTechDurationChange);
+    this.on(this.tech, 'fullscreenchange', this.onTechFullscreenChange);


fullscreenchange is actually a player event, not tech.

This fullscreenchange listener could still be removed, now that you've added it to the player.

eXon · 2015-05-02T00:21:08Z

Hopefully I have addressed everything here and we can move on smaller PR now :)

eXon · 2015-05-02T00:22:23Z

Big thanks for the merge by the way @heff, very helpful

gkatsev · 2015-05-04T18:42:47Z

Looks like you need to rebase against master again, sorry. On the bright side, the flash tech should be working again :)

eXon · 2015-05-04T18:44:39Z

Yay! I'll try tonight :)

On Mon, May 4, 2015 at 2:42 PM Gary Katsevman [email protected]
wrote:

Looks like you need to rebase against master again, sorry. On the bright
side, the flash tech should be working again :)

—
Reply to this email directly or view it on GitHub
#2057 (comment).

eXon · 2015-05-05T01:21:25Z

Flash is working perfectly now :) let's merge this

heff · 2015-05-06T16:31:02Z

src/js/player.js

-    var techReady = function(){
-      this.player_.triggerReady();
-    };
+    var techReady = Lib.bind(this, function() {


I think this is duplicating line 297 now? https://github.com/videojs/video.js/pull/2057/files#diff-3b0266ff1c037b289ec624ab25b0272eR297

heff · 2015-05-06T17:58:20Z

I want to go ahead and pull this in to prevent further merge conflicts, but there's a list of follow tasks we'll need to do to really finish it off. See #2126.

eXon · 2015-05-06T18:03:36Z

Awesome :) I'll take a closer look at all this in the upcoming days

heff · 2015-05-06T18:03:56Z

Woo!

heff · 2015-05-06T18:05:57Z

@eXon do you think you could add any relevant notes for other tech developers to the 5.0 changes?
https://github.com/videojs/video.js/wiki/5.0-Change-Details

eXon · 2015-05-06T18:07:40Z

Of course. You can add this to #2126

eXon added 2 commits April 22, 2015 07:09

Tech triggers on himself instead of the player

6576e4e

Dispose should already remove the event listeners

ae25b8f

heff reviewed Apr 23, 2015
View reviewed changes

eXon added 3 commits April 22, 2015 21:35

Listen for tech error + save tech instance in the DOM for flash

34f90bb

Added missing HTML5 events the player needs to listen and trigger bac…

537907e

…k on himself

Player is responsible for adding the tech DOM element inside himself

005aa9d

eXon reviewed Apr 23, 2015
View reviewed changes

Improved the check if the tech is in the DOM

11bbb7b

heff reviewed Apr 23, 2015
View reviewed changes

eXon added 2 commits April 23, 2015 20:34

Fix the HTML5 video reuse

19d19f8

Removed player references in the Tech base class

358880b

heff reviewed Apr 24, 2015
View reviewed changes

eXon added 4 commits May 1, 2015 20:06

Fixed the setControls call when using native controls

d699417

Removed unused variable

0b51928

Removed firefox fix because it has been fixed in the browser

ccf0308

Making sure textTracks_ is set before it can be touched

a8062ed

Merge with upstream

5fa8eea

heff reviewed May 6, 2015
View reviewed changes

heff mentioned this pull request May 6, 2015

Additional Tech 2.0 Tasks #2126

Closed

8 tasks

heff closed this in e5595b1 May 6, 2015

heff added enhancement confirmed labels May 6, 2015

saxena-gaurav mentioned this pull request May 6, 2015

Bugfix for dispose when changing techs. #2125

Closed

gkatsev mentioned this pull request Jul 15, 2015

Captions: MediaTechController initializes some listeners for TextTrackDisplay #1863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tech 2.0 #2057

Tech 2.0 #2057

eXon commented Apr 22, 2015

gkatsev commented Apr 22, 2015

heff Apr 23, 2015

heff Apr 23, 2015

eXon Apr 23, 2015

heff commented Apr 23, 2015

eXon commented Apr 23, 2015

eXon Apr 23, 2015

gkatsev Apr 30, 2015

heff May 1, 2015

heff Apr 23, 2015

heff commented Apr 23, 2015

gkatsev commented Apr 23, 2015

heff commented Apr 23, 2015

gkatsev commented Apr 24, 2015

heff Apr 24, 2015

heff Apr 28, 2015

eXon commented May 2, 2015

eXon commented May 2, 2015

gkatsev commented May 4, 2015

eXon commented May 4, 2015

eXon commented May 5, 2015

heff May 6, 2015

heff commented May 6, 2015

eXon commented May 6, 2015

heff commented May 6, 2015

heff commented May 6, 2015

eXon commented May 6, 2015

Tech 2.0 #2057

Tech 2.0 #2057

Conversation

eXon commented Apr 22, 2015

Roadmap

gkatsev commented Apr 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Apr 23, 2015

eXon commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Apr 23, 2015

gkatsev commented Apr 23, 2015

heff commented Apr 23, 2015

gkatsev commented Apr 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eXon commented May 2, 2015

eXon commented May 2, 2015

gkatsev commented May 4, 2015

eXon commented May 4, 2015

eXon commented May 5, 2015

Choose a reason for hiding this comment

heff commented May 6, 2015

eXon commented May 6, 2015

heff commented May 6, 2015

heff commented May 6, 2015

eXon commented May 6, 2015