Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r?gijs draft
authorJared Wein <jwein@mozilla.com>
Thu, 13 Oct 2016 16:12:50 -0400
changeset 427073 230bacc0a829e0a187d0fd3287c990d18082aa5a
parent 424919 83461556cda59e8bcd54f09b9263e3202ce01dc2
child 534362 10eba6def9d1b1e27030b5257971122d8701075b
push id32904
push userjwein@mozilla.com
push dateWed, 19 Oct 2016 16:57:11 +0000
reviewersgijs
bugs1232357
milestone52.0a1
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r?gijs MozReview-Commit-ID: BNaVgrX6jsv
browser/app/profile/firefox.js
browser/base/content/tabbrowser.xml
browser/base/content/test/general/browser_audioTabIcon.js
testing/profiles/prefs_general.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -431,16 +431,18 @@ pref("browser.tabs.drawInTitlebar", true
 // When tabs opened by links in other tabs via a combination of
 // browser.link.open_newwindow being set to 3 and target="_blank" etc are
 // closed:
 // true   return to the tab that opened this tab (its owner)
 // false  return to the adjacent tab (old default)
 pref("browser.tabs.selectOwnerOnClose", true);
 
 pref("browser.tabs.showAudioPlayingIcon", true);
+// This should match Chromium's audio indicator delay.
+pref("browser.tabs.delayHidingAudioPlayingIconMS", 3000);
 
 pref("browser.tabs.dontfocusfordialogs", true);
 
 pref("browser.ctrlTab.previews", false);
 
 // By default, do not export HTML at shutdown.
 // If true, at shutdown the bookmarks in your menu and toolbar will
 // be exported as HTML to the bookmarks.html file.
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -760,16 +760,18 @@
                     ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
                      aLocation.spec != "about:blank") ||
                     (isSameDocument && this.mBrowser.inLoadURI)) {
                   this.mBrowser.userTypedValue = null;
                 }
 
                 // If the browser was playing audio, we should remove the playing state.
                 if (this.mTab.hasAttribute("soundplaying") && !isSameDocument) {
+                  clearTimeout(this.mTab._soundPlayingAttrRemovalTimer);
+                  this.mTab._soundPlayingAttrRemovalTimer = 0;
                   this.mTab.removeAttribute("soundplaying");
                   this.mTabBrowser._tabAttrModified(this.mTab, ["soundplaying"]);
                 }
 
                 // If the browser was previously muted, we should restore the muted state.
                 if (this.mTab.hasAttribute("muted")) {
                   this.mTab.linkedBrowser.mute();
                 }
@@ -2711,16 +2713,24 @@
             if (otherBrowser.hasAttribute("usercontextid")) {
               ourBrowser.setAttribute("usercontextid", otherBrowser.getAttribute("usercontextid"));
             }
 
             // That's gBrowser for the other window, not the tab's browser!
             var remoteBrowser = aOtherTab.ownerDocument.defaultView.gBrowser;
             var isPending = aOtherTab.hasAttribute("pending");
 
+            // Expedite the removal of the icon if it was already scheduled.
+            if (aOtherTab._soundPlayingAttrRemovalTimer) {
+              clearTimeout(aOtherTab._soundPlayingAttrRemovalTimer);
+              aOtherTab._soundPlayingAttrRemovalTimer = 0;
+              aOtherTab.removeAttribute("soundplaying");
+              remoteBrowser._tabAttrModified(aOtherTab, ["soundplaying"]);
+            }
+
             // First, start teardown of the other browser.  Make sure to not
             // fire the beforeunload event in the process.  Close the other
             // window if this was its last tab.
             if (!remoteBrowser._beginRemoveTab(aOtherTab, aOurTab, true))
               return;
 
             let modifiedAttrs = [];
             if (aOtherTab.hasAttribute("muted")) {
@@ -4863,16 +4873,17 @@
             },
             // Also support adding event listeners (forward to the tab container)
             addEventListener: function (a, b, c) { this.self.tabContainer.addEventListener(a, b, c); },
             removeEventListener: function (a, b, c) { this.self.tabContainer.removeEventListener(a, b, c); }
           });
         ]]>
         </getter>
       </property>
+      <field name="_soundPlayingAttrRemovalTimer">0</field>
     </implementation>
 
     <handlers>
       <handler event="DOMWindowClose" phase="capturing">
         <![CDATA[
           if (!event.isTrusted)
             return;
 
@@ -4993,16 +5004,19 @@
               !event.isTrusted)
             return;
 
           var browser = event.originalTarget;
           var tab = this.getTabForBrowser(browser);
           if (!tab)
             return;
 
+          clearTimeout(tab._soundPlayingAttrRemovalTimer);
+          tab._soundPlayingAttrRemovalTimer = 0;
+
           if (!tab.hasAttribute("soundplaying")) {
             tab.setAttribute("soundplaying", true);
             this._tabAttrModified(tab, ["soundplaying"]);
           }
         ]]>
       </handler>
       <handler event="DOMAudioPlaybackStopped">
         <![CDATA[
@@ -5011,18 +5025,21 @@
             return;
 
           var browser = event.originalTarget;
           var tab = this.getTabForBrowser(browser);
           if (!tab)
             return;
 
           if (tab.hasAttribute("soundplaying")) {
-            tab.removeAttribute("soundplaying");
-            this._tabAttrModified(tab, ["soundplaying"]);
+            let removalDelay = Services.prefs.getIntPref("browser.tabs.delayHidingAudioPlayingIconMS");
+            tab._soundPlayingAttrRemovalTimer = setTimeout(() => {
+              tab.removeAttribute("soundplaying");
+              this._tabAttrModified(tab, ["soundplaying"]);
+            }, removalDelay);
           }
         ]]>
       </handler>
     </handlers>
   </binding>
 
   <binding id="tabbrowser-tabbox"
            extends="chrome://global/content/bindings/tabbox.xml#tabbox">
--- a/browser/base/content/test/general/browser_audioTabIcon.js
+++ b/browser/base/content/test/general/browser_audioTabIcon.js
@@ -1,17 +1,19 @@
 const PAGE = "https://example.com/browser/browser/base/content/test/general/file_mediaPlayback.html";
+const TABATTR_REMOVAL_PREFNAME = "browser.tabs.delayHidingAudioPlayingIconMS";
+const INITIAL_TABATTR_REMOVAL_DELAY_MS = Services.prefs.getIntPref(TABATTR_REMOVAL_PREFNAME);
 
 function* wait_for_tab_playing_event(tab, expectPlaying) {
   if (tab.soundPlaying == expectPlaying) {
     ok(true, "The tab should " + (expectPlaying ? "" : "not ") + "be playing");
     return true;
   }
   return yield BrowserTestUtils.waitForEvent(tab, "TabAttrModified", false, (event) => {
-    if (event.detail.changed.indexOf("soundplaying") >= 0) {
+    if (event.detail.changed.includes("soundplaying")) {
       is(tab.hasAttribute("soundplaying"), expectPlaying, "The tab should " + (expectPlaying ? "" : "not ") + "be playing");
       is(tab.soundPlaying, expectPlaying, "The tab should " + (expectPlaying ? "" : "not ") + "be playing");
       return true;
     }
     return false;
   });
 }
 
@@ -20,24 +22,51 @@ function* play(tab) {
   yield ContentTask.spawn(browser, {}, function* () {
     let audio = content.document.querySelector("audio");
     audio.play();
   });
 
   yield wait_for_tab_playing_event(tab, true);
 }
 
-function* pause(tab) {
-  let browser = tab.linkedBrowser;
-  yield ContentTask.spawn(browser, {}, function* () {
-    let audio = content.document.querySelector("audio");
-    audio.pause();
-  });
+function* pause(tab, options) {
+  ok(tab.hasAttribute("soundplaying"), "The tab should have the soundplaying attribute when pause() is called");
+
+  let extendedDelay = options && options.extendedDelay;
+  if (extendedDelay) {
+    // Use 10s to remove possibility of race condition with attr removal.
+    Services.prefs.setIntPref(TABATTR_REMOVAL_PREFNAME, 10000);
+  }
 
-  yield wait_for_tab_playing_event(tab, false);
+  try {
+    let browser = tab.linkedBrowser;
+    let awaitDOMAudioPlaybackStopped =
+      BrowserTestUtils.waitForEvent(browser, "DOMAudioPlaybackStopped", "DOMAudioPlaybackStopped event should get fired after pause");
+    let awaitTabPausedAttrModified =
+      wait_for_tab_playing_event(tab, false);
+    yield ContentTask.spawn(browser, {}, function* () {
+      let audio = content.document.querySelector("audio");
+      audio.pause();
+    });
+
+    if (extendedDelay) {
+      ok(tab.hasAttribute("soundplaying"), "The tab should still have the soundplaying attribute immediately after pausing");
+
+      yield awaitDOMAudioPlaybackStopped;
+      ok(tab.hasAttribute("soundplaying"), "The tab should still have the soundplaying attribute immediately after DOMAudioPlaybackStopped");
+    }
+
+    yield awaitTabPausedAttrModified;
+    ok(!tab.hasAttribute("soundplaying"), "The tab should not have the soundplaying attribute after the timeout has resolved");
+  } finally {
+    // Make sure other tests don't timeout if an exception gets thrown above.
+    // Need to use setIntPref instead of clearUserPref because prefs_general.js
+    // overrides the default value to help this and other tests run faster.
+    Services.prefs.setIntPref(TABATTR_REMOVAL_PREFNAME, INITIAL_TABATTR_REMOVAL_DELAY_MS);
+  }
 }
 
 function disable_non_test_mouse(disable) {
   let utils = window.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIDOMWindowUtils);
   utils.disableNonTestMouseEvents(disable);
 }
 
@@ -78,17 +107,17 @@ function* test_tooltip(icon, expectedToo
 }
 
 // The set of tabs which have ever had their mute state changed.
 // Used to determine whether the tab should have a muteReason value.
 let everMutedTabs = new WeakSet();
 
 function get_wait_for_mute_promise(tab, expectMuted) {
   return BrowserTestUtils.waitForEvent(tab, "TabAttrModified", false, event => {
-    if (event.detail.changed.indexOf("muted") >= 0) {
+    if (event.detail.changed.includes("muted")) {
       is(tab.hasAttribute("muted"), expectMuted, "The tab should " + (expectMuted ? "" : "not ") + "be muted");
       is(tab.muted, expectMuted, "The tab muted property " + (expectMuted ? "" : "not ") + "be true");
 
       if (expectMuted || everMutedTabs.has(tab)) {
         everMutedTabs.add(tab);
         is(tab.muteReason, null, "The tab should have a null muteReason value");
       } else {
         is(tab.muteReason, undefined, "The tab should have an undefined muteReason value");
@@ -205,55 +234,40 @@ function* test_playing_icon_on_tab(tab, 
 
 function* test_swapped_browser_while_playing(oldTab, newBrowser) {
   ok(oldTab.hasAttribute("muted"), "Expected the correct muted attribute on the old tab");
   is(oldTab.muteReason, null, "Expected the correct muteReason attribute on the old tab");
   ok(oldTab.hasAttribute("soundplaying"), "Expected the correct soundplaying attribute on the old tab");
 
   let newTab = gBrowser.getTabForBrowser(newBrowser);
   let AttrChangePromise = BrowserTestUtils.waitForEvent(newTab, "TabAttrModified", false, event => {
-    return event.detail.changed.indexOf("soundplaying") >= 0 &&
-           event.detail.changed.indexOf("muted") >= 0;
+    return event.detail.changed.includes("soundplaying") &&
+           event.detail.changed.includes("muted");
   });
 
   gBrowser.swapBrowsersAndCloseOther(newTab, oldTab);
   yield AttrChangePromise;
 
   ok(newTab.hasAttribute("muted"), "Expected the correct muted attribute on the new tab");
   is(newTab.muteReason, null, "Expected the correct muteReason property on the new tab");
   ok(newTab.hasAttribute("soundplaying"), "Expected the correct soundplaying attribute on the new tab");
 
-  let receivedSoundPlaying = 0;
-  // We need to receive two TabAttrModified events with 'soundplaying'
-  // because swapBrowsersAndCloseOther involves nsDocument::OnPageHide and
-  // nsDocument::OnPageShow. Each methods lead to TabAttrModified event.
-  yield BrowserTestUtils.waitForEvent(newTab, "TabAttrModified", false, event => {
-    if (event.detail.changed.indexOf("soundplaying") >= 0) {
-      return (++receivedSoundPlaying == 2);
-    }
-    return false;
-  });
-
-  ok(newTab.hasAttribute("muted"), "Expected the correct muted attribute on the new tab");
-  is(newTab.muteReason, null, "Expected the correct muteReason property on the new tab");
-  ok(newTab.hasAttribute("soundplaying"), "Expected the correct soundplaying attribute on the new tab");
-
   let icon = document.getAnonymousElementByAttribute(newTab, "anonid",
                                                      "soundplaying-icon");
   yield test_tooltip(icon, "Unmute tab", true);
 }
 
 function* test_swapped_browser_while_not_playing(oldTab, newBrowser) {
   ok(oldTab.hasAttribute("muted"), "Expected the correct muted attribute on the old tab");
   is(oldTab.muteReason, null, "Expected the correct muteReason property on the old tab");
   ok(!oldTab.hasAttribute("soundplaying"), "Expected the correct soundplaying attribute on the old tab");
 
   let newTab = gBrowser.getTabForBrowser(newBrowser);
   let AttrChangePromise = BrowserTestUtils.waitForEvent(newTab, "TabAttrModified", false, event => {
-    return event.detail.changed.indexOf("muted") >= 0;
+    return event.detail.changed.includes("muted");
   });
 
   let AudioPlaybackPromise = new Promise(resolve => {
     let observer = (subject, topic, data) => {
       ok(false, "Should not see an audio-playback notification");
     };
     Services.obs.addObserver(observer, "audiochannel-activity-normal", false);
     setTimeout(() => {
@@ -354,17 +368,17 @@ function* test_click_on_pinned_tab_after
 function* test_cross_process_load() {
   function* test_on_browser(browser) {
     let tab = gBrowser.getTabForBrowser(browser);
 
     //   Start playback and wait for it to finish.
     yield play(tab);
 
     let soundPlayingStoppedPromise = BrowserTestUtils.waitForEvent(tab, "TabAttrModified", false,
-      event => event.detail.changed.indexOf("soundplaying") >= 0
+      event => event.detail.changed.includes("soundplaying")
     );
 
     // Go to a different process.
     browser.loadURI("about:");
     yield BrowserTestUtils.browserLoaded(browser);
 
     yield soundPlayingStoppedPromise;
 
@@ -442,16 +456,34 @@ function* test_on_browser(browser) {
       gBrowser,
       url: "data:text/html,test"
     }, () => test_on_browser(browser));
   } else {
     yield test_browser_swapping(tab, browser);
   }
 }
 
+function* test_delayed_tabattr_removal() {
+  function* test_on_browser(browser) {
+    let tab = gBrowser.getTabForBrowser(browser);
+    yield play(tab);
+
+    // Extend the delay to guarantee the soundplaying attribute
+    // is not removed from the tab when audio is stopped. Without
+    // the extended delay the attribute could be removed in the
+    // same tick and the test wouldn't catch that this broke.
+    yield pause(tab, {extendedDelay: true});
+  }
+
+  yield BrowserTestUtils.withNewTab({
+    gBrowser,
+    url: PAGE
+  }, test_on_browser);
+}
+
 add_task(function*() {
   yield new Promise((resolve) => {
     SpecialPowers.pushPrefEnv({"set": [
                                 ["browser.tabs.showAudioPlayingIcon", true],
                               ]}, resolve);
   });
 });
 
@@ -463,8 +495,10 @@ add_task(function* test_page() {
   }, test_on_browser);
 });
 
 add_task(test_click_on_pinned_tab_after_mute);
 
 add_task(test_cross_process_load);
 
 add_task(test_mute_keybinding);
+
+add_task(test_delayed_tabattr_removal);
--- a/testing/profiles/prefs_general.js
+++ b/testing/profiles/prefs_general.js
@@ -317,16 +317,18 @@ user_pref("media.autoplay.enabled", true
 
 #if defined(XP_WIN)
 user_pref("media.decoder.heuristic.dormant.timeout", 0);
 #endif
 
 // Don't use auto-enabled e10s
 user_pref("browser.tabs.remote.autostart.1", false);
 user_pref("browser.tabs.remote.autostart.2", false);
+// Don't show a delay when hiding the audio indicator during tests
+user_pref("browser.tabs.delayHidingAudioPlayingIconMS", 0);
 // Don't forceably kill content processes after a timeout
 user_pref("dom.ipc.tabs.shutdownTimeoutSecs", 0);
 
 // Don't block add-ons for e10s
 user_pref("extensions.e10sBlocksEnabling", false);
 
 // Avoid performing Reader Mode intros during tests.
 user_pref("browser.reader.detectedFirstArticle", true);