Last Comment Bug 681005 - Restore pinned tabs before normal tabs
: Restore pinned tabs before normal tabs
Status: RESOLVED FIXED
[good first bug][mentor=zpao]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Andres Hernandez [:andreshm]
:
:
Mentors:
Depends on: 771504
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-22 11:58 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-09-03 12:41 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled


Attachments
Moves the pinned tab to the beggining of the tabs arrays (879 bytes, patch)
2011-08-31 16:37 PDT, Pablo Terradillos
bugspam.Callek: feedback-
Details | Diff | Splinter Review
Same diff as before but with more context (2.04 KB, patch)
2011-09-01 01:44 PDT, Pablo Terradillos
no flags Details | Diff | Splinter Review
Fixed identation (2.07 KB, patch)
2011-09-01 01:52 PDT, Pablo Terradillos
no flags Details | Diff | Splinter Review
The reorder for pinned tabs was merged with the reorder for unhidden tabs (2.04 KB, patch)
2012-02-05 20:07 PST, Pablo Terradillos
no flags Details | Diff | Splinter Review
Patch v2 (6.12 KB, patch)
2012-04-30 17:51 PDT, Andres Hernandez [:andreshm]
paul: feedback+
Details | Diff | Splinter Review
Patch v3 (11.13 KB, patch)
2012-06-13 15:19 PDT, Andres Hernandez [:andreshm]
paul: review-
paul: feedback+
Details | Diff | Splinter Review
patch v4 (14.84 KB, patch)
2012-06-18 15:21 PDT, Andres Hernandez [:andreshm]
paul: review+
Details | Diff | Splinter Review
Backout patch (14.74 KB, patch)
2012-09-01 07:14 PDT, :Ehsan Akhgari
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-22 11:58:28 PDT
Right now we reorder the array of tabs so that we restore tabs that are visible on the tabstrip before other tabs, with hidden tabs last. App tabs are just restored at some point (depends on how many tabs) even though they are visible.

We should push them up, so I'm thinking we restore in this order:
selected tab, app tabs, other visible tabs, non-visible tabs, hidden tabs.

This will help with bug 674452 (even after prioritizing app tabs there, they'll still be restored after other tabs)
Comment 1 Siddhartha Dugar [:sdrocking] 2011-08-23 11:52:01 PDT
Mark this as blocking bug 674452
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-23 13:40:19 PDT
(In reply to sdrocking from comment #1)
> Mark this as blocking bug 674452

I thought about that but it's not really necessary. That can land without this. It would help a little bit, but the end result here is mostly that we'll have a few normal tabs restored while the "priority" bucket is empty, but once we hit the apptabs, they'll end up in the priority bucket and will be restored before any more normal tabs. The likely case is that 3 tabs will be restored before apptabs which is definitely not the end of the world.
Comment 3 Pablo Terradillos 2011-08-30 17:06:00 PDT
I'm working on this bug. I've just got some help of :zpao. Think I'll have a diff soon.
Comment 4 Pablo Terradillos 2011-08-31 16:37:57 PDT
Created attachment 557365 [details] [diff] [review]
Moves the pinned tab to the beggining of the tabs arrays

We get the pinned tabs at beginning of the tabs' array. After that, the selectedTab is putted at the very beginning so we get the desire flow.
The way of reordering is based on the one done for the hidden tabs.
Comment 5 Justin Wood (:Callek) 2011-08-31 22:43:15 PDT
Comment on attachment 557365 [details] [diff] [review]
Moves the pinned tab to the beggining of the tabs arrays

I'm not a reviewer, but lets start with the simple.

There was many tabs brought into this here, when in fact it should just have been spaces.

I would also normally recommend at least 4, more often 8 lines of context, as well as using -p with the diff for when you are submitting patches, I am unsure what paul would like here though. (but usually more context can't hurt too much.)

See-Also: https://developer.mozilla.org/en/Creating_a_patch
Comment 6 Pablo Terradillos 2011-09-01 01:44:54 PDT
Created attachment 557443 [details] [diff] [review]
Same diff as before but with more context

I've just added more lines to the diff.
Comment 7 Pablo Terradillos 2011-09-01 01:52:45 PDT
Created attachment 557444 [details] [diff] [review]
Fixed identation

Used spaces instead of tabs. I've used the same indentation level that the rest of the file. Seems that my vim configuration was messing with spaces.
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-09-07 15:23:11 PDT
Comment on attachment 557444 [details] [diff] [review]
Fixed identation

I talked with Pablo on IRC about adding a test and doing this slightly differently. He said he would attach a new patch when that was ready, so I'm clearing review from the existing patch.
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 20:43:08 PST
Pablo, are you going to have the time to follow up and add some tests?
Comment 10 Pablo Terradillos 2011-11-20 15:18:22 PST
Yes Paul, sorry. I'll have it by the end or this week (I had some issues with my pc)
Comment 11 Pablo Terradillos 2011-11-20 15:18:46 PST
Yes Paul, sorry. I'll have it by the end or this week (I had some issues with my pc)
Comment 12 Jason Yeo[:jyeo] 2012-02-04 12:09:52 PST
hi, it seems that pablo is not responding. I would like to complete his work.

So for this bug what's left are the testcases right? Where should the testcases go to?
Comment 13 Josh Matthews [:jdm] 2012-02-04 12:40:25 PST
Paul may correct me on this, but I expect a new test file in browser/components/sessionstore/test would be desired.
Comment 14 Jason Yeo[:jyeo] 2012-02-04 12:55:51 PST
(In reply to Josh Matthews [:jdm] from comment #13)
> Paul may correct me on this, but I expect a new test file in
> browser/components/sessionstore/test would be desired.

which test framework should i use? I have created xpcshell testcases for thunderbird but have not used mozmill or anything else before.
Comment 15 Josh Matthews [:jdm] 2012-02-04 13:13:29 PST
The sessionstore tests require browser-chrome tests: https://developer.mozilla.org/en/Browser_chrome_tests . See the other tests in that directory for examples of how they work.
Comment 16 Pablo Terradillos 2012-02-05 20:07:54 PST
Created attachment 594621 [details] [diff] [review]
The reorder for pinned tabs was merged with the reorder for unhidden tabs

I've attached a diff with the same modifications but merging the loop for looking the pinned tabs with the one for unhidden tabs.

I've been messing around with the tests but couldn't finish. Probably you can give me a hand with this.
Comment 17 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-06 12:54:47 PST
(In reply to Josh Matthews [:jdm] from comment #13)
> Paul may correct me on this, but I expect a new test file in
> browser/components/sessionstore/test would be desired.

Last time we changed the order, we just modified https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_480148.js, so let's just do that again.
Comment 18 Andres Hernandez [:andreshm] 2012-04-30 17:51:30 PDT
Created attachment 619785 [details] [diff] [review]
Patch v2

When debugging the previous patch, there is a case when the pinned tabs are not restored in the desired order. Since are moved back in the process that optimize the visible tabs, that depends in the window size. 
So, I moved the pinned tabs reordering after the visible tabs process is done and before the selected tab is moved to the first position.
This way, we can ensure the restore order: selected tab, app tabs, other visible tabs, non-visible tabs, hidden tabs. 
The test is updated to add cases, however, I would like to receive feedback first before adding the test cases.
Comment 19 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-17 13:10:06 PDT
Comment on attachment 619785 [details] [diff] [review]
Patch v2

Review of attachment 619785 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review until there are more tests to make sure it's working but f+ overall.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2964,5 @@
>        return;
>      }
>  
>      let unhiddenTabs = aTabData.filter(function (aData) !aData.hidden).length;
> +    let pinnedTabs = aTabData.filter(function (aData) aData.pinned).length;

Nit: move this down to where it's actually being used.

@@ +3005,5 @@
> +    if (pinnedTabs && aTabs.length > 1) {
> +      for (let t = 0; t < aTabs.length; t++) {
> +        if (aTabData[t].pinned) {
> +          aTabs.unshift(aTabs.splice(t, 1)[0]);
> +          aTabData.unshift(aTabData.splice(t, 1)[0]);

I think this is going to reverse the order of the pinned tabs (going in order and unshifting onto the front as you go...). We can make sure with tests though!

::: browser/components/sessionstore/test/browser_480148.js
@@ +128,1 @@
>      }

You mentioned adding more test cases, which is good. This new case only hits the pinned tab being selected and we should get the other cases.
Comment 20 Andres Hernandez [:andreshm] 2012-06-13 15:19:02 PDT
Created attachment 632909 [details] [diff] [review]
Patch v3

Improved the previous patch, in order to not alter the pinned tabs order. Also, updated tests to compare the restoring tab index with the tabs index (oranges with oranges) and not the position index of the tab in tabbar.
Comment 21 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-06-16 11:43:51 PDT
Comment on attachment 632909 [details] [diff] [review]
Patch v3

Review of attachment 632909 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but can you pull out the reordering out to a new function. restoreHistoryPrecursor is getting ugly and hard to follow.

f+, r- for the smaller bits. Just a quick review to follow up.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2877,5 @@
>  
> +    // Store the pinned tabs and update the selected tab before ordering
> +    let pinnedTabs = aTabData.filter(function (aData) aData.pinned).length;
> +    let pinnedTabsArray = new Array();
> +    let pinnedTabsDataArray = new Array();

Please just use Array literals, not new Array() - `a = []`

@@ +2895,5 @@
> +          } else if (pinnedSelectedTab) 
> +            ++pinnedSelectedTab;
> +        }
> +      }
> +    }

This seems overly complicated but it's working. Can you add a comment above the block explaining how this reordering is working.

@@ +2938,5 @@
> +      for (let t = pinnedTabsArray.length - 1; t >= 0; t--) {
> +        aTabs.unshift(pinnedTabsArray.splice(t, 1)[0]);
> +        aTabData.unshift(pinnedTabsDataArray.splice(t, 1)[0]);
> +        if (pinnedSelectedTab) {
> +          aSelectTab = pinnedSelectedTab;

We shouldn't need to do this each time through the loop. Just do it once outside the loop.

::: browser/components/sessionstore/test/browser_480148.js
@@ +139,5 @@
> +      selectedTab: 2,
> +      shownTabs: 6,
> +      hiddenTabs: [],
> +      pinnedTabs: [0,1,2],
> +      order: [2, 0, 1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]

Can you add a test case where a tab is both pinned and hidden (pinned should override)
Comment 22 Andres Hernandez [:andreshm] 2012-06-18 15:21:42 PDT
Created attachment 634204 [details] [diff] [review]
patch v4

Applied suggestions.
Comment 23 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-06-21 19:05:51 PDT
Comment on attachment 634204 [details] [diff] [review]
patch v4

Review of attachment 634204 [details] [diff] [review]:
-----------------------------------------------------------------

If tests are passing, then feel free to land this. Thanks Andres!
Comment 24 Tim Taubert [:ttaubert] 2012-07-03 02:17:49 PDT
Andres, did you have a chance to push it to try, yet?
Comment 25 Tim Taubert [:ttaubert] 2012-07-04 08:46:29 PDT
https://hg.mozilla.org/integration/fx-team/rev/43c35b832235
Comment 26 Tim Taubert [:ttaubert] 2012-07-04 14:29:43 PDT
https://hg.mozilla.org/mozilla-central/rev/43c35b832235
Comment 27 Peter Henkel [:Terepin] 2012-07-06 05:55:22 PDT
This patch may be causing and issue with app tabs. At least for me:
I have 4 app tabs in following order: Google+, Gmail, Facebook and Twitter. When I start Nightly, instead of focusing on start page it focuses on the last app tab. When I unpinned Twitter, after new start Facebook showed up instead of Google home page. Should I fill a bug?
Comment 28 Tim Taubert [:ttaubert] 2012-07-06 06:25:45 PDT
(In reply to Peter Henkel [:Terepin] from comment #27)
> This patch may be causing and issue with app tabs. At least for me:
> I have 4 app tabs in following order: Google+, Gmail, Facebook and Twitter.
> When I start Nightly, instead of focusing on start page it focuses on the
> last app tab. When I unpinned Twitter, after new start Facebook showed up
> instead of Google home page. Should I fill a bug?

I can't reproduce this problem but feel free to file a bug to investigate this a little further if you can still reproduce it with all add-ons disabled.
Comment 29 Dão Gottwald [:dao] 2012-08-31 19:51:37 PDT
Andres, as per bug 771504 comment 41, could you prepare a backout patch for mozilla-beta, attach it here and ask for approval?
Comment 30 :Ehsan Akhgari 2012-09-01 07:14:34 PDT
Created attachment 657564 [details] [diff] [review]
Backout patch
Comment 31 Alex Keybl [:akeybl] 2012-09-02 14:33:50 PDT
Comment on attachment 657564 [details] [diff] [review]
Backout patch

[Triage Comment]
Approving this backout to resolve bug 771504 in FF16.

Note You need to log in before you can comment on or make changes to this bug.