Provide access to sessionstore tab data sooner

VERIFIED FIXED in Firefox 4.0b7

Status

()

defect
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: iangilman, Assigned: zpao)

Tracking

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

9 years ago
Posted patch suggest patch from zpao (obsolete) — Splinter Review
Tab Candy needs this; otherwise we don't get our tab location data right at start up.
Component: TabCandy → Session Restore
QA Contact: tabcandy → session.restore
Reporter

Updated

9 years ago
Duplicate of this bug: 591709
Comment on attachment 468764 [details] [diff] [review]
suggest patch from zpao

>   setTabValue: function sss_setTabValue(aTab, aKey, aStringValue) {
>-    if (!aTab.__SS_extdata) {
>+    // If the tab hasn't been restored, then set the data there, otherwise we
>+    // could lose newly added data.
>+    let saveTo;
>+    if (aTab.linkedBrowser.__ss_data && aTab.linkedBrowser.__ss_data.extData) {
>+      saveTo = aTab.linkedBrowser.__ss_data.extData;
>+    }

>+    else if (!aTab.__SS_extdata) {
>       aTab.__SS_extdata = {};
>+      saveTo = aTab.__SS_extdata;
>     }
>-    aTab.__SS_extdata[aKey] = aStringValue;
>+    else {
>+      saveTo = aTab.__extData;

Err that would need to be aTab.__SS_extData.

>+    }
>+    saveTo[aKey] = aStringValue;
>     this.saveStateDelayed(aTab.ownerDocument.defaultView);
>   },

We should also be able to just reduce the whole else, else if chunk to:

  else {
    saveTo = aTab.__SS_extData = aTab.__SS_extData || {};
  }

Mitcho, want to try applying this patch and see if this helps you?
Reporter

Updated

9 years ago
Duplicate of this bug: 596047
Reporter

Comment 5

9 years ago
One source of brittleness in Panorama is the fact that it sometimes takes a
while from when a tab is loaded until its sessionstore data is available. 

We've got code for this in the tabcandy-central branch; we should pull a patch
out of there.

Note that it's not just the revisions mitcho mentions in comment 3, nor is it the patch originally attached to this bug.
Priority: -- → P3
Ian, that sounds great. Can we assign this to you, or should it be zpao?
Assignee: nobody → ian
Reporter

Comment 7

9 years ago
I should be the one to pull it out of tabcandy-central at least.
Ian, Pull it out of tabcandy-central and attach the final patch.

I would say assign to mitcho or you even since you fixed it. It'd would be cheating a bit for me to review it, but we could talk to a peer about it if Dietrich isn't around to look at it.
Without this fix, the tabs are not assigned to their groups until they are already loaded.

This results in all tabs being automatically placed into the first group before they are sorted to where they're supposed to be. For one thing, this sorting only appears to occur when the Panorama view is actually open, and then only after each tab's preview has loaded.

In conjunction with bug 591704, this results in an extremely long wait after the browser first loads before the Panorama view is actually usable. When you first open the Panorama view, you get to watch each tab preview load and then move to the appropriate group. This happens very slowly, as the browser is also trying to actually load each individual tab. And if you exit the Panorama view before this sorting is done, it freezes its progress, leaving the first tab group with the wrong tabs and other tab groups missing tabs.
Reporter

Updated

9 years ago
Priority: P3 → P2
Reporter

Comment 10

9 years ago
Paul, how will this be affected by bug 595601? If we don't load background tabs, will we at least have access to their session data? If not, we'll have a serious problem.
Blocks: 597043
(In reply to comment #10)
> Paul, how will this be affected by bug 595601? If we don't load background
> tabs, will we at least have access to their session data? If not, we'll have a
> serious problem.

It should be ok. The extData is stuck on the tab no differently before. So long as the process currently in m-c for cascaded loading (bug 586068) sticks, everything should work fine.
Reporter

Comment 12

9 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Ok, here's what we had in tabcandy-central; seemed to work. 

I have no idea how one would go about writing a test for such a thing; it depends on loading a whole bunch of tabs all at once and catching it before they've actually loaded. Sounds like a recipe for a test with intermittent failures.
Attachment #468764 - Attachment is obsolete: true
Attachment #477764 - Flags: feedback?(paul)
Reporter

Updated

9 years ago
Blocks: 598795
Reporter

Updated

9 years ago
Status: NEW → ASSIGNED
Comment on attachment 477764 [details] [diff] [review]
patch v2

>   setTabValue: function sss_setTabValue(aTab, aKey, aStringValue) {
>-    if (!aTab.__SS_extdata) {
>+    // If the tab hasn't been restored, then set the data there, otherwise we
>+    // could lose newly added data.
>+    let saveTo;
>+    if (aTab.__SS_extdata) {
>+      saveTo = aTab.__SS_extdata;
>+    } 
>+    else if (aTab.linkedBrowser.__SS_data && aTab.linkedBrowser.__SS_data.extData) {

I'd just move the comment from above to here since it applies more directly.

>+      saveTo = aTab.linkedBrowser.__SS_data.extData;
>+    }
>+    else {

I know part of that is my code + the smartness you added. It looks good. Dietrich should review it since I wrote the first part.

As for testing, you could do this:
- setWindowState with a lot of tabs with unique data
- listen for SSTabRestoring (will be fired from each tab)
  - when you get it, discard that tab
- loop over tabs, if you haven't gotten SSTabRestoring, then you can check that tab
- if you have no tabs that haven't gotten SSTabRestoring, just mark the test as passed (or no tests).

It might be easier to actually do the check if you setBrowserState with a multiwindow state and observe domwindowopened. I can help out with the test if need be.
Attachment #477764 - Flags: feedback?(paul) → feedback+
I take it from the summary of this bug that (for some reason) Panorama doesn't have direct access to the session restore data? So it has to manipulate it between getting the data and displaying the tabs?

If so, that still leaves the actual data in the same order as if the tabs were opened linearly. For now (and I believe there's another bug on file to change this), that means that the actual about:sessionrestore page still shows the tabs in the order that they were opened.

This can be confusing when having to deal with the actual session restore page. Until that other bug is fixed, is there any way to have Panorama reorder the stored tab data so that tabs in the same group are at least placed next to each other?
(In reply to comment #14)
> This can be confusing when having to deal with the actual session restore page.
> Until that other bug is fixed, is there any way to have Panorama reorder the
> stored tab data so that tabs in the same group are at least placed next to each
> other?

I like the idea, but it's not really related to this bug. Please file a new bug in Firefox::Tab Candy for this.
(In reply to comment #14)
> opened linearly. For now (and I believe there's another bug on file to change
> this), that means that the actual about:sessionrestore page still shows the

Heh, I was the one who filed it: bug 591911.

(In reply to comment #15)
> (In reply to comment #14)
> > This can be confusing when having to deal with the actual session restore page.
> > Until that other bug is fixed, is there any way to have Panorama reorder the
> > stored tab data so that tabs in the same group are at least placed next to each
> > other?
> 
> I like the idea, but it's not really related to this bug. Please file a new bug
> in Firefox::Tab Candy for this.

Done: bug 598843.
Reporter

Comment 17

9 years ago
(In reply to comment #13)
> As for testing, you could do this:
> - setWindowState with a lot of tabs with unique data
> - listen for SSTabRestoring (will be fired from each tab)
>   - when you get it, discard that tab
> - loop over tabs, if you haven't gotten SSTabRestoring, then you can check that
> tab
> - if you have no tabs that haven't gotten SSTabRestoring, just mark the test as
> passed (or no tests).
> 
> It might be easier to actually do the check if you setBrowserState with a
> multiwindow state and observe domwindowopened. I can help out with the test if
> need be.

I think I understood about half of the above, so yes, some help would be great! Could you write the first draft (after you're done with b7, of course)?
This is one (of a few) of the root causes of the Panorama load being so slow, particularly right after session startup. Requesting blocking beta 8 or so.
blocking2.0: --- → ?
Reporter

Comment 19

9 years ago
Assigning to Paul to write the test.
Assignee: ian → paul
Reporter

Updated

9 years ago
Blocks: 591711
Reporter

Updated

9 years ago
Blocks: 602099
No longer blocks: 591711

Updated

9 years ago
Target Milestone: --- → Firefox 4.0
Paul, any word on the test?
The test. It's a little confusing to follow, but it will fail currently and passes with Ian's patch.
Attachment #482588 - Flags: review?(dietrich)
Comment on attachment 477764 [details] [diff] [review]
patch v2

Looking at the patch again, here's 2 nits:

>     return data[aKey] || "";
>   },
>-
>+  

Can you undo this whitespace change?

>   setTabValue: function sss_setTabValue(aTab, aKey, aStringValue) {
>-    if (!aTab.__SS_extdata) {
>+    // If the tab hasn't been restored, then set the data there, otherwise we
>+    // could lose newly added data.
>+    let saveTo;
>+    if (aTab.__SS_extdata) {
>+      saveTo = aTab.__SS_extdata;
>+    } 

^^ Please remove the trailing whitespace here too.
Attachment #477764 - Flags: review?(dietrich)
Reporter

Updated

9 years ago
Blocks: 594644
Reporter

Comment 23

9 years ago
Posted patch patch v3Splinter Review
(In reply to comment #22)
> Can you undo this whitespace change?

Done.

> ^^ Please remove the trailing whitespace here too.

Done.
Attachment #477764 - Attachment is obsolete: true
Attachment #484488 - Flags: review?(dietrich)
Attachment #477764 - Flags: review?(dietrich)
Reporter

Comment 24

9 years ago
This now blocks at least one blocker (bug 594644); can we get this marked as blocking as well?
blocking2.0: ? → betaN+
Comment on attachment 484488 [details] [diff] [review]
patch v3

changes look fine. canceling review though, needs a test.
Attachment #484488 - Flags: review?(dietrich)
Comment on attachment 484488 [details] [diff] [review]
patch v3

Test is attached separately.
Attachment #484488 - Flags: review?(dietrich)
Comment on attachment 484488 [details] [diff] [review]
patch v3

ah, i see. r=me.
Attachment #484488 - Flags: review?(dietrich) → review+
Reporter

Comment 28

9 years ago
(In reply to comment #27)
> Comment on attachment 484488 [details] [diff] [review]
> patch v3
> 
> ah, i see. r=me.

You're still planning on reviewing the test as well, right?
Attachment #482588 - Flags: review?(dietrich) → review+
Reporter

Comment 29

9 years ago
Thanks, Dietrich! 

Pushed to try: 0f1d9b3bd42d
Pushed http://hg.mozilla.org/mozilla-central/rev/85cf0af22be3 (patch) and http://hg.mozilla.org/mozilla-central/rev/e98bd46f2640 (test)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 4.0 → Firefox 4.0b8
Looks great! Thanks for the fix. Tested with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101027 Firefox/4.0b8pre and the TabView open while restarting Minefield. No shuffling of tabs between groups anymore.
Status: RESOLVED → VERIFIED
Version: unspecified → Trunk
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7

Updated

8 years ago
Blocks: 633126
You need to log in before you can comment on or make changes to this bug.