681005 - Restore pinned tabs before normal tabs

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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]]

Mark this as blocking bug 674452

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(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]

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]

Attachment: 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)]

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]

Attachment: Same diff as before but with more context

I've just added more lines to the diff.

Comment 7

[Pablo Terradillos]

Attachment: 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 bugmail, email directly)]

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 bugmail, email directly)]

Pablo, are you going to have the time to follow up and add some tests?

Comment 10

[Pablo Terradillos]

Yes Paul, sorry. I'll have it by the end or this week (I had some issues with my pc)

Comment 11

[Pablo Terradillos]

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]]

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]]

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]]

(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]]

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]

Attachment: 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 bugmail, email directly)]

(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]]

Attachment: 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 bugmail, email directly)]

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]]

Attachment: 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 bugmail, email directly)]

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]]

Attachment: patch v4

Applied suggestions.

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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] (inactive)]

Andres, did you have a chance to push it to try, yet?

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/43c35b832235

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/43c35b832235

Comment 27

[Peter Henkel [:Terepin]]

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] (inactive)]

(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]]

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

[(no longer active)]

Attachment: Backout patch

Comment 31

[Alex Keybl [:akeybl]]

Comment on attachment 657564 [details] [diff] [review]
Backout patch

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

Comment 32

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3790585b2a2