99243 - Invalid history pref entry is reset to 0, not previous value

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect, P3)

Description

[Claudius Gayle]

this little nugget sprung from bug 81415.

In the Navigator-->History pref panel invalid entries into the session history
textfield are reset to '0' when the prefs window is closed.

to reproduce:
1. Choose Edit-->Preferences.
2. Select the Navigator-->History panel.
3. In either textfield, enter and invalid value e.g. 'p'.
4. Click 'OK'
5. Reopen prefs to the same panel.
Actual results:
The text entry is reset to '0' which is neither the previous entry OR the
default setting.
Expected results:
invalid entry should be reset to previous value to avoid the
destructive/unexpected possibilities of expirig history after 0 days. If
remembering the previous value turns out to be challenging, default values could
be a stop-gap measure.

Comment 1

[sairuh (rarely reading bugmail)]

possible backend issue?

Comment 2

[Brian Nesse (gone)]

No, this is a UI issue. There was a similar bug to this filed about setting the
cache size to a value and that value being converted to a different number. The
issue has something to do with automatic value conversion in XUL/JS as I recall.

Adding eddyk as I think he did the investigation/work on the cache value bug.

Comment 3

[Claudius Gayle]

go read bug 81415, that's why i referenced it and bugzilla provides a nice
hotlink. One could probably isolate blake's exact checkin related to this bug.

Comment 4

[eddyk]

I haven't looked into this issue very closely, but it does seem to be similar to
the bug 63117.

I wonder what the status of bug 66163 is?  That would take care of this and
other similar issues.

Other possible solutions would be to make nsPrefWindow/nsWidgetStateManager
handle these cases more robustly, or to have validation js code in every pref panel.

Comment 5

[Brian Nesse (gone)]

Eddy, yes bug 63117 is the one I was thinking of.

Claudius, I have read bug 81415, which references bug 66163 as being the right fix.

Blake's change (in pref-history.xul) forces the text input field to be 0 if it
is invalid:
  if (maxSize < 0) {
    // set the pref to 0 so the UI reflects the proper value
    if (panelIsShown)
      document.getElementById("shistMax").value = "0";
    else
      data['shistMax'].value = "0";
 
    maxSize = 0;
  }

If the backend prefs code is told to set the value to 0, it will. On the other
hand, if the backend prefs code is not called to change the value, the value
will remain unchanged. Either way, the backend is only doing what the frontend
is telling it to do.

The value of "browser.sessionhistory.max_entries" in prefs.js will reflect the
last value given to the pref backend.

Comment 6

[Samir Gehani]

Moving to mozilla0.9.7.

Comment 7

[Samir Gehani]

-> mozilla1.0.1

Comment 8

[Claudius Gayle]

I'm still a little skittish about setting people's history pref to '0'. Is there
not some simple change to be made to mediate the behavior in the meantime. If we
can't completely fix it before 1.0.1?

Comment 9

[Samir Gehani]

-> History for stop-gap measure.

Comment 10

[Blake Ross]

Bug 66163 is the real fix for this.  I agree, we shouldn't default to 0. I'm
going just take the easy way out and set to 50 (the default).

Comment 11

[Blake Ross]

Attachment: patch

r=ben
sr=hewitt

Comment 12

[Blake Ross]

Fix checked in. The patch also fixes entering non-alphanumeric characters, so
that should be tested when verifying.

Note that currently entering 0 into *any* textbox in preferences, pressing OK,
and then reopening prefs shows no value in that textbox, instead of 0 (even
though the pref was correctly set). So I didn't introduce that bug, but I'm
going to file it and try to fix it.

Comment 13

[David Illsley]

*** Bug 120311 has been marked as a duplicate of this bug. ***