1470865 - Unmuted tabs are not displayed under the “Hidden Tabs”

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[CosminB]

Attachment: Unmuted.gif

[Affected versions]:
- Firefox 62.0a1 (20180624220331)

[Affected platforms]:
- Win 7 64-bit
- Mac OS X 10.13.3
- Ubuntu 14.04 32-bit

[Steps to reproduce]:
1.Install https://addons.mozilla.org/en-US/firefox/addon/hide-tab/ 
2.Hide some tabs that play audio content.
3.Click on the “v” button.
4.Mute some tabs.
5.Wait until the muted tabs are moved in the “Hidden Tabs” menu.
6.Click on the “Hidden Tabs”.
7.Unmute some tabs.
8.Click on the “<” arrow to go back.

[Expected results]:
- The unmuted tabs are displayed again under the “Hidden Tabs” menu.

[Actual results]:
- The unmuted tabs are not displayed after you navigate back.
- You need to reopen the all tabs menu to see the unmuted tab again.

Please see the attached video .

Comment 1

[Dão Gottwald [:dao]]

Mark, can you please take a look?

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

When a hidden audio tab is muted it isn't pulled into the main all tabs
panel. Unmuting that tab will now add it to the main all tabs panel,
even if the all tabs menu it open.

Review commit: https://reviewboard.mozilla.org/r/256410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256410/

Comment 3

[Mark Striemer [:mstriemer]]

Attachment: add-hidden-audio-on-unmute.mov.gif

This will handle adding rows for tabs that start to match the criteria for a certain section. The likely case would be muting or unmuting hidden tabs, but could also be hiding or unhiding tabs.

This doesn't handle the case where a new tab opens while the menu is still open.

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256410/diff/1-2/

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review266992

::: browser/modules/TabsList.jsm:88
(Diff revision 2)
> +  _addElement(elementOrFragment) {
> +    if (this.insertBefore) {
> +      this.insertBefore.parentNode.insertBefore(elementOrFragment, this.insertBefore);
> +    } else {
> +      this.containerNode.appendChild(elementOrFragment);
> +    }

Can this be simplified to:

this.containerNode.insertBefore(elementOrFragment, this.insertBefore);

?

insertBefore() appends to the end if the second argument is null.

::: browser/modules/TabsList.jsm:118
(Diff revision 2)
>  
>    _tabAttrModified(tab) {
>      let item = this.tabToElement.get(tab);
>      if (item) {
>        if (!this.filterFn(tab)) {
> -        // If the tab is no longer in this set of tabs, hide the item.
> +        // The tab no longer matches our criteria, hide it.

s/hide/remove/

::: browser/modules/TabsList.jsm:130
(Diff revision 2)
> +      this._addTab(tab);
> +    }
> +  }
> +
> +  _addTab(newTab) {
> +    // We want to keep the order of tabToElement, so recreate it.

Why do we want to keep the order? The order doesn't seem relevant anywhere in this file from what I can tell.

Comment 6

[Mark Striemer [:mstriemer]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review266992

> Why do we want to keep the order? The order doesn't seem relevant anywhere in this file from what I can tell.

The order is used to decide where to put the new entry. Maintaining the order will keep the tabs in order, it also seem intuitive for the filtered set of tabs to be in order still.

Comment 7

[Mark Striemer [:mstriemer]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256410/diff/2-3/

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mark Striemer [:mstriemer] from comment #6)
> Comment on attachment 8991508 [details]
> Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu
> 
> https://reviewboard.mozilla.org/r/256410/#review266992
> 
> > Why do we want to keep the order? The order doesn't seem relevant anywhere in this file from what I can tell.
> 
> The order is used to decide where to put the new entry.

This gets the order from gBrowser.tabs, not from tabToElement, as far as I can tell.

> Maintaining the
> order will keep the tabs in order, it also seem intuitive for the filtered
> set of tabs to be in order still.

The primary Map use case is to keep track of relationships. Order is a secondary feature and not generally expected from Maps, and if you really care about this you should probably use a different data structure such as an array, where you can insert items in the middle without rebuilding everything.

Comment 9

[Mark Striemer [:mstriemer]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256410/diff/3-4/

Comment 10

[Mark Striemer [:mstriemer]]

You're right, maintaining the order isn't necessary for the current code. I've removed the rebuilding of the Map to maintain the order.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review268220

::: browser/modules/TabsList.jsm:133
(Diff revision 4)
> +        // If there is a new row, insert it before this one.
> +        let row = this.tabToElement.get(tab);
> +        row.parentNode.insertBefore(newRow, row);
> +        newRow = null;
> +      }
> +    }

This loop looks equivalent to (but slower than) e.g.:

let newRow = this._createRow(newTab);
let nextTab = newTab.nextSibling;
while (nextTab && !this.filterFn(nextTab)) {
  nextTab = nextTab.nextSibling;
}
let nextRow = nextTab && this.tabToElement.get(nextTab);
nextRow.parentNode.insertBefore(newRow, nextRow);

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256410/diff/4-5/

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

https://reviewboard.mozilla.org/r/256410/#review268418

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8880495b4615
Dynamically show hidden audio tabs in all tabs menu r=dao

Comment 15

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8880495b4615

Comment 16

[CosminB]

Attachment: Bug1470865.gif

This issue is verified as fixed on Firefox 63.0a1(20180807220134) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.

Comment 17

[Julien Cristau [:jcristau]]

Is this something we ought to uplift to beta?

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Dão, do you want to request uplift?

Comment 19

[Mark Striemer [:mstriemer]]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Approval Request Comment
[Feature/Bug causing the regression]: bug 1446101
[User impact if declined]: Recently unmuted audio tabs won't re-appear in the all tabs menu when it is open
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Small code change that has been on Nightly for a while
[String changes made/needed]: No

Sorry, I thought I had submitted this. I'm not sure this is necessary for uplift but it shouldn't hurt so could be worth it.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8991508 [details]
Bug 1470865 - Dynamically show hidden audio tabs in all tabs menu

Fix for minor regression, let's uplift for beta 19.

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/37110fa0b0b1

Comment 22

[Cristian Fogel [:cfogel]]

Confirmed issue on 62.0b18 on macOS 10.12 using steps in Comment 0.
Verified fix on macOS 10.12, Ubuntu 18.04LTS, Windows 10x64 and can confirm the issue is no longer reproducible.