From 44261504887e6a57870f7b6374565c822a3cfa5a Mon Sep 17 00:00:00 2001 From: Jared Wein Date: Thu, 13 Oct 2016 16:12:50 -0400 Subject: [PATCH] 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 | 2 + browser/base/content/tabbrowser.xml | 21 ++++- .../test/general/browser_audioTabIcon.js | 90 +++++++++++++------ testing/profiles/prefs_general.js | 2 + 4 files changed, 85 insertions(+), 30 deletions(-) diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index 596c70d95238..a212826c587d 100644 --- a/browser/app/profile/firefox.js +++ b/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); diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml index caf6d8d3ede2..57fc0460b9bc 100644 --- a/browser/base/content/tabbrowser.xml +++ b/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 @@ ]]> + 0 @@ -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"); - this._tabAttrModified(tab, ["soundplaying"]); + let removalDelay = Services.prefs.getIntPref("browser.tabs.delayHidingAudioPlayingIconMS"); + tab._soundPlayingAttrRemovalTimer = setTimeout(() => { + tab.removeAttribute("soundplaying"); + this._tabAttrModified(tab, ["soundplaying"]); + }, removalDelay); } ]]> diff --git a/browser/base/content/test/general/browser_audioTabIcon.js b/browser/base/content/test/general/browser_audioTabIcon.js index 902e1fe345a0..8f066ed1b145 100644 --- a/browser/base/content/test/general/browser_audioTabIcon.js +++ b/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) { - 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"); - 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 && - event.detail.changed.indexOf("muted") >= 0; + return event.detail.changed.includes("soundplaying") && + 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); diff --git a/testing/profiles/prefs_general.js b/testing/profiles/prefs_general.js index 0953cdf583fc..7cafbac27fa2 100644 --- a/testing/profiles/prefs_general.js +++ b/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);