586147 - Restore visible tabs (and hide hidden ones)

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Ed Lee :Mardak]

With bug 582116, m-c will have an api to hide/show tabs. tabcandy-central has augmented sessionstore to restore that information with bug 583306.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Should this block bug 574217?

Comment 2

[(no longer active)]

(In reply to comment #1)
> Should this block bug 574217?

Yes.

Comment 3

[(no longer active)]

Mardak, do you think you can take this?  It should be pretty simple (the code change is already there, you only need to write a test for it.)  I'm trying to work on debugging the leaks...

Comment 4

[Ed Lee :Mardak]

Attachment: v1

Comment 5

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

Comment on attachment 464698 [details] [diff] [review]
v1

>+	browser_hidden_tabs.js \

Please name the test browser_586147.js. While it would be great to have a filename that indicates a feature it's testing, we don't roll like that.

It looks fine, it's just missing something. My concern is that the tab loading order is going to be wrong now - that is, the order we load history into tabs & actually kick off the requests is going to include !visible tabs, and those should be last. As is, if you have 50 !visible tabs "first" and then your other tabs, it's quite possible that those 50 !visible tabs are going to load first or at least before some of your visible tabs, which is suboptimal.

The tests for bug 480148 don't do any checks for !visible tabs, but we should probably add at least one.

Comment 6

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

Comment on attachment 464698 [details] [diff] [review]
v1

I said as much on IRC, but I'll say it here for a nice record.

It's not the worst thing to break (temporarily). Bug 586211 has been filed for a followup and Aza has assured me it will be fixed in a week.

r=zpao with test file name change.

Comment 7

[Ed Lee :Mardak]

Attachment: v1.1

Comment 8

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

(In reply to comment #5)
> Please name the test browser_586147.js. While it would be great to have a
> filename that indicates a feature it's testing, we don't roll like that.

Why shouldn't we roll like that, for new tests? I don't think consistency in test file naming is important.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 464734 [details] [diff] [review]
v1.1

a=beltzner, a momentous occasion!

Comment 10

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

(In reply to comment #8)
> (In reply to comment #5)
> > Please name the test browser_586147.js. While it would be great to have a
> > filename that indicates a feature it's testing, we don't roll like that.
> 
> Why shouldn't we roll like that, for new tests? I don't think consistency in
> test file naming is important.

Meh, it's not important - I opted for consistency. I could do with some boat rocking though. If you leave it as is, just make sure you add a comment in the test file indicating which bug it originated in to save having to look at hg annotate.

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/90b492f96ceb
Port (copy) sessionstore changes from bug 583306 and add tests.