Closed Bug 639852 Opened 11 years ago Closed 11 years ago

Port Bug 627642 [Don't restore session into a new window if only the home page is open]

Categories

(SeaMonkey :: Session Restore, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: misak.bugzilla, Assigned: misak.bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
From parent bug:

The original work in bug 588482 is pretty good, but it was pretty dumb in
determining if a window should be reused.

The most common case now is going to be a single tab (about:home). We can check
for a single about:home tab & no extData (which makes the panorama case
harder), no recently closed tabs, no history in that single tab. We could
probably ignore the extData case (potential for data loss but small) so that
entering panorama and pressing restore previous session doesn't suck.

With bug 592822 landing and soon bug 593421 and bug 589665, I think this is
going to be a much bigger deal than it was before.
Attachment #517756 - Flags: review?(neil)
Comment on attachment 517756 [details] [diff] [review]
patch

>+        tabbrowser.removeTab(removableTabs.pop(), { animate: false });
On an unrelated note, tabbrowser.removeTab actually already has a second parameter, aDisableUndo. I wonder how many problems that causes?
Comment on attachment 517756 [details] [diff] [review]
patch

>+    let homePages = aWindow.gHomeButton.getHomePage().split("|");
This is just aWindow.getHomePage() in our case.
(In reply to comment #1)
>(From update of attachment 517756 [details] [diff] [review])
>>+        tabbrowser.removeTab(removableTabs.pop(), { animate: false });
>On an unrelated note, tabbrowser.removeTab actually already has a second
>parameter, aDisableUndo. I wonder how many problems that causes?
It looks like browser_bug617076.js also calls removeTab with animate set to false. I guess I need to file a bug on supporting parmeters to removeTab.
Depends on: 642998
Attached patch updated patch with nit (obsolete) — Splinter Review
Attachment #517756 - Attachment is obsolete: true
Attachment #517756 - Flags: review?(neil)
Attachment #521479 - Flags: review?(neil)
Comment on attachment 521479 [details] [diff] [review]
updated patch with nit

>+    // Step 2 of processing:
Aside: This really belongs in a separate method. Then the caller can be
>// If there's a window already open that we can restore into, use that
>if (this._canUseWindow(windowToUse) {
>  let canOverwriteTabs = this._prepWindowToRestoreInto(windowToUse);

>+    let normalTabsLen = tabbrowser.tabs.length - tabbrowser._numPinnedTabs;
>+    for (let i = tabbrowser._numPinnedTabs; i < tabbrowser.tabs.length; i++) {
We don't have pinned tabs so this isn't going to work. I tried changing this to
>for (let i = 0; i < tabbrowser.tabs.length; i++) {
and this seems to make it work. If this is the correct fix, r=me.
Attachment #521479 - Flags: review?(neil) → review+
Pushed: http://hg.mozilla.org/comm-central/rev/1c816798b5df
Attachment #521479 - Attachment is obsolete: true
Attachment #521844 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.