868486 - Session restore logic appears broken in Browser preference pane

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[rsx11m]

There is a "When restoring sessions and windows" groupbox in the Edit > Preferences > Browser main tab. The dial in the "Restore [__] tab(s) at a time" box is apparently tied too directly into the respective preference.

Observed behavior:

Default:
  ( ) Restore immediately
  (*) Restore [ 3] tab(s)
  ( ) Restore when needed

Now, if you change to a different option, you see this:
  ( ) Restore immediately
  ( ) Restore [ 0] tab(s)
  (*) Restore when needed

or even this:
  (*) Restore immediately
  ( ) Restore [-1] tab(s)
  ( ) Restore when needed

and you need to click twice on the center option to get back to the "n tab(s)" selection. On the other hand, the dial allows you to go to "0" or "-1" as well, thus implicitly selecting the other options without even touching the radiobuttons.

This feature was introduced with bug 731140 and amended per bug 737260.

Comment 1

[rsx11m]

That's weird, bug 731140 is still open while parts of it are already available in the regular releases (behavior seen on 2.17.1 as well). Thus, I'm blocking that bug as well in case you want to keep this a separate issue. Otherwise, we can close this "Fixed as part of..." if and when it has been taken care of in the main bug.

Comment 2

[[e-mail]]

Because it's not clear what a setting of -1 or 0 means, I added the radiogroup to show what was going on. However a side-effect of that is that the radiogroup has to update the pref to -1 or 0 to take effect, thus setting the textbox. (Although, now that I think about it, it might be possible to avoid that by creative use of onsyncfrompreference.)

Comment 3

[[e-mail]]

Attachment: Creative use of onsyncfrompreference

I switched from explicitly testing against 0 and -1 to a simple > 0 comparison as I thought it looked clearer, however it breaks people setting the pref to -2 (but then, setting the pref to 2147483647 probably breaks anyway).

Updating the radiogroup from the pref works the same as before, values of 0 and -1 select the appropriate radiobutton while positive values are mapped to 3 which is an arbitrary value for the other radiobutton (arbitrary in the sense that the only reason I chose 3 is that it's the default value of the pref).

Updating the textbox from the pref cheats - the textbox does *not* update if the value is 0 or -1, by way of a hack that reads the current value as the value to write. The default value of the textbox is also set to 3, again because this is the default value of the pref, so that if the pref is currently 0 or -1 then it is conveniently simple to set it back to 3.

Changing the textbox sets the preference which automatically updates the radiogroup. It's therefore possible that I didn't need to change the code that updates the preference from the textbox, but that's not necessary for your feedback. (Note that clicking the textbox to change it focuses the radiogroup. That is probably a radiogroup bug.)

Changing the selected radiobutton also sets the preference, but nothing changes because the textbox doesn't update to 0 or -1 and if you select the third radiobutton then the textbox is already the correct value.

Comment 4

[rsx11m]

Comment on attachment 746068 [details] [diff] [review]
Creative use of onsyncfrompreference

Yes, this works better than before, but I'm seeing the following error in the Error Console whenever I change a radiobutton of that group:

> Error: document.getElementById(...) is null
> Source File: chrome://communicator/content/pref/pref-navigator.js
> Line: 270

You are looking for maxConcurrentTabsGroup there which seems nowhere defined.

In one instance I saw also those, but couldn't reproduce later:

> Error: document.getElementById(...).ReadConcurrentTabsRadio is not a function
> Source File: chrome://global/content/bindings/preferences.xml
> Line: 402

> Error: document.getElementById(...).ReadConcurrentTabsTextbox is not a function
> Source File: chrome://global/content/bindings/preferences.xml
> Line: 402

The textbox remains enabled even if I chose one of the other options, but it does no longer show "0" or "-1" values (and I can't select them manually). Thus, it's definitely going in the right direction.

Comment 5

[[e-mail]]

Attachment: Working patch

So, it turns out that the second time you open the Preferences window, onsyncfrompreference attributes in the default pane (depending on which window opened the Preferences window) cannot access any of the scripts for that pane. Everything works fine if you open another pane first and then switch.

As there's no easy fix for that, I've had to embed the code in the attribute, which does unfortunately make for a rather unwieldy line...

Comment 6

[rsx11m]

Comment on attachment 774215 [details] [diff] [review]
Working patch

(In reply to [e-mail] from comment #5)
> onsyncfrompreference attributes in the default pane (...)
> cannot access any of the scripts for that pane.

This reminds me of bug 835134 comment #25 where Firefox held the functions in a global variable to be able to access them to be able to use those, assuming that's the same scoping issue you are running into here.

Anyway, works fine for me now on both Linux and Windows platforms.

>+          <textbox id="maxConcurrentTabs" type="number" size="2"
>+                   min="1" value="3"

I'd define max="999" for that textbox. While 4 digits barely fit on Windows, I don't think there is much of a practical use case to keep 1000+ tabs open at a time.

Comment 7

[[e-mail]]

(In reply to rsx11m from comment #6)
> (In reply to comment #5)
> > onsyncfrompreference attributes in the default pane (...)
> > cannot access any of the scripts for that pane.
> 
> This reminds me of bug 835134 comment #25 where Firefox held the functions
> in a global variable to be able to access them to be able to use those,
> assuming that's the same scoping issue you are running into here.

No, it's slightly different, in that the functions get called before they have been loaded, because of a race condition in the order that elements get constructed.

Comment 8

[[e-mail]]

Comment on attachment 774215 [details] [diff] [review]
Working patch

Looks like I forgot to request review for this. Oops.

Comment 9

[[e-mail]]

Pushed comm-central changeset 44ca1df12431.

Comment 10

[[e-mail]]

Comment on attachment 774215 [details] [diff] [review]
Working patch

[Approval Request Comment]
Regression caused by (bug #): 731140/737260
User impact if declined: User confusion
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None

Comment 11

[[e-mail]]

http://hg.mozilla.org/releases/comm-aurora/rev/66f9ea142d33

Comment 12

[[e-mail]]

http://hg.mozilla.org/releases/comm-beta/rev/7910cdf27f07