From 67ce786112b1ecff9a34dd58549c1ba3db770220 Mon Sep 17 00:00:00 2001
From: Dan Mosedale <dmose@meer.net>
Date: Fri, 27 Sep 2013 13:47:05 -0700
Subject: [PATCH 1/5] Window resize handler to staple local video to remote
 video

---
 static/css/chat.css                 |  5 +-
 static/js/chat/views.js             | 24 +++++++++
 static/js/utils.js                  |  7 +++
 test/frontend/chat/callview_test.js | 77 +++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/static/css/chat.css b/static/css/chat.css
index 3af32de2..b637f6b6 100644
--- a/static/css/chat.css
+++ b/static/css/chat.css
@@ -135,13 +135,14 @@ body {
   z-index: 1;
 }
 
-/* XXX once we get the local-video stapled to the remote-video,
+/* YYYaudit once we get the local-video stapled to the remote-video,
    much of this stuff may be irrelevant.  Audit this as part
    of that landing. */
 #call .local-video {
   position: absolute;
   bottom: 10px; /* 10px seems to give stablest stapling across most sizes */
-  right: 5px;
+  right: 5px; /* Adjusted dynamically, currently by callView resize handler
+               * If you change this; edit that handler */
   width: 20vw;
   min-width: 80px;
   max-width: 200px;
diff --git a/static/js/chat/views.js b/static/js/chat/views.js
index 0c2b3801..f7f9ce77 100644
--- a/static/js/chat/views.js
+++ b/static/js/chat/views.js
@@ -397,6 +397,7 @@
 
       this.call.on('change:state', this.render, this);
 
+      window.addEventListener("resize", this._onWindowResize);
       this.render();
     },
 
@@ -442,6 +443,29 @@
         this.$el.show();
       else
         this.$el.hide();
+    },
+
+    _localVideoGutterWidth: 5, // XXX change chat.css prop in sync with this
+    _onWindowResize: function(event) {
+
+      // (re)positions localVideo to be stapled inside right of remote-video
+
+      var localVideo = this.$('#local-video').get(0);
+      var remoteVideo = this.$('#remote-video').get(0);
+
+      var streamHeight = remoteVideo.videoHeight;
+      var streamWidth = remoteVideo.videoWidth;
+
+      if (!streamHeight || !streamWidth) {
+        return;
+      }
+
+      var pillarboxWidth = app.utils.getPillarboxWidth(
+        [remoteVideo.width, remoteVideo.height],
+        [streamWidth, streamHeight]);
+
+      localVideo.style.right =
+        this._localVideoGutterWidth + pillarboxWidth + "px";
     }
   });
 
diff --git a/static/js/utils.js b/static/js/utils.js
index 09b51795..c6b9d881 100644
--- a/static/js/utils.js
+++ b/static/js/utils.js
@@ -183,4 +183,11 @@
     return [scaleFactor * streamSize[0], scaleFactor * streamSize[1]];
   };
 
+  /**
+   * YYY empty because it needs to be stubbed for a test elsewhere.
+   * Up next: implement this
+   */
+  app.utils.getPillarboxWidth = function () {
+
+  };
 })(app, _);
diff --git a/test/frontend/chat/callview_test.js b/test/frontend/chat/callview_test.js
index aebd22ef..00f977bb 100644
--- a/test/frontend/chat/callview_test.js
+++ b/test/frontend/chat/callview_test.js
@@ -54,6 +54,17 @@ describe("CallView", function() {
       expect(shouldExplode).to.Throw(Error, /missing parameter: el/);
     });
 
+    it("should attach the #_onWindowResize handler to the window", function() {
+
+      sandbox.stub(window, "addEventListener");
+
+      var callView = new app.views.CallView({el: el, call: call});
+
+      sinon.assert.calledOnce(window.addEventListener);
+      sinon.assert.calledWithExactly(window.addEventListener, "resize",
+        callView._onWindowResize);
+    });
+
     describe("Change events", function() {
       var callView;
 
@@ -201,4 +212,70 @@ describe("CallView", function() {
       });
     });
   });
+
+  // Sure would be nice to test this using synthetic events, but those don't
+  // seem to work well enough just yet.  Oh well, one of these years!
+  describe("#_onWindowResize", function() {
+
+    var el, callView, localElement, fakeEvent, remoteElement,
+      fakePillarboxWidth;
+
+    beforeEach(function() {
+      el = $(['<div>',
+        '  <div id="local-video" width="20" height="20"></div>',
+        '  <div id="remote-video" width="320" height="200"></div>',
+        '</div>'].join(''));
+      $("#fixtures").append(el);
+      var $remoteElement = el.find('#remote-video');
+      remoteElement = $remoteElement.get(0);
+      remoteElement.videoHeight = 300;
+      remoteElement.videoWidth = 180;
+
+      callView = new app.views.CallView({el: el, call: call});
+
+      fakeEvent = {};
+
+      fakePillarboxWidth = 40;
+      sandbox.stub(app.utils, "getPillarboxWidth").returns(fakePillarboxWidth);
+    });
+
+    afterEach(function() {
+      $("#fixtures").empty();
+    });
+
+    it("should get the pillarbox width of the remoteVideo element",
+      function() {
+        callView._onWindowResize(fakeEvent);
+
+        sinon.assert.calledOnce(app.utils.getPillarboxWidth);
+        sinon.assert.calledWith(app.utils.getPillarboxWidth, sinon.match.array,
+          sinon.match.array);
+      });
+
+    it("should set the CSS |right| property on the localVideo element to" +
+      " pillarboxWidth + gutterWidth",
+      function() {
+        var $localElement = el.find('#local-video');
+        localElement = $localElement.get(0);
+        localElement.style.right = "";
+
+        callView._onWindowResize(fakeEvent);
+
+        expect(localElement.style.right).to.
+          equal(fakePillarboxWidth + callView._localVideoGutterWidth + "px");
+      });
+
+    // there's no meaningful thing to resize in this case, and making the call
+    // would violate the getPillarboxWidth API and cause an exception
+    it("should not call getPillarboxWidth if the remote stream's height" +
+      " or width is 0",
+      function() {
+        remoteElement.videoHeight = 0;
+
+        callView._onWindowResize(fakeEvent);
+
+        sinon.assert.notCalled(app.utils.getPillarboxWidth);
+      });
+  });
+
 });

From 2844292e7b587afcc011d9ce282d04c9d4ec8d64 Mon Sep 17 00:00:00 2001
From: Dan Mosedale <dmose@meer.net>
Date: Sun, 29 Sep 2013 10:35:45 -0700
Subject: [PATCH 2/5] Add fn to get width of pillarbox for displayed video

---
 static/js/utils.js          | 22 +++++++++++++++++++---
 test/frontend/utils_test.js | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/static/js/utils.js b/static/js/utils.js
index c6b9d881..b3100116 100644
--- a/static/js/utils.js
+++ b/static/js/utils.js
@@ -184,10 +184,26 @@
   };
 
   /**
-   * YYY empty because it needs to be stubbed for a test elsewhere.
-   * Up next: implement this
+   * Returns the width of the pillarbox (blank vertical column to the left
+   * or right of the video when preserving the video stream's aspect ratio
+   * requires the displayed video to be smaller than its containing box.
+   *
+   * XXX need to define behavior when width is not an integer
+   *
+   * @param {Array} boxSize -- size of entire <video> element: [width, height]
+   * @param {Array} streamSize -- size of the media stream: [width, height]
+   *
+   * @return {Number} the width of one of the pillarboxes (left and right
+   *                  pillarboxes should be sized identically
    */
-  app.utils.getPillarboxWidth = function () {
+  app.utils.getPillarboxWidth = function(boxSize, streamSize) {
+
+    var boxWidth = boxSize[0];
+
+    var displayedSize =
+      app.utils.computeDisplayedVideoSize(boxSize, streamSize);
+    var displayedWidth = displayedSize[0];
 
+    return (boxWidth - displayedWidth) / 2;
   };
 })(app, _);
diff --git a/test/frontend/utils_test.js b/test/frontend/utils_test.js
index 81456af8..eb2a02ab 100644
--- a/test/frontend/utils_test.js
+++ b/test/frontend/utils_test.js
@@ -301,5 +301,36 @@ describe('Utils', function() {
      */
     it("should have defined behavior when sizes are non-integer multiples");
     it("should have defined behavior on zero-box-size elements");
-  });   
+  });
+
+  describe("#getPillarboxWidth", function () {
+
+    // We're stubbing out computeDisplayedVideoSize even though it's
+    // in module because testing this API only would be very hard to
+    // ensure we got sufficient coverage.  I.e. this is a "humble function",
+    // analogous to the "humble object" pattern.
+
+    it("should return half the difference between the box width and" +
+      " the displayed video width",
+      function() {
+        var boxWidth=400;
+        var fakeDisplayedWidth = 271;
+        var fakeDimension = 1;
+        sandbox.stub(app.utils, "computeDisplayedVideoSize").
+          returns([fakeDisplayedWidth, fakeDimension]);
+
+        var pbWidth = app.utils.getPillarboxWidth([boxWidth, fakeDimension],
+          [fakeDimension, fakeDimension]);
+
+        expect(pbWidth).to.equal((boxWidth - fakeDisplayedWidth) / 2);
+      });
+
+
+    // XXX need to write test to define behavior when returned value would
+    // not be an integer, make the test pass, and remove the similar
+    // comment in the impl code
+    it("should behave in a defined way when the return value would not" +
+      " be an int");
+
+  });
 });

From 8cc55f2271b51657b20e7677e1595446e6b7f1d4 Mon Sep 17 00:00:00 2001
From: Dan Mosedale <dmose@meer.net>
Date: Sun, 29 Sep 2013 10:46:11 -0700
Subject: [PATCH 3/5] Fix resize handler binding; add some XXX comments

---
 static/js/chat/views.js             |  7 +++++--
 test/frontend/chat/callview_test.js | 13 +++++++++----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/static/js/chat/views.js b/static/js/chat/views.js
index f7f9ce77..ebcd6dfa 100644
--- a/static/js/chat/views.js
+++ b/static/js/chat/views.js
@@ -397,7 +397,7 @@
 
       this.call.on('change:state', this.render, this);
 
-      window.addEventListener("resize", this._onWindowResize);
+      window.addEventListener("resize", this._onWindowResize.bind(this));
       this.render();
     },
 
@@ -445,7 +445,10 @@
         this.$el.hide();
     },
 
-    _localVideoGutterWidth: 5, // XXX change chat.css prop in sync with this
+    // be sure to change the localVideo.right property in chat.css when
+    // changing this!
+    _localVideoGutterWidth: 5,
+
     _onWindowResize: function(event) {
 
       // (re)positions localVideo to be stapled inside right of remote-video
diff --git a/test/frontend/chat/callview_test.js b/test/frontend/chat/callview_test.js
index 00f977bb..cc402d22 100644
--- a/test/frontend/chat/callview_test.js
+++ b/test/frontend/chat/callview_test.js
@@ -54,15 +54,15 @@ describe("CallView", function() {
       expect(shouldExplode).to.Throw(Error, /missing parameter: el/);
     });
 
-    it("should attach the #_onWindowResize handler to the window", function() {
+    it("should attach a resize handler to the window", function() {
 
       sandbox.stub(window, "addEventListener");
 
-      var callView = new app.views.CallView({el: el, call: call});
+      new app.views.CallView({el: el, call: call});
 
       sinon.assert.calledOnce(window.addEventListener);
-      sinon.assert.calledWithExactly(window.addEventListener, "resize",
-        callView._onWindowResize);
+      sinon.assert.calledWith(window.addEventListener, "resize",
+        sinon.match.func);
     });
 
     describe("Change events", function() {
@@ -220,6 +220,7 @@ describe("CallView", function() {
     var el, callView, localElement, fakeEvent, remoteElement,
       fakePillarboxWidth;
 
+    // XXX adjust tests and code to use remote*.client*
     beforeEach(function() {
       el = $(['<div>',
         '  <div id="local-video" width="20" height="20"></div>',
@@ -243,6 +244,10 @@ describe("CallView", function() {
       $("#fixtures").empty();
     });
 
+    // XXX
+    it("should not fire when the view is not small");
+
+    // XXX adjust test to also check that the view is small
     it("should get the pillarbox width of the remoteVideo element",
       function() {
         callView._onWindowResize(fakeEvent);

From 1b2b9971939369af67239bf3c0a00686b0a21eb4 Mon Sep 17 00:00:00 2001
From: Dan Mosedale <dmose@meer.net>
Date: Mon, 30 Sep 2013 12:03:21 -0700
Subject: [PATCH 4/5] Correct tests and code to check that pillarbox info is
 based on computed style, not the width attribute

---
 static/js/chat/views.js             |  2 +-
 test/frontend/chat/callview_test.js | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/static/js/chat/views.js b/static/js/chat/views.js
index ebcd6dfa..34c1eaf4 100644
--- a/static/js/chat/views.js
+++ b/static/js/chat/views.js
@@ -464,7 +464,7 @@
       }
 
       var pillarboxWidth = app.utils.getPillarboxWidth(
-        [remoteVideo.width, remoteVideo.height],
+        [remoteVideo.clientWidth, remoteVideo.clientHeight],
         [streamWidth, streamHeight]);
 
       localVideo.style.right =
diff --git a/test/frontend/chat/callview_test.js b/test/frontend/chat/callview_test.js
index cc402d22..68875df5 100644
--- a/test/frontend/chat/callview_test.js
+++ b/test/frontend/chat/callview_test.js
@@ -222,6 +222,13 @@ describe("CallView", function() {
 
     // XXX adjust tests and code to use remote*.client*
     beforeEach(function() {
+
+      // note that we specify width and height here, even though they don't
+      // get laid out, and will therefore be _different_ than clientWidth
+      // and clientWidth (which happens for different reasons with <video>
+      // in the production code).  This is intentional so that our tests later
+      // can ensure the implementation will compute the correct results by
+      // using client* for its math.
       el = $(['<div>',
         '  <div id="local-video" width="20" height="20"></div>',
         '  <div id="remote-video" width="320" height="200"></div>',
@@ -252,9 +259,12 @@ describe("CallView", function() {
       function() {
         callView._onWindowResize(fakeEvent);
 
+        // Note that we are testing against clientWidth and clientHeight,
+        // because using width & height will not work in production.
         sinon.assert.calledOnce(app.utils.getPillarboxWidth);
-        sinon.assert.calledWith(app.utils.getPillarboxWidth, sinon.match.array,
-          sinon.match.array);
+        sinon.assert.calledWith(app.utils.getPillarboxWidth,
+          [remoteElement.clientWidth, remoteElement.clientHeight],
+          [remoteElement.videoWidth, remoteElement.videoHeight]);
       });
 
     it("should set the CSS |right| property on the localVideo element to" +

From 94e9f3446c05a492b4c266e0103d7a4a5cd9a5b4 Mon Sep 17 00:00:00 2001
From: Dan Mosedale <dmose@meer.net>
Date: Mon, 30 Sep 2013 16:21:05 -0700
Subject: [PATCH 5/5] Remove obsolete comment

---
 test/frontend/chat/callview_test.js | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/frontend/chat/callview_test.js b/test/frontend/chat/callview_test.js
index 68875df5..3bb1666f 100644
--- a/test/frontend/chat/callview_test.js
+++ b/test/frontend/chat/callview_test.js
@@ -220,7 +220,6 @@ describe("CallView", function() {
     var el, callView, localElement, fakeEvent, remoteElement,
       fakePillarboxWidth;
 
-    // XXX adjust tests and code to use remote*.client*
     beforeEach(function() {
 
       // note that we specify width and height here, even though they don't