724346 - Ctrl-Tab should not cycle through hidden tabs

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[lwz]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

1. Set preference "browser.ctrlTab.previews" to true
2. Create at least two tab groups
3. Switch tab groups (e.g. press Ctrl-`)
4. Hold down Ctrl and press and release the Tab key to open the Ctrl-Tab panel


Actual results:

Panel shows a tab from the other tab group.


Expected results:

Panel should only show tabs from current tab groups.

Comment 1

[lwz]

Attachment: My Fix (in my own coding style)

Filters away hidden tabs from recentlyUsedTabs before adding them to panel.

Comment 2

[lwz]

Attachment: Patched get tabList

It's not exactly a patch in the official format, but similar enough. The main idea is to filter hidden tabs out of recentlyUsedTabs, and to use gBrowser.visibleTabs by modifying the loop. I've tested it out by overriding the getter function from ScratchPad.

Comment 3

[lwz]

Has anyone taken a look at the patch? I just don't want to forget about it or let it be forgotten.

Comment 4

[lwz]

Comment on attachment 624296 [details] [diff] [review]
Patched get tabList

@Tim: Sorry, as Dao wasn't responding, I was hoping you could review this patch instead. Thanks!

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 624296 [details] [diff] [review]
Patched get tabList

>   get tabList () {
>     if (this._tabList)
>       return this._tabList;
> 
>-    // Using gBrowser.tabs instead of gBrowser.visibleTabs, as the latter
>-    // exlcudes closing tabs, breaking the following loop in case the the
>-    // selected tab is closing.
>-    let list = Array.filter(gBrowser.tabs, function (tab) !tab.hidden);
>+    let a, l, list = gBrowser.visibleTabs.slice(0);
> 
>     // Rotate the list until the selected tab is first
>-    while (!list[0].selected)
>-      list.push(list.shift());
>-
>-    list = list.filter(function (tab) !tab.closing);
>+    for (a = 0, l = list.length; a < l; a++) {
>+      if (list[a].selected)
>+        break;
>+      list.push(list[a]);
>+    }
>+    list.splice(0, a);

It's unclear why you're changing this. Seems unrelated to this bug. File a separate bug on it if you think it's a useful change.

Making your patch easy to understand is the best way to bring forward quick reviews ;)

Comment 6

[lwz]

Attachment: Patched get tabList v2

Ok. I have changed only the relevant part now.

The earlier patch changed the original list of tabs to be based on visibleTabs back again but without the infinite loop problem, and didn't use shift (which I think might be inefficient). Also, I thought that splice and unshift might be inefficient, so in that patch I rewrote the last part also. But I suppose those are irrelevant now. Perhaps you could enlighten me about how the JavaScript engine does shift, unshift and splice for arrays... :)

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 644636 [details] [diff] [review]
Patched get tabList v2

>diff --git a/browser/base/content/browser-tabPreviews.js b/browser/base/content/browser-tabPreviews.js
>--- a/browser/base/content/browser-tabPreviews.js
>+++ b/browser/base/content/browser-tabPreviews.js
>@@ -185,17 +185,28 @@ var ctrlTab = {
>     // Rotate the list until the selected tab is first
>     while (!list[0].selected)
>       list.push(list.shift());
> 
>     list = list.filter(function (tab) !tab.closing);
> 
>     if (this.recentlyUsedLimit != 0) {
>-      let recentlyUsedTabs = this._recentlyUsedTabs;
>-      if (this.recentlyUsedLimit > 0)
>-        recentlyUsedTabs = this._recentlyUsedTabs.slice(0, this.recentlyUsedLimit);
>+      let rlist = this._recentlyUsedTabs, r = this.recentlyUsedLimit, recentlyUsedTabs;

nit: one variable declaration per line, and please name them more clearly

>+      // Filter rlist for relevant tabs and puts them into recentlyUsedTabs
>+      if (r > 0) {
>+        // Limit the number of recently used tabs
>+        let i = 0;
>+        recentlyUsedTabs = [];
>+        while (i < rlist.length && recentlyUsedTabs.length < r) {
>+          if (!rlist[i].hidden && !rlist[i].closing)
>+            recentlyUsedTabs.push(rlist[i]);
>+          i++;
>+        }
>+      } else {
>+        recentlyUsedTabs = rlist.filter(function (tab) !tab.hidden && !tab.closing);
>+      }
>       for (let i = recentlyUsedTabs.length - 1; i >= 0; i--) {
>         list.splice(list.indexOf(recentlyUsedTabs[i]), 1);
>         list.unshift(recentlyUsedTabs[i]);
>       }
>     }
> 
>     return this._tabList = list;

Would it be simpler if we put "...filter(function (tab) !tab.hidden && !tab.closing)" at the bottom, right before the return?

Comment 8

[lwz]

(In reply to Dão Gottwald [:dao] from comment #7)
> nit: one variable declaration per line, and please name them more clearly

IMO it's clear enough, but I will change it.

> Would it be simpler if we put "...filter(function (tab) !tab.hidden &&
> !tab.closing)" at the bottom, right before the return?

Because then the recentlyUsedLimit will not be accurate anymore.

Comment 9

[lwz]

What variable names would you like? allRecentlyUsedTabs and recentlyUsedLimit? I've always been in favour of short variable names especially when they are temporary.

Comment 10

[lwz]

Attachment: Patched get tabList v3

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 644718 [details] [diff] [review]
Patched get tabList v3

>+      // Filter rlist for relevant tabs and puts them into recentlyUsedTabs

rlist -> allRecentlyUsedTabs
puts -> put

>+      if (recentlyUsedLimit > 0) {
>+        // Limit the number of recently used tabs
>+        let i = 0;
>+        recentlyUsedTabs = [];
>+        while (i < allRecentlyUsedTabs.length && recentlyUsedTabs.length < recentlyUsedLimit) {
>+          if (!allRecentlyUsedTabs[i].hidden && !allRecentlyUsedTabs[i].closing)
>+            recentlyUsedTabs.push(allRecentlyUsedTabs[i]);
>+          i++;
>+        }
>+      } else {
>+        recentlyUsedTabs = allRecentlyUsedTabs.filter(function (tab) !tab.hidden && !tab.closing);
>+      }

Looks like this can be simplified. How about:

>      let recentlyUsedTabs = [];
>      for (let tab of allRecentlyUsedTabs) {
>        if (!tab.hidden && !tab.closing) {
>          recentlyUsedTabs.push(tab);
>          if (recentlyUsedLimit > 0 && recentlyUsedTabs.length >= recentlyUsedLimit)
>            break;
>        }
>      }

Comment 12

[lwz]

Attachment: Patched get tabList v4

OK. As usual, I thought it would be more efficient doing it in the original way.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 652795 [details] [diff] [review]
Patched get tabList v4

>     if (this.recentlyUsedLimit != 0) {
>-      let recentlyUsedTabs = this._recentlyUsedTabs;
>-      if (this.recentlyUsedLimit > 0)
>-        recentlyUsedTabs = this._recentlyUsedTabs.slice(0, this.recentlyUsedLimit);
>+      let allRecentlyUsedTabs = this._recentlyUsedTabs;
>+      let recentlyUsedLimit = this.recentlyUsedLimit
>+      let recentlyUsedTabs = [];
>+      // Filter allRecentlyUsedTabs for relevant tabs and put them into recentlyUsedTabs
>+      for (let tab of allRecentlyUsedTabs) {
>+        if (!tab.hidden && !tab.closing) {
>+          recentlyUsedTabs.push(tab);
>+          if (recentlyUsedLimit > 0 && recentlyUsedTabs.length >= recentlyUsedLimit)
>+            break;
>+        }
>+      }

Actually, since this code only runs if this.recentlyUsedLimit != 0, you can remove the recentlyUsedLimit > 0 check.
Secondly, you can use this._recentlyUsedTabs and this.recentlyUsedLimit directly at this point without declaring the allRecentlyUsedTabs and recentlyUsedLimit variables. (You'll also need to replace "allRecentlyUsedTabs" in the comment, or just remove the comment.)

r=me with these changes. Thanks!

Comment 14

[lwz]

Actually I thought that if recentlyUsedLimit is negative, it is supposed to add all the tabs (the original worked that way)? I suppose we could change that behavior. Also, I thought looking up a local variable in the immediate scope is more efficient than accessing it from an object?

Comment 15

[Dão Gottwald [:dao]]

(In reply to lwz from comment #14)
> Actually I thought that if recentlyUsedLimit is negative, it is supposed to
> add all the tabs (the original worked that way)?

You're right, I didn't consider this case.

> Also, I thought looking up a local variable in the immediate
> scope is more efficient than accessing it from an object?

The difference, if there's one at all, is going to be negligible.

Comment 16

[lwz]

Attachment: Patched get tabList v5

I've done some testing, and accessing a member of an object with a few members the performance is ~50% while with a large number of members, the performance can go as low as 10%. However, I have done as you wish.

I don't know how to complete this patching process, and I think I'm not able to. My patch is made with WinMerge with 7 lines of context (I can't change that).

Comment 17

[Dão Gottwald [:dao]]

I'm not sure what your 50% and 10% figures refer to.

Comment 18

[lwz]

It's the speed of accessing the variable from an object compared to accessing the value from a variable in the current scope.

Comment 19

[Dão Gottwald [:dao]]

What absolute numbers are we talking about and how do they relate to the total time the loop takes?

Comment 20

[lwz]

I'm making a general statement based on my experiments with JSPerf. I am not making any specific claim with this particular implementation. The point is that, at least in relative performance, it is just not negligible. If you have say a thousand tabs across all tab groups, there is going to be a slight performance difference.

Whatever the case, I think I have already done what I can with the patching. I don't think I want to debate about this anymore.

Comment 21

[Dão Gottwald [:dao]]

The relative difference between this.recentlyUsedLimit and recentlyUsedLimit is only interesting when putting it in perspective, e.g. by answering the two questions in comment 19. Not every micro-optimization is worth it.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Green on Try (the m-oth orange was due to another patch in the push).
https://tbpl.mozilla.org/?tree=Try&rev=4c6632e2334a

https://hg.mozilla.org/integration/mozilla-inbound/rev/1db0e95acab8

Should this have a test?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1db0e95acab8

Comment 24

[M8R-20dnio]

I can't pass up this opportunity to complain. This "fix" drove me nuts on upgrading to 17.0.1. It breaks one of the most useful and powerful UI features - to me, anyway. Am I a minority of one? What criteria determined the (apparently consensus) view that "ctrl-tab should not cycle through hidden tabs"? Is it by decree that no information should span tab groups, or did introducing tab groups uncover too many lower-level bugs to fix?

Comment 25

[Darren Kalck [:D-Kalck]]

I totally agree with the comment above. Why there's no hidden pref to revert to the previous behavior ?
Could somebody post a link to the discussion that drove to this change ? Don't tell me that there was no discussion about this !

Comment 26

[lwz]

Ok. I don't disagree with you both, but the reason for doing so was to make it consistent with the behavior without Ctrl-Tab turned on. I'm completely fine if the patch was reverted based on your reasons. However I don't have the authority to do that.
@Dao/Ryan: Are any of you willing to undo the patch?

Comment 27

[Darren Kalck [:D-Kalck]]

I suggest adding a preference browser.ctrlTab.showHiddenTab or something, that should be simple.

Comment 28

[lwz]

Attachment: Reverts the behavior by default

With this patch, by default Ctrl-Tab will cycle through hidden tabs. Only if a preference "browser.ctrlTab.showHiddenTabs=false" is added, then Ctrl-Tab will not cycle through hidden tabs.

Comment 29

[lwz]

Attachment: Reverts the behavior by default

With this patch, by default Ctrl-Tab will cycle through hidden tabs. Only if a preference "browser.ctrlTab.showHiddenTabs=false" is added, then Ctrl-Tab will not cycle through hidden tabs.

Edit: Now it would not throw an error if you created the preference with a different type.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 693368 [details] [diff] [review]
Reverts the behavior by default

I'm not convinced there's enough demand for a hidden pref here. I'd rather just completely revert the change.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Let's not reopen the bug unless we actually do decide to revert the change (in which case we can just move it directly to WONTFIX). Seems like we should have that discussion in bug 808540.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Changing to WONTFIX as bug 808540 is on its way to being fixed.