Ian Gilman [:iangilman] Reporter
	Description • 14 years ago

Tab Candy needs this; otherwise we don't get our tab location data right at start up.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 2 • 14 years ago

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?

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 3 • 14 years ago

zpao's patch (with a further correction from him) has been committed to tabcandy-central for testing by the Panorama team:

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/46873e7c6f83
http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/1f889ee708f9

	Ian Gilman [:iangilman] Reporter
	Comment 5 • 14 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.

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 6 • 14 years ago

Ian, that sounds great. Can we assign this to you, or should it be zpao?

	Ian Gilman [:iangilman] Reporter
	Comment 7 • 14 years ago

I should be the one to pull it out of tabcandy-central at least.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 8 • 14 years ago

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.

	Gordon P. Hemsley [:GPHemsley]
	Comment 9 • 14 years ago

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.

	Ian Gilman [:iangilman] Reporter
	Comment 10 • 14 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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 11 • 14 years ago

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

	Ian Gilman [:iangilman] Reporter
	Comment 12 • 14 years ago

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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 13 • 14 years ago

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.

	Gordon P. Hemsley [:GPHemsley]
	Comment 14 • 14 years ago

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?

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 15 • 14 years ago

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

	Gordon P. Hemsley [:GPHemsley]
	Comment 16 • 14 years ago

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

	Ian Gilman [:iangilman] Reporter
	Comment 17 • 14 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)?

	Michael Yoshitaka Erlewine [:mitcho]
	Comment 18 • 14 years ago

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.

	Ian Gilman [:iangilman] Reporter
	Comment 19 • 14 years ago

Assigning to Paul to write the test.

	Aza Raskin [:aza]
	Comment 20 • 14 years ago

Paul, any word on the test?

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 21 • 14 years ago

The test. It's a little confusing to follow, but it will fail currently and passes with Ian's patch.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 22 • 14 years ago

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.

	Ian Gilman [:iangilman] Reporter
	Comment 23 • 14 years ago

(In reply to comment #22)
> Can you undo this whitespace change?

Done.

> ^^ Please remove the trailing whitespace here too.

Done.

	Ian Gilman [:iangilman] Reporter
	Comment 24 • 14 years ago

This now blocks at least one blocker (bug 594644); can we get this marked as blocking as well?

	Dietrich Ayala (:dietrich)
	Comment 25 • 14 years ago

Comment on attachment 484488 [details] [diff] [review]
patch v3

changes look fine. canceling review though, needs a test.

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 26 • 14 years ago

Comment on attachment 484488 [details] [diff] [review]
patch v3

Test is attached separately.

	Dietrich Ayala (:dietrich)
	Comment 27 • 14 years ago

Comment on attachment 484488 [details] [diff] [review]
patch v3

ah, i see. r=me.

	Ian Gilman [:iangilman] Reporter
	Comment 28 • 14 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?

	Ian Gilman [:iangilman] Reporter
	Comment 29 • 14 years ago

Thanks, Dietrich! 

Pushed to try: 0f1d9b3bd42d

	Paul O'Shannessy [:zpao] (not bugmail, email directly) Assignee
	Comment 30 • 14 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/85cf0af22be3 (patch) and http://hg.mozilla.org/mozilla-central/rev/e98bd46f2640 (test)

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 31 • 14 years ago

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.