Closed
Bug 1387976
Opened 7 years ago
Closed 6 years ago
Normal tabs are not remembering their previously muted state and outputs sound after restart
Categories
(Firefox :: Session Restore, defect, P2)
Firefox
Session Restore
Tracking
()
VERIFIED
FIXED
Firefox 61
People
(Reporter: emilghitta, Assigned: ablayelyfondou, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 3 obsolete files)
3.11 MB,
image/gif
|
Details | |
4.95 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
4.94 KB,
patch
|
Details | Diff | Splinter Review |
[Affected versions] Firefox 57.0a1 (Build Id:20170806100257) Firefox 55.0 (Build Id:20170803103124) [Affected platforms]: Windows 10 64bit. macOS 10.11.6. Ubuntu 14.04 64 bit. [Steps to reproduce]: 1.Launch Firefox with a clean profile. 2.Access the https://www.youtube.com/ webpage. 3.Open a video in a new tab. 4.Mute the opened tab. 5.Restart Firefox. 6.Reload all tabs. [Expected result]: The previously opened tab remembers it's previous state and is muted. [Actual result]: The previously opened tab outputs sound. [Regression range]: I don't think that this is a regression since it is reproducible with Firefox 48.0a1 also. [Additional notes] Please observe the attached screencast for further information. This issue is also reproducible after restoring the previous session after a crash. This issue is not reproducible with pinned tabs.
Updated•7 years ago
|
Blocks: ss-feature
Priority: -- → P2
Comment 1•7 years ago
|
||
So I'm thinking that someone who worked on the mute tab indicators before would be the ideal candidate to fix this most efficiently. Dão, since Ehsan is away for a while, I can't really ask him... do you know who may be able to triage this tab mute issue?
Mentor: mdeboer
Flags: needinfo?(dao+bmo)
Comment 2•7 years ago
|
||
:alwu worked somewhat recently on bug 1347791 which seems related.
Flags: needinfo?(dao+bmo)
Comment 3•7 years ago
|
||
Indeed, thanks! Alastor, do you or someone on your team have time to work on this bug? I'd be more than happy to mentor the Sessionstore component related bits!
Flags: needinfo?(alwu)
Comment 4•7 years ago
|
||
Sure, I can take this bug, but I have other works need to be done first. I'll go back on it later.
Assignee: nobody → alwu
Updated•6 years ago
|
Assignee: alastor0325 → nobody
Flags: needinfo?(alastor0325)
Updated•6 years ago
|
Assignee: nobody → ablayelyfondou
Mentor: jaws
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
FYI: There is no test case for this patch, thanks on giving hints in case we need one.
Flags: needinfo?(jaws)
Comment 7•6 years ago
|
||
Looks good! I tested out your patch on my machine and it fixes the bug for me :) browser/base/content/test/general/browser_audioTabIcon.js has some tests for this feature. I ran the test locally and it continues to pass, which is good news. To run the test, you would run the following command: > `./mach test browser/base/content/test/general/browser_audioTabIcon.js` We should add some new tests for the work that you are doing, but not in that file since that test takes far too long to run already (over 30 seconds). We can't add any new tests to /browser/base/content/test/general since that directory is too large. It also helps to put the tests in a specific directory so it is easy to run a small subset of tests to check that a minor change hasn't broken a feature. You can add the new test in `/browser/base/content/test/tabs`. Feel free to follow the style of browser_audioTabIcon.js, specifically it looks like get_tab_state() would be useful. It looks like you may be able to simulate a restart of a specific tab by doing this: https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/base/content/test/general/browser_restore_isAppTab.js#35-50
Flags: needinfo?(jaws)
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7) > It looks like you may be able to simulate a restart of a specific tab by > doing this: > https://searchfox.org/mozilla-central/rev/ > d03ad8843e3bf2e856126bc53b0475c595e5183b/browser/base/content/test/general/ > browser_restore_isAppTab.js#35-50 This does not work for this specific situation, because the tab isn't actually removed and keeps its state so. The only thing that I feel is really restarted here is the tab browser. So I needed to simulate the restart of the tab by "replacing" it with a new lazy tab restored with the removed tab data of course.
Flags: needinfo?(jaws)
Comment 10•6 years ago
|
||
Comment on attachment 8952577 [details] [diff] [review] Adding test for the bug patch Review of attachment 8952577 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Can you combine your patches and reupload them with the following commit message: `Bug 1387976 - Persist tab-muted state across restarts of the browser. r?jaws` Then we can get this landed. Nice job! ::: browser/base/content/test/tabs/browser.ini @@ +34,4 @@ > [browser_visibleTabs_bookmarkAllTabs.js] > [browser_visibleTabs_contextMenu.js] > [browser_open_newtab_start_observer_notification.js] > +[browser_bug_1387976_restore_lazy_tab_browser_muted_state.js] Can you add a blank line at the end of the file like it was before?
Attachment #8952577 -
Flags: review+
Updated•6 years ago
|
Attachment #8950833 -
Flags: review+
Updated•6 years ago
|
Flags: needinfo?(jaws)
Assignee | ||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
final upload: combined patches renamed commit message Added a blank line at the end of browser/base/content/test/tabs/browser.ini
Comment 13•6 years ago
|
||
Bug 1392352 moved this code out of tabbrowser.xml and in to browser/base/content/tabbrowser.js. Can you pull the latest code from mozilla-central and update your patch?
Flags: needinfo?(ablayelyfondou)
Comment 14•6 years ago
|
||
Comment on attachment 8954625 [details] [diff] [review] Persist tab-muted state across restarts of the browser Review of attachment 8954625 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +2368,4 @@ > let setter; > switch (name) { > case "audioMuted": > + getter = () => aTab.hasAttribute("muted"); Should this be using `SessionStore.getLazyTabValue(aTab, "muted");` ?
Assignee | ||
Comment 15•6 years ago
|
||
> ::: browser/base/content/tabbrowser.xml > @@ +2368,4 @@ > let setter; > switch (name) { > case "audioMuted": > + getter = () => aTab.hasAttribute("muted"); > Should this be using `SessionStore.getLazyTabValue(aTab, "muted");` ? This can actually work but requires several changes such as: 1. Adding a "muted" attribute for "tab.__SS_lazyData" object in SessionStore.jsm 2. defining the method "setLazyTabValue" because the "muted" attribute may change for a lazy tab browser. 3. call "setLazyTabValue" method when the muted icon is toggled (toggleMuteAudio in tabbrowser.xml) Personally I find this approach less clean & more complicated. So what do you think ?
Flags: needinfo?(ablayelyfondou)
Assignee | ||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
If I understand https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/base/content/tabbrowser.js#98-106 correctly, changing the muted state while the browser is lazy will cause the browser to wake up, so I think we need to do what you've described in comment #15.
Assignee | ||
Comment 18•6 years ago
|
||
> changing the muted state while the browser is lazy will cause the browser to wake up
No, it won't. The browser will still remain lazy. Actually, only the invocation of methods "browser.mute()" or "browser.unmute()" in toggleMuteAudio methods wake up the browser and fortunately we can avoid this.
Briefly, in both approaches we will have the browser remaining lazy when its muted state changes.
Comment 19•6 years ago
|
||
Comment on attachment 8955961 [details] [diff] [review] Persist tab-muted state across restarts of the browser Review of attachment 8955961 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, I tested this out and it works great. I'll get this landed now :)
Attachment #8955961 -
Flags: review+
Updated•6 years ago
|
Attachment #8950833 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8952577 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8954625 -
Attachment is obsolete: true
Updated•6 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
Mentor: mdeboer
Comment 20•6 years ago
|
||
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7958b0ccbe0b Persist tab-muted state across restarts of the browser. r=jaws
Keywords: checkin-needed
Updated•6 years ago
|
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
Comment 21•6 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b708ab55bc72 Fix merge and eslint issues with the landing of the patch on a CLOSED TREE. r=me
Comment 22•6 years ago
|
||
Hey Abdoulaye, the patch that you had attached to the bug had some + characters at the beginning of two lines in tabbrowser.xml. I landed a follow-up patch to remove those (and also fix a spacing issue in the comments). Did you hand-edit the patch file or have some issues with merging/rebasing while you were working on it? There are no problems, I just want to help you so in the future we won't run in to this again :)
Flags: needinfo?(ablayelyfondou)
Comment 23•6 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d91b6a40b32a Fix a copy/paste error in a comment (should have been mute, not unmute). r=me
Assignee | ||
Comment 24•6 years ago
|
||
Oh sorry!!! > Did you hand-edit the patch file No, I did not. I always use hg import. > or have some issues with merging/rebasing while you were working on it? No, but what I actually did was to add the changes into tabbrowser.js by copying code from tortoise-hg (which is why we have these + caracteres). I will be more careful next time. Can I reupload the patch here. Thank you for your help.
Flags: needinfo?(ablayelyfondou) → needinfo?(jaws)
Comment 25•6 years ago
|
||
Backout by csabou@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/7c01c3875cae Backed out 3 changesets because of merge conflicts on AsyncTabSwitcher.jsm.
Comment 26•6 years ago
|
||
Sorry, had to back this out because of merge conflicts: https://hg.mozilla.org/mozilla-central/rev/7c01c3875cae3d9b8a3fab22c4b07efe20f6d48f merging browser/base/content/tabbrowser.js merging browser/base/content/tabbrowser.xml merging browser/modules/AsyncTabSwitcher.jsm and browser/base/content/tabbrowser.js to browser/modules/AsyncTabSwitcher.jsm warning: conflicts while merging browser/modules/AsyncTabSwitcher.jsm! (edit, then use 'hg resolve --mark') 170 files updated, 2 files merged, 0 files removed, 1 files unresolved
Flags: needinfo?(ablayelyfondou)
Comment 27•6 years ago
|
||
Yes, you can reupload the patch. You will need to rebase the patch too.
Flags: needinfo?(jaws)
Assignee | ||
Comment 28•6 years ago
|
||
Assignee | ||
Comment 29•6 years ago
|
||
Patch reuploaded: - Conflicts resolved - removed "+" caracteres - fixed spacing issue on comments
Flags: needinfo?(ablayelyfondou)
Updated•6 years ago
|
Flags: needinfo?(jaws)
Assignee | ||
Comment 30•6 years ago
|
||
Hey Jared, have you seen the last upload ? FYI: It's still rebasing without conflict.
Comment 31•6 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/de798abb5ac7 Persist tab-muted state across restarts of the browser. r=jaws
Updated•6 years ago
|
Flags: needinfo?(jaws)
Comment 32•6 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a049e86626d4 Add missing test file for persisting tab-muted state across restarts of the browser. r=jaws
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de798abb5ac7 https://hg.mozilla.org/mozilla-central/rev/a049e86626d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 34•6 years ago
|
||
Thanks for fixing this bug Abdoulaye! I will try to find you another bug you can work on.
Assignee | ||
Comment 35•6 years ago
|
||
The pleasure is mine and your help was great! I am looking foward to get a new bug to work on.
Flags: needinfo?(jaws)
Updated•6 years ago
|
Flags: needinfo?(jaws)
Updated•6 years ago
|
Comment 36•6 years ago
|
||
I have reproduced this bug with Nightly 57.0a1 (2017-08-07) on Windows 10 , 64 Bit ! This bug's fix is Verified with latest Nightly ! Build ID 20180327105613 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 [bugday-20180321]
Comment 37•6 years ago
|
||
I successfully reproduced the bug on Firefox Nightly 57.0a1 (2017-08-06) under Windows 10 (X64). The bug is no longer reproducible on latest Nightly 61.0a1 (2018-03-27) under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.13. Thank you Sajedul Islam for helping out with testing.
Status: RESOLVED → VERIFIED
Reporter | ||
Updated•6 years ago
|
Flags: in-qa-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•