Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r=gijs

MozReview-Commit-ID: BNaVgrX6jsv

--HG--
extra : rebase_source : 54bc9b692838dc801cc274cea19107974dcd595a

browser/app/profile/firefox.js

@@ -436,6 +436,8 @@ pref("browser.tabs.drawInTitlebar", true);
 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);
 

browser/base/content/tabbrowser.xml

@@ -765,6 +765,8 @@
 
                 // 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"]);
                 }
@@ -2720,6 +2722,14 @@
             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.
@@ -4872,6 +4882,7 @@
         ]]>
         </getter>
       </property>
+      <field name="_soundPlayingAttrRemovalTimer">0</field>
     </implementation>
 
     <handlers>
@@ -5002,6 +5013,9 @@
           if (!tab)
             return;
 
+          clearTimeout(tab._soundPlayingAttrRemovalTimer);
+          tab._soundPlayingAttrRemovalTimer = 0;
+
           if (!tab.hasAttribute("soundplaying")) {
             tab.setAttribute("soundplaying", true);
             this._tabAttrModified(tab, ["soundplaying"]);
@@ -5020,8 +5034,11 @@
             return;
 
           if (tab.hasAttribute("soundplaying")) {
-            tab.removeAttribute("soundplaying");
+            let removalDelay = Services.prefs.getIntPref("browser.tabs.delayHidingAudioPlayingIconMS");
-            this._tabAttrModified(tab, ["soundplaying"]);
+            tab._soundPlayingAttrRemovalTimer = setTimeout(() => {
+              tab.removeAttribute("soundplaying");
+              this._tabAttrModified(tab, ["soundplaying"]);
+            }, removalDelay);
           }
         ]]>
       </handler>

browser/base/content/test/general/browser_audioTabIcon.js

@@ -1,4 +1,6 @@
 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) {
@@ -6,7 +8,7 @@ function* wait_for_tab_playing_event(tab, expectPlaying) {
     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;
@@ -25,14 +27,41 @@ function* play(tab) {
   yield wait_for_tab_playing_event(tab, true);
 }
 
-function* pause(tab) {
+function* pause(tab, options) {
-  let browser = tab.linkedBrowser;
+  ok(tab.hasAttribute("soundplaying"), "The tab should have the soundplaying attribute when pause() is called");
-  yield ContentTask.spawn(browser, {}, function* () {
-    let audio = content.document.querySelector("audio");
-    audio.pause();
-  });
 
-  yield wait_for_tab_playing_event(tab, false);
+  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);
+  }
+
+  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) {
@@ -83,7 +112,7 @@ 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");
 
@@ -210,8 +239,8 @@ function* test_swapped_browser_while_playing(oldTab, newBrowser) {
 
   let newTab = gBrowser.getTabForBrowser(newBrowser);
   let AttrChangePromise = BrowserTestUtils.waitForEvent(newTab, "TabAttrModified", false, event => {
-    return event.detail.changed.indexOf("soundplaying") >= 0 &&
+    return event.detail.changed.includes("soundplaying") &&
-           event.detail.changed.indexOf("muted") >= 0;
+           event.detail.changed.includes("muted");
   });
 
   gBrowser.swapBrowsersAndCloseOther(newTab, oldTab);
@@ -221,21 +250,6 @@ function* test_swapped_browser_while_playing(oldTab, newBrowser) {
   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);
@@ -248,7 +262,7 @@ function* test_swapped_browser_while_not_playing(oldTab, newBrowser) {
 
   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 => {
@@ -359,7 +373,7 @@ function* test_cross_process_load() {
     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.
@@ -447,6 +461,24 @@ function* test_on_browser(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": [
@@ -468,3 +500,5 @@ 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);

testing/profiles/prefs_general.js

@@ -318,6 +318,8 @@ user_pref("media.decoder.heuristic.dormant.timeout", 0);
 // 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);