1141296 - switch to parentless video elements and generate our own markup

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

AdamU from Tokbox suggested an approach for providing our own markup and styling.  I'd like to pair a bit to come up with some strategies for how we go about this.

It may be desirable to use the existing (rebased) BEM markup/css branch as a starting point, but maybe not.

After our meeting last week Rolly, Jonathan and I got together and had a think about your problems with our CSS/layout. We have come up with an interesting solution which we think will suit you nicely. It occurred to us that you can already use your own video element and just set the source of that element to be the same as the source of our video element. You don’t even need to add our UI to the DOM. If you create a parentless element and tell the subscribe and publish methods to replace that element then the DOM doesn’t get affected. Then on the completion handler you can set the video source of your own video element and call play. Something like:

var publisherVideo = document.querySelector('#publisherVideo'),
  mockPublisherEl = document.createElement('div'),
  publisher = OT.initPublisher(mockPublisherEl, function(err) {
    if (!err) {
      // Let's put the publisher into my own custom video element
      var pubVid = mockPublisherEl.querySelector('video');
      if (pubVid.mozSrcObject !== void 0) {
        publisherVideo.mozSrcObject = pubVid.mozSrcObject;
      } else {
        publisherVideo.src = pubVid.src;
      }
      publisherVideo.play();
    }
  });


I have created a jsbin as a proof of concept.
http://jsbin.com/rukoha

The nice thing about this is that it requires no special API from our perspective, you can start using it now. Also this should always work unless we drastically change our API. The downside is that it won’t work for IE, but my understanding is that this isn’t an issue for you guys right now.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

>      if (pubVid.mozSrcObject !== void 0) {

Out of simple curiosity, why don't we just test for undefined here? (or falsy)

Comment 2

[Adam Ullman]

I use `void 0` out of habit instead of undefined. The reason being that undefined isn't a protected keyword in javascript so someone could do a `var undefined = 'foo';` or `window.undefined = 'foo';` which could break the code if you're checking for undefined. `void 0` just evaluates to undefined but the void keyword is protected. Although I believe this isn't really an issue in modern browsers anyway.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Folks interested in the WIP can see it here: https://github.com/dmose/gecko-dev/compare/dmose:fx-team...parentless-video-1141296?diff=split&name=parentless-video-1141296

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Current work is happening on https://github.com/dmose/gecko-dev/compare/dmose:fx-team...markup-extraction-1141296?expand=1

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

https://github.com/dmose/gecko-dev/commit/aa8317892fb9e93db7ae5eef546bbe892396d133

The last commit currently on the branch fixes the direct muting stuff, but has the room views in broken state. That said, the ui-showcase is working.  

Alternately, "git checkout HEAD^" will get you a generally working thing, modulo facemute and some other bits.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Forced pushed new changes: 

* fixed more merge bustage
* did some of the simple renames requested in the first reviews

Then it became clear that things were in a broken enough state to do more review changes immediately, because it'd be flying _too_ blind and complex stuff might break midway through and we'd have to go bisect to figure out what was going on.

So, I switched gears, and worked on getting things into better shape.  Notably

* the UI showcase now reflects the StandaloneRoomView facemute stuff
* the StandaloneRoomView refactoring is basically working (still needs tests and cleanup)
* run-all-loop-tests.sh now passes all tests except for one of the LoopContacts tests, which seems implausible that these changes could have broken it (though I haven't looked closely).

Excitingly, the force push hid all the comments, because we were just using a compare view rather than an actual PR.  Fortunately, I have all the comments still in my Github notifications, and they link to working stuff, so it's all actually still there (you might have even seen one or two of my responses).

This time, I created an actual pull request, so, with luck, the next rebase/forced push will be better behaved:

https://github.com/dmose/gecko-dev/pull/6

My suggestion as to next steps:

* keep kicking butt on the review front :-)

If you get to the end of that and have bandwidth, use the StandaloneRoomView code and diffs from the last two WIP patches as a pattern to do the same port on the DesktopRoomConversationView.

Main things to do after that will be to:

* cleanup and write tests for the WIP checkins
* go through and address the remaining review comments.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

The PR might be sorta "feature complete" at this point, though there's still plenty of cleanup to do.  The DesktopRoomConversationView has been ported, the remaining screenshare code has been been ported as well, though I'm concerned that there may be some edge-raciness.  The screenshare stuff needs more hand testing.

My (modified) suggestion as to next steps:

* keep kicking butt on the review front :-)

If you get to the end of that and have bandwidth, the key remaining stuff to do will be:

* hand-test the screenshare stuff on both Fx and Chrome
* cleanup and write tests for the WIP checkins (**)
* go through and address the remaining review comments
* various cleanups/bug-fixes from my notes not yet enumerated here :-)

My current thinking here is that the shortest path to getting tested code is simply to parameterize and hoist the majority of the various shared functions across the views into a single mixin.  The one exception is that it may make sense to allow the views themselves to either override or implement ShouldRenderRemoteVideo, as that doesn't generalize quite so nicely as the rest.

Ultimately, of course, we want to compose each of these giant views from a bunch of simpler sub-components, but that's for another day...

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

I've updated https://github.com/dmose/gecko-dev/pull/6 (mostly just screenshare cleanup folded into the last commit).

Since there will now be a few of us driving this to completion, I've taken my local notes, and stuffed them into https://etherpad.mozilla.org/markup-extraction for general use.  I did a bunch of triage, and although it looks like a big list, I expect many if not most of the things to be pretty straightforward.

Mark, feel free to pick up stuff from that list.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

I've landed the otSdkDriver changes on the MediaView commits and rebased.  There are a couple of small regressions, so I haven't folded them together on the rebase just yet.

I've also pushed to a new branch and a new PR just-in-case anything went wrong, so we still have the old branch around to look at.

For development, though, I suggest switching to:

https://github.com/dmose/gecko-dev/pull/7

See the Etherpad for the regression details.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

I added tests for the StandloneRoomViews, addressed all of Standard8's review comments, and have rebased and force-pushed the PR.  The etherpad has been updated.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

OK, I filed some more bugs, and cleaned things up so that I believe all the XXXs we introduced are in reasonable shape, i.e. ones where it seemed important to have bugs linked to have that.  The only YYY comments left are in log messages which we'll remove as a last step before we squash and land.

I've visually verified a bunch of views.  Once I hit the standalone views with video, I started seeing the chat textbox showing up in my real profile, even though it's not in the ui-showcase, and my pref for that is set to the default (false).  Next step for that is try a clean profile.

In the meantime, we're very close now, I think.

Comment 12

[Mark Banner (:standard8)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #11)
> I've visually verified a bunch of views.  Once I hit the standalone views
> with video, I started seeing the chat textbox showing up in my real profile,
> even though it's not in the ui-showcase, and my pref for that is set to the
> default (false).  Next step for that is try a clean profile.

Fixed now - it was a bug in the otSdkDriver that had been introduced in the patch set. Not sure why I wasn't seeing it before.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/df5ab2448944

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

r=Standard8 via github comments, pairing, and IRC.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/df5ab2448944