The default bug view has changed. See this FAQ.

Restore pinned tabs before normal tabs

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Session Restore
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: zpao, Assigned: andreshm)

Tracking

unspecified
Firefox 16
Points:
---

Firefox Tracking Flags

(firefox16 disabled)

Details

(Whiteboard: [good first bug][mentor=zpao])

Attachments

(2 attachments, 6 obsolete attachments)

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)
Mark this as blocking bug 674452
(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

6 years ago
I'm working on this bug. I've just got some help of :zpao. Think I'll have a diff soon.

Comment 4

6 years ago
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.
Attachment #557365 - Flags: review?(paul)
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
Attachment #557365 - Flags: feedback-

Comment 6

6 years ago
Created attachment 557443 [details] [diff] [review]
Same diff as before but with more context

I've just added more lines to the diff.
Attachment #557365 - Attachment is obsolete: true
Attachment #557365 - Flags: review?(paul)
Attachment #557443 - Flags: review?(paul)

Comment 7

6 years ago
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.
Attachment #557443 - Attachment is obsolete: true
Attachment #557443 - Flags: review?(paul)
Attachment #557444 - Flags: review?(paul)
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.
Attachment #557444 - Flags: review?(paul)
Pablo, are you going to have the time to follow up and add some tests?

Comment 10

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

Comment 11

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

Comment 12

5 years ago
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?
Paul may correct me on this, but I expect a new test file in browser/components/sessionstore/test would be desired.

Comment 14

5 years ago
(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.
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

5 years ago
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.
Attachment #557444 - Attachment is obsolete: true
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → andres
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 18

5 years ago
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.
Attachment #619785 - Flags: review?(paul)
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.
Attachment #619785 - Flags: review?(paul) → feedback+
(Assignee)

Comment 20

5 years ago
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.
Attachment #619785 - Attachment is obsolete: true
Attachment #632909 - Flags: review?(paul)
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)
Attachment #632909 - Flags: review?(paul)
Attachment #632909 - Flags: review-
Attachment #632909 - Flags: feedback+
(Assignee)

Comment 22

5 years ago
Created attachment 634204 [details] [diff] [review]
patch v4

Applied suggestions.
Attachment #594621 - Attachment is obsolete: true
Attachment #632909 - Attachment is obsolete: true
Attachment #634204 - Flags: review?(paul)
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!
Attachment #634204 - Flags: review?(paul) → review+
Andres, did you have a chance to push it to try, yet?
https://hg.mozilla.org/integration/fx-team/rev/43c35b832235
Whiteboard: [good first bug][mentor=zpao] → [good first bug][mentor=zpao][fixed-in-fx-team]
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/43c35b832235
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=zpao][fixed-in-fx-team] → [good first bug][mentor=zpao]
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?
(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.
Depends on: 771504
Andres, as per bug 771504 comment 41, could you prepare a backout patch for mozilla-beta, attach it here and ask for approval?
Created attachment 657564 [details] [diff] [review]
Backout patch
Attachment #657564 - Flags: approval-mozilla-beta?
Comment on attachment 657564 [details] [diff] [review]
Backout patch

[Triage Comment]
Approving this backout to resolve bug 771504 in FF16.
Attachment #657564 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/a3790585b2a2
status-firefox16: --- → disabled
You need to log in before you can comment on or make changes to this bug.