From d14777ac8667909c73c01ac3ffbd0122d90c8637 Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Mon, 9 Sep 2019 17:31:28 -0700
Subject: [PATCH 1/8] fix(viewer): Pause audio instead of muting it if autoplay
 is prevented

---
 src/lib/viewers/media/MP3Viewer.js            | 27 +++++++++++++++++++
 src/lib/viewers/media/MediaBaseViewer.js      | 11 ++++++--
 .../viewers/media/__tests__/MP3Viewer-test.js | 13 +++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/src/lib/viewers/media/MP3Viewer.js b/src/lib/viewers/media/MP3Viewer.js
index 6f35a3dc6..393ddf042 100644
--- a/src/lib/viewers/media/MP3Viewer.js
+++ b/src/lib/viewers/media/MP3Viewer.js
@@ -34,6 +34,33 @@ class MP3Viewer extends MediaBaseViewer {
         this.mediaControls.show();
         this.mediaControls.resizeTimeScrubber();
     }
+
+    /**
+     * Determines if media should autoplay based on cached settings value.
+     *
+     * @override
+     * @emits volume
+     * @return {Promise}
+     */
+    autoplay() {
+        const autoPlayPromise = this.mediaEl.play();
+
+        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
+            return autoPlayPromise
+                .then(() => {
+                    this.handleRate();
+                    this.handleVolume();
+                })
+                .catch(() => {
+                    // Auto-play was prevented, pause
+                    this.mediaEl.pause();
+                });
+        }
+
+        // Fallback to traditional autoplay tag if play does not return a promise
+        this.mediaEl.autoplay = true;
+        return Promise.resolve();
+    }
 }
 
 export default MP3Viewer;
diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index 0a823442e..8b2271467 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -317,15 +317,22 @@ class MediaBaseViewer extends BaseViewer {
         const autoPlayPromise = this.mediaEl.play();
 
         if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
-            this.handleRate();
             return autoPlayPromise
                 .then(() => {
+                    this.handleRate();
                     this.handleVolume();
                 })
                 .catch(() => {
                     // Auto-play was prevented, try muted play
                     this.setVolume(0);
-                    this.mediaEl.play();
+                    this.mediaEl
+                        .play()
+                        .then(() => {
+                            this.handleRate();
+                        })
+                        .catch(() => {
+                            this.mediaEl.pause();
+                        });
                 });
         }
 
diff --git a/src/lib/viewers/media/__tests__/MP3Viewer-test.js b/src/lib/viewers/media/__tests__/MP3Viewer-test.js
index e38edbc1e..4edf691bf 100644
--- a/src/lib/viewers/media/__tests__/MP3Viewer-test.js
+++ b/src/lib/viewers/media/__tests__/MP3Viewer-test.js
@@ -74,4 +74,17 @@ describe('lib/viewers/media/MP3Viewer', () => {
             mp3.loadUI();
         });
     });
+
+    describe('autoplay()', () => {
+        it('should pause autoplay if the promise is rejected', done => {
+            mp3.mediaEl = {
+                play: sandbox.stub().returns(Promise.reject()),
+                pause: sandbox.stub(),
+            };
+            mp3.autoplay().then(() => {
+                expect(mp3.mediaEl.pause).to.be.called;
+                done();
+            });
+        });
+    });
 });

From f5c829f17a75142f5c0adbb4d1ccbfbd19bcb6ec Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Wed, 11 Sep 2019 14:02:46 -0700
Subject: [PATCH 2/8] fix(viewer): clean up

---
 src/lib/viewers/media/MP3Viewer.js            | 17 +++-----
 src/lib/viewers/media/MediaBaseViewer.js      | 42 ++++---------------
 src/lib/viewers/media/VideoBaseViewer.js      | 27 ++++++++++++
 .../viewers/media/__tests__/MP3Viewer-test.js |  8 ++--
 .../media/__tests__/MediaBaseViewer-test.js   | 32 --------------
 .../media/__tests__/VideoBaseViewer-test.js   | 27 ++++++++++++
 6 files changed, 71 insertions(+), 82 deletions(-)

diff --git a/src/lib/viewers/media/MP3Viewer.js b/src/lib/viewers/media/MP3Viewer.js
index 393ddf042..59d279e83 100644
--- a/src/lib/viewers/media/MP3Viewer.js
+++ b/src/lib/viewers/media/MP3Viewer.js
@@ -36,25 +36,20 @@ class MP3Viewer extends MediaBaseViewer {
     }
 
     /**
-     * Determines if media should autoplay based on cached settings value.
+     * Autoplay the audio
      *
      * @override
      * @emits volume
      * @return {Promise}
      */
     autoplay() {
-        const autoPlayPromise = this.mediaEl.play();
+        // Play may return a promise depending on browser support. This promise
+        // will resolve when playback starts. If it fails, we pause the audio.
+        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
+        const autoPlayPromise = this.play();
 
         if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
-            return autoPlayPromise
-                .then(() => {
-                    this.handleRate();
-                    this.handleVolume();
-                })
-                .catch(() => {
-                    // Auto-play was prevented, pause
-                    this.mediaEl.pause();
-                });
+            return autoPlayPromise.catch(this.pause);
         }
 
         // Fallback to traditional autoplay tag if play does not return a promise
diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index 8b2271467..aa4f9d4e7 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -303,43 +303,14 @@ class MediaBaseViewer extends BaseViewer {
     }
 
     /**
-     * Determines if media should autoplay based on cached settings value.
+     * Autoplay the media
+     * Noop here and overrided in child class
      *
      * @private
      * @emits volume
      * @return {Promise}
      */
-    autoplay() {
-        // Play may return a promise depending on browser support. This promise
-        // will resolve when playback starts. If it fails, we mute the video
-        // and try to play again.
-        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
-        const autoPlayPromise = this.mediaEl.play();
-
-        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
-            return autoPlayPromise
-                .then(() => {
-                    this.handleRate();
-                    this.handleVolume();
-                })
-                .catch(() => {
-                    // Auto-play was prevented, try muted play
-                    this.setVolume(0);
-                    this.mediaEl
-                        .play()
-                        .then(() => {
-                            this.handleRate();
-                        })
-                        .catch(() => {
-                            this.mediaEl.pause();
-                        });
-                });
-        }
-
-        // Fallback to traditional autoplay tag if play does not return a promise
-        this.mediaEl.autoplay = true;
-        return Promise.resolve();
-    }
+    autoplay() {}
 
     /**
      * Determines if autoplay is enabled
@@ -577,7 +548,7 @@ class MediaBaseViewer extends BaseViewer {
      * @param {number} start - start time in seconds
      * @param {number} end - end time in seconds
      * @emits play
-     * @return {void}
+     * @return {Promise}
      */
     play(start, end) {
         const hasValidStart = this.isValidTime(start);
@@ -591,10 +562,13 @@ class MediaBaseViewer extends BaseViewer {
             this.setMediaTime(start);
         }
         if (arguments.length === 0 || hasValidStart) {
-            this.mediaEl.play();
+            const autoPlayPromise = this.mediaEl.play();
             this.handleRate();
             this.handleVolume();
+
+            return autoPlayPromise;
         }
+        return Promise.resolve();
     }
 
     /**
diff --git a/src/lib/viewers/media/VideoBaseViewer.js b/src/lib/viewers/media/VideoBaseViewer.js
index 9e57b2895..2cc43adb1 100644
--- a/src/lib/viewers/media/VideoBaseViewer.js
+++ b/src/lib/viewers/media/VideoBaseViewer.js
@@ -234,6 +234,33 @@ class VideoBaseViewer extends MediaBaseViewer {
     onKeydown(key) {
         return super.onKeydown(key);
     }
+
+    /**
+     * Autoplay the video
+     *
+     * @override
+     * @emits volume
+     * @return {Promise}
+     */
+    autoplay() {
+        // Play may return a promise depending on browser support. This promise
+        // will resolve when playback starts. If it fails, we mute the video
+        // and try to play again.
+        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
+        const autoPlayPromise = this.play();
+
+        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
+            return autoPlayPromise.catch(() => {
+                // Auto-play was prevented, try muted play
+                this.setVolume(0);
+                this.play().catch(this.pause);
+            });
+        }
+
+        // Fallback to traditional autoplay tag if play does not return a promise
+        this.mediaEl.autoplay = true;
+        return Promise.resolve();
+    }
 }
 
 export default VideoBaseViewer;
diff --git a/src/lib/viewers/media/__tests__/MP3Viewer-test.js b/src/lib/viewers/media/__tests__/MP3Viewer-test.js
index 4edf691bf..d0236af18 100644
--- a/src/lib/viewers/media/__tests__/MP3Viewer-test.js
+++ b/src/lib/viewers/media/__tests__/MP3Viewer-test.js
@@ -77,12 +77,10 @@ describe('lib/viewers/media/MP3Viewer', () => {
 
     describe('autoplay()', () => {
         it('should pause autoplay if the promise is rejected', done => {
-            mp3.mediaEl = {
-                play: sandbox.stub().returns(Promise.reject()),
-                pause: sandbox.stub(),
-            };
+            mp3.play = sandbox.stub().returns(Promise.reject());
+            sandbox.stub(mp3, 'pause');
             mp3.autoplay().then(() => {
-                expect(mp3.mediaEl.pause).to.be.called;
+                expect(mp3.pause).to.be.called;
                 done();
             });
         });
diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
index 21900ac75..1de0a24c3 100644
--- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
@@ -271,38 +271,6 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
         });
     });
 
-    describe('autoplay()', () => {
-        beforeEach(() => {
-            media.mediaEl = {
-                play: sandbox.stub().returns(Promise.resolve()),
-            };
-
-            sandbox.stub(media, 'isAutoplayEnabled').returns(true);
-            sandbox.stub(Browser, 'isIOS').returns(false);
-        });
-
-        it('should set autoplay if setting is enabled and handle the promise if it is a valid promise', () => {
-            media.autoplay();
-            expect(media.mediaEl.play).to.be.called;
-            expect(media.mediaEl.autoplay).to.be.undefined;
-        });
-
-        it('should set autoplay to true if play does not return a promise', () => {
-            media.mediaEl.play.returns(undefined);
-            media.autoplay();
-            expect(media.mediaEl.autoplay).to.be.true;
-        });
-
-        it('should muted autoplay if the promise is rejected', done => {
-            sandbox.stub(media, 'setVolume');
-            media.mediaEl.play = sandbox.stub().returns(Promise.reject());
-            media.autoplay().then(() => {
-                expect(media.setVolume).to.be.calledWith(0);
-                done();
-            });
-        });
-    });
-
     describe('loadUI()', () => {
         it('should set up media controls and element', () => {
             const duration = 10;
diff --git a/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js b/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js
index ff32c51bc..8490b08ea 100644
--- a/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js
@@ -244,4 +244,31 @@ describe('lib/viewers/media/VideoBaseViewer', () => {
             expect(videoBase.rootEl.classList.contains('bp-dark')).to.be.true;
         });
     });
+
+    describe('autoplay()', () => {
+        beforeEach(() => {
+            videoBase.play = sandbox.stub().returns(Promise.resolve());
+        });
+
+        it('should set autoplay if setting is enabled and handle the promise if it is a valid promise', () => {
+            videoBase.autoplay();
+            expect(videoBase.play).to.be.called;
+            expect(videoBase.mediaEl.autoplay).to.be.undefined;
+        });
+
+        it('should set autoplay to true if play does not return a promise', () => {
+            videoBase.play.returns(undefined);
+            videoBase.autoplay();
+            expect(videoBase.mediaEl.autoplay).to.be.true;
+        });
+
+        it('should muted autoplay if the promise is rejected', done => {
+            sandbox.stub(videoBase, 'setVolume');
+            videoBase.play.returns(Promise.reject());
+            videoBase.autoplay().then(() => {
+                expect(videoBase.setVolume).to.be.calledWith(0);
+                done();
+            });
+        });
+    });
 });

From 56284b41d84aa8c5b98a5dd0394ad83ecb90322f Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Wed, 11 Sep 2019 15:47:21 -0700
Subject: [PATCH 3/8] fix(viewer): small change

---
 src/lib/viewers/media/MediaBaseViewer.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index aa4f9d4e7..8ab0e2c51 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -562,11 +562,11 @@ class MediaBaseViewer extends BaseViewer {
             this.setMediaTime(start);
         }
         if (arguments.length === 0 || hasValidStart) {
-            const autoPlayPromise = this.mediaEl.play();
+            const playPromise = this.mediaEl.play();
             this.handleRate();
             this.handleVolume();
 
-            return autoPlayPromise;
+            return playPromise;
         }
         return Promise.resolve();
     }

From 5af4d6dc3bcf13420fb3d7736481b6dc2857ec23 Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Thu, 12 Sep 2019 16:02:54 -0700
Subject: [PATCH 4/8] fix(viewer): avoid duplicating autoplay logic

---
 src/lib/viewers/media/MP3Viewer.js            | 21 +++-----------
 src/lib/viewers/media/MediaBaseViewer.js      | 24 +++++++++++++--
 src/lib/viewers/media/VideoBaseViewer.js      | 27 ++++-------------
 .../viewers/media/__tests__/MP3Viewer-test.js | 11 +++----
 .../media/__tests__/MediaBaseViewer-test.js   | 28 ++++++++++++++++++
 .../media/__tests__/VideoBaseViewer-test.js   | 29 +++++--------------
 6 files changed, 70 insertions(+), 70 deletions(-)

diff --git a/src/lib/viewers/media/MP3Viewer.js b/src/lib/viewers/media/MP3Viewer.js
index 59d279e83..9bdcd4047 100644
--- a/src/lib/viewers/media/MP3Viewer.js
+++ b/src/lib/viewers/media/MP3Viewer.js
@@ -36,26 +36,13 @@ class MP3Viewer extends MediaBaseViewer {
     }
 
     /**
-     * Autoplay the audio
+     * Auto-play was prevented, pause the audio
      *
      * @override
-     * @emits volume
-     * @return {Promise}
      */
-    autoplay() {
-        // Play may return a promise depending on browser support. This promise
-        // will resolve when playback starts. If it fails, we pause the audio.
-        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
-        const autoPlayPromise = this.play();
-
-        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
-            return autoPlayPromise.catch(this.pause);
-        }
-
-        // Fallback to traditional autoplay tag if play does not return a promise
-        this.mediaEl.autoplay = true;
-        return Promise.resolve();
-    }
+    handleAutoplayFail = () => {
+        this.pause();
+    };
 }
 
 export default MP3Viewer;
diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index 8ab0e2c51..801a9c2f6 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -302,15 +302,35 @@ class MediaBaseViewer extends BaseViewer {
         this.emit('autoplay', this.isAutoplayEnabled());
     }
 
+    /**
+     * Handler for autoplay failure
+     * overrided in child class
+     *
+     * @private
+     */
+    handleAutoplayFail = () => {};
+
     /**
      * Autoplay the media
-     * Noop here and overrided in child class
      *
      * @private
      * @emits volume
      * @return {Promise}
      */
-    autoplay() {}
+    autoplay() {
+        // Play may return a promise depending on browser support. This promise
+        // will resolve when playback starts.
+        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
+        const autoPlayPromise = this.play();
+
+        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
+            return autoPlayPromise.catch(this.handleAutoplayFail);
+        }
+
+        // Fallback to traditional autoplay tag if play does not return a promise
+        this.mediaEl.autoplay = true;
+        return Promise.resolve();
+    }
 
     /**
      * Determines if autoplay is enabled
diff --git a/src/lib/viewers/media/VideoBaseViewer.js b/src/lib/viewers/media/VideoBaseViewer.js
index 2cc43adb1..543fa48a7 100644
--- a/src/lib/viewers/media/VideoBaseViewer.js
+++ b/src/lib/viewers/media/VideoBaseViewer.js
@@ -236,31 +236,14 @@ class VideoBaseViewer extends MediaBaseViewer {
     }
 
     /**
-     * Autoplay the video
+     * Auto-play was prevented, try muted play
      *
      * @override
-     * @emits volume
-     * @return {Promise}
      */
-    autoplay() {
-        // Play may return a promise depending on browser support. This promise
-        // will resolve when playback starts. If it fails, we mute the video
-        // and try to play again.
-        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
-        const autoPlayPromise = this.play();
-
-        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
-            return autoPlayPromise.catch(() => {
-                // Auto-play was prevented, try muted play
-                this.setVolume(0);
-                this.play().catch(this.pause);
-            });
-        }
-
-        // Fallback to traditional autoplay tag if play does not return a promise
-        this.mediaEl.autoplay = true;
-        return Promise.resolve();
-    }
+    handleAutoplayFail = () => {
+        this.setVolume(0);
+        this.play().catch(this.pause);
+    };
 }
 
 export default VideoBaseViewer;
diff --git a/src/lib/viewers/media/__tests__/MP3Viewer-test.js b/src/lib/viewers/media/__tests__/MP3Viewer-test.js
index d0236af18..e8ee67d85 100644
--- a/src/lib/viewers/media/__tests__/MP3Viewer-test.js
+++ b/src/lib/viewers/media/__tests__/MP3Viewer-test.js
@@ -75,14 +75,11 @@ describe('lib/viewers/media/MP3Viewer', () => {
         });
     });
 
-    describe('autoplay()', () => {
-        it('should pause autoplay if the promise is rejected', done => {
-            mp3.play = sandbox.stub().returns(Promise.reject());
+    describe('handleAutoplayFail()', () => {
+        it('should call pause', () => {
             sandbox.stub(mp3, 'pause');
-            mp3.autoplay().then(() => {
-                expect(mp3.pause).to.be.called;
-                done();
-            });
+            mp3.handleAutoplayFail();
+            expect(mp3.pause).to.be.called;
         });
     });
 });
diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
index 1de0a24c3..f8f96c5f8 100644
--- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
@@ -271,6 +271,34 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
         });
     });
 
+    describe('autoplay()', () => {
+        beforeEach(() => {
+            media.mediaEl = {};
+            media.play = sandbox.stub().returns(Promise.resolve());
+        });
+
+        it('should set autoplay if setting is enabled and handle the promise if it is a valid promise', () => {
+            media.autoplay();
+            expect(media.play).to.be.called;
+            expect(media.mediaEl.autoplay).to.be.undefined;
+        });
+
+        it('should set autoplay to true if play does not return a promise', () => {
+            media.play.returns(undefined);
+            media.autoplay();
+            expect(media.mediaEl.autoplay).to.be.true;
+        });
+
+        it('should call handleAutoplayFail if the promise is rejected', done => {
+            sandbox.stub(media, 'handleAutoplayFail');
+            media.play.returns(Promise.reject());
+            media.autoplay().then(() => {
+                expect(media.handleAutoplayFail).to.be.called;
+                done();
+            });
+        });
+    });
+
     describe('loadUI()', () => {
         it('should set up media controls and element', () => {
             const duration = 10;
diff --git a/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js b/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js
index 8490b08ea..3ffd111bf 100644
--- a/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/VideoBaseViewer-test.js
@@ -245,30 +245,15 @@ describe('lib/viewers/media/VideoBaseViewer', () => {
         });
     });
 
-    describe('autoplay()', () => {
-        beforeEach(() => {
-            videoBase.play = sandbox.stub().returns(Promise.resolve());
-        });
+    describe('handleAutoplayFail()', () => {
+        it('should mute and play again', () => {
+            sandbox.stub(videoBase, 'setVolume');
+            videoBase.play = sandbox.stub().returns(Promise.reject());
 
-        it('should set autoplay if setting is enabled and handle the promise if it is a valid promise', () => {
-            videoBase.autoplay();
-            expect(videoBase.play).to.be.called;
-            expect(videoBase.mediaEl.autoplay).to.be.undefined;
-        });
+            videoBase.handleAutoplayFail();
 
-        it('should set autoplay to true if play does not return a promise', () => {
-            videoBase.play.returns(undefined);
-            videoBase.autoplay();
-            expect(videoBase.mediaEl.autoplay).to.be.true;
-        });
-
-        it('should muted autoplay if the promise is rejected', done => {
-            sandbox.stub(videoBase, 'setVolume');
-            videoBase.play.returns(Promise.reject());
-            videoBase.autoplay().then(() => {
-                expect(videoBase.setVolume).to.be.calledWith(0);
-                done();
-            });
+            expect(videoBase.setVolume).to.be.calledWith(0);
+            expect(videoBase.play).to.be.called;
         });
     });
 });

From 0ebf7030817834383ce9f0a95a8c8e4d4769eb53 Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Thu, 12 Sep 2019 19:07:18 -0700
Subject: [PATCH 5/8] fix(viewer): fix typo

---
 src/lib/viewers/media/MediaBaseViewer.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index 801a9c2f6..add53079e 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -304,9 +304,9 @@ class MediaBaseViewer extends BaseViewer {
 
     /**
      * Handler for autoplay failure
-     * overrided in child class
+     * Overridden in child class
      *
-     * @private
+     * @protected
      */
     handleAutoplayFail = () => {};
 

From df6e0f7ad25771835d4da6eb4ee63c83bb37a007 Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Fri, 13 Sep 2019 14:44:23 -0700
Subject: [PATCH 6/8] perf(sidebar): force play to always return a promise

---
 src/lib/viewers/media/MediaBaseViewer.js      | 34 +++++++++++--------
 .../media/__tests__/MediaBaseViewer-test.js   | 18 +++++-----
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index add53079e..14a78ff03 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -19,6 +19,7 @@ const TIMESTAMP_UNIT_NAME = 'timestamp';
 const INITIAL_TIME_IN_SECONDS = 0;
 const ONE_MINUTE_IN_SECONDS = 60;
 const ONE_HOUR_IN_SECONDS = 60 * ONE_MINUTE_IN_SECONDS;
+const ERROR_BROWSER_NOT_SUPPORT = 'browsernotsupport';
 
 class MediaBaseViewer extends BaseViewer {
     /**
@@ -315,21 +316,19 @@ class MediaBaseViewer extends BaseViewer {
      *
      * @private
      * @emits volume
-     * @return {Promise}
+     * @return {void}
      */
-    autoplay() {
-        // Play may return a promise depending on browser support. This promise
-        // will resolve when playback starts.
-        // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
-        const autoPlayPromise = this.play();
-
-        if (autoPlayPromise && typeof autoPlayPromise.then === 'function') {
-            return autoPlayPromise.catch(this.handleAutoplayFail);
+    async autoplay() {
+        try {
+            await this.play();
+        } catch (error) {
+            if (error.message === ERROR_BROWSER_NOT_SUPPORT) {
+                // Fallback to traditional autoplay tag if mediaEl.play does not return a promise
+                this.mediaEl.autoplay = true;
+            } else {
+                this.handleAutoplayFail();
+            }
         }
-
-        // Fallback to traditional autoplay tag if play does not return a promise
-        this.mediaEl.autoplay = true;
-        return Promise.resolve();
     }
 
     /**
@@ -582,11 +581,18 @@ class MediaBaseViewer extends BaseViewer {
             this.setMediaTime(start);
         }
         if (arguments.length === 0 || hasValidStart) {
+            // Play may return a promise depending on browser support. This promise
+            // will resolve when playback starts.
+            // https://webkit.org/blog/7734/auto-play-policy-changes-for-macos/
             const playPromise = this.mediaEl.play();
             this.handleRate();
             this.handleVolume();
 
-            return playPromise;
+            if (playPromise && typeof playPromise.then === 'function') {
+                return playPromise;
+            }
+
+            return Promise.reject(new Error(ERROR_BROWSER_NOT_SUPPORT));
         }
         return Promise.resolve();
     }
diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
index f8f96c5f8..15b46c2e1 100644
--- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
@@ -272,6 +272,8 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
     });
 
     describe('autoplay()', () => {
+        const ERROR_BROWSER_NOT_SUPPORT = 'browsernotsupport';
+
         beforeEach(() => {
             media.mediaEl = {};
             media.play = sandbox.stub().returns(Promise.resolve());
@@ -283,19 +285,17 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
             expect(media.mediaEl.autoplay).to.be.undefined;
         });
 
-        it('should set autoplay to true if play does not return a promise', () => {
-            media.play.returns(undefined);
-            media.autoplay();
+        it('should set autoplay to true if mediaEl.play does not return a promise', async () => {
+            media.play.returns(Promise.reject(new Error(ERROR_BROWSER_NOT_SUPPORT)));
+            await media.autoplay();
             expect(media.mediaEl.autoplay).to.be.true;
         });
 
-        it('should call handleAutoplayFail if the promise is rejected', done => {
+        it('should call handleAutoplayFail if the promise is rejected', async () => {
             sandbox.stub(media, 'handleAutoplayFail');
-            media.play.returns(Promise.reject());
-            media.autoplay().then(() => {
-                expect(media.handleAutoplayFail).to.be.called;
-                done();
-            });
+            media.play.returns(Promise.reject(new Error('NotAllowedError')));
+            await media.autoplay();
+            expect(media.handleAutoplayFail).to.be.called;
         });
     });
 

From 5ebe8b013a83aa5be11547c6ec6941aa4f81e07f Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Fri, 13 Sep 2019 15:32:57 -0700
Subject: [PATCH 7/8] perf(sidebar): remove await async

---
 src/lib/viewers/media/MediaBaseViewer.js         | 10 ++++------
 .../media/__tests__/MediaBaseViewer-test.js      | 16 ++++++++++------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index 14a78ff03..30ef6144e 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -316,19 +316,17 @@ class MediaBaseViewer extends BaseViewer {
      *
      * @private
      * @emits volume
-     * @return {void}
+     * @return {Promise}
      */
-    async autoplay() {
-        try {
-            await this.play();
-        } catch (error) {
+    autoplay() {
+        return this.play().catch(error => {
             if (error.message === ERROR_BROWSER_NOT_SUPPORT) {
                 // Fallback to traditional autoplay tag if mediaEl.play does not return a promise
                 this.mediaEl.autoplay = true;
             } else {
                 this.handleAutoplayFail();
             }
-        }
+        });
     }
 
     /**
diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
index 15b46c2e1..ceb0dc8d6 100644
--- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
@@ -285,17 +285,21 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
             expect(media.mediaEl.autoplay).to.be.undefined;
         });
 
-        it('should set autoplay to true if mediaEl.play does not return a promise', async () => {
+        it('should set autoplay to true if mediaEl.play does not return a promise', done => {
             media.play.returns(Promise.reject(new Error(ERROR_BROWSER_NOT_SUPPORT)));
-            await media.autoplay();
-            expect(media.mediaEl.autoplay).to.be.true;
+            media.autoplay().then(() => {
+                expect(media.mediaEl.autoplay).to.be.true;
+                done();
+            });
         });
 
-        it('should call handleAutoplayFail if the promise is rejected', async () => {
+        it('should call handleAutoplayFail if the promise is rejected', done => {
             sandbox.stub(media, 'handleAutoplayFail');
             media.play.returns(Promise.reject(new Error('NotAllowedError')));
-            await media.autoplay();
-            expect(media.handleAutoplayFail).to.be.called;
+            media.autoplay().then(() => {
+                expect(media.handleAutoplayFail).to.be.called;
+                done();
+            });
         });
     });
 

From 0e569796e47a643e04a37d738f9abd3abf8b4b77 Mon Sep 17 00:00:00 2001
From: Mingze Xiao <mxiao@box.com>
Date: Tue, 17 Sep 2019 11:02:53 -0700
Subject: [PATCH 8/8] perf(sidebar): minor changes

---
 src/lib/viewers/media/MediaBaseViewer.js            | 13 +++++--------
 .../viewers/media/__tests__/MediaBaseViewer-test.js |  4 ++--
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/lib/viewers/media/MediaBaseViewer.js b/src/lib/viewers/media/MediaBaseViewer.js
index 30ef6144e..0d2f48df7 100644
--- a/src/lib/viewers/media/MediaBaseViewer.js
+++ b/src/lib/viewers/media/MediaBaseViewer.js
@@ -19,7 +19,7 @@ const TIMESTAMP_UNIT_NAME = 'timestamp';
 const INITIAL_TIME_IN_SECONDS = 0;
 const ONE_MINUTE_IN_SECONDS = 60;
 const ONE_HOUR_IN_SECONDS = 60 * ONE_MINUTE_IN_SECONDS;
-const ERROR_BROWSER_NOT_SUPPORT = 'browsernotsupport';
+const PLAY_PROMISE_NOT_SUPPORTED = 'play_promise_not_supported';
 
 class MediaBaseViewer extends BaseViewer {
     /**
@@ -315,12 +315,11 @@ class MediaBaseViewer extends BaseViewer {
      * Autoplay the media
      *
      * @private
-     * @emits volume
      * @return {Promise}
      */
     autoplay() {
         return this.play().catch(error => {
-            if (error.message === ERROR_BROWSER_NOT_SUPPORT) {
+            if (error.message === PLAY_PROMISE_NOT_SUPPORTED) {
                 // Fallback to traditional autoplay tag if mediaEl.play does not return a promise
                 this.mediaEl.autoplay = true;
             } else {
@@ -586,11 +585,9 @@ class MediaBaseViewer extends BaseViewer {
             this.handleRate();
             this.handleVolume();
 
-            if (playPromise && typeof playPromise.then === 'function') {
-                return playPromise;
-            }
-
-            return Promise.reject(new Error(ERROR_BROWSER_NOT_SUPPORT));
+            return playPromise && typeof playPromise.then === 'function'
+                ? playPromise
+                : Promise.reject(new Error(PLAY_PROMISE_NOT_SUPPORTED));
         }
         return Promise.resolve();
     }
diff --git a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
index ceb0dc8d6..6216d8bbb 100644
--- a/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
+++ b/src/lib/viewers/media/__tests__/MediaBaseViewer-test.js
@@ -272,7 +272,7 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
     });
 
     describe('autoplay()', () => {
-        const ERROR_BROWSER_NOT_SUPPORT = 'browsernotsupport';
+        const PLAY_PROMISE_NOT_SUPPORTED = 'play_promise_not_supported';
 
         beforeEach(() => {
             media.mediaEl = {};
@@ -286,7 +286,7 @@ describe('lib/viewers/media/MediaBaseViewer', () => {
         });
 
         it('should set autoplay to true if mediaEl.play does not return a promise', done => {
-            media.play.returns(Promise.reject(new Error(ERROR_BROWSER_NOT_SUPPORT)));
+            media.play.returns(Promise.reject(new Error(PLAY_PROMISE_NOT_SUPPORTED)));
             media.autoplay().then(() => {
                 expect(media.mediaEl.autoplay).to.be.true;
                 done();