197466 - Back button (session history) has 50 entries regardless of setting in prefs.js

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[zug_treno]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312

If I set the browser.sessionhistory.max_entries setting to either a lower (10)
or a higher (250) number, the actual number of entries stays at default (50). 

Reproducible: Always

Steps to Reproduce:
1. Change browser.sessionhistory.max_entries to another number using about:config.
2. Surf to more then that number (or 50: whatever number is lower) pages.
3. Use the dropdown list from back button to go back as far as possible.

Actual Results:  
It will go back to up to 50 entries.

Expected Results:  
It should go back up to the number set in browser.sessionhistory.max_entries. 

This happens with both an old profile and a brand new profile, using the classic
theme.

Comment 1

[Radha on family leave (not reading bugmail)]

Could this bug be something to do with the recent changes in that part of the code?

Comment 2

[Boris Zbarsky [:bzbarsky]]

See bug 126826.  Basically, the user pref for this was removed; non-user prefs
(from all.js) will still work.

We _could_ follow the user_pref if and only if it's set to be above the non-user
one, perhaps....

Comment 3

[John Morrison]

I'd suggest wontfix. Hack your own copy of 'all.js' if you really want 714 
session history entries.

Comment 4

[zug_treno]

I changed pref("browser.sessionhistory.max_entries", 50); in all.js, but 50 is
still the magic number. Are there any other settings or files that should be
changed?

Comment 5

[R.K.Aa.]

Is this also the case if you quit mozilla (and quicklaunch) before editing
prefs.js or all.js?

Comment 6

[John Morrison]

worksforme. setting |pref("browser.sessionhistory.max_entries", 100);| in 
all.js allows me to get more than 50 entries in session history.

Comment 7

[John Morrison]

oops

Comment 8

[John Morrison]

wontfix

Comment 9

[zug_treno]

All.js setting WFM after some experimentation: it picked up the new default
value after I deleted the backup copy of all.js in the same directory. I really
hope this all.js setting won't dissapear in future releases like the one in
prefs.js. Everybody thanks for the help.

Comment 10

[zug_treno]

Nowadays the browser.sessionhistory.max_entries setting can be found in
(\Program Files\mozilla.org\Mozilla\defaults\pref\) browser-prefs.js.

Comment 11

[OstGote!]

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

Comment 12

[zug_treno]

Attachment: Simple patch to follow user prefs (> 10 entries) again

(In reply to comment #2)
> We _could_ follow the user_pref if and only if it's set to be above the non-user
> one, perhaps.

It seems that Minimo has the lowest default pref (10 entries), so this patch uses the browser.sessionhistory.max_entries again but any value under 10 is increased to 10.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Wouldn't it make more sense to disallow setting it to below the default, but allow setting it above the default, whatever the default is?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Put another way, I don't want a magic constant here.

Comment 15

[zug_treno]

Attachment: Simple patch to follow user prefs (> default) again

Updated patch, the number of session history entries is made equal to the default number of entries if browser.sessionhistory.max_entries is smaller than the default value.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 304817 [details] [diff] [review]
Simple patch to follow user prefs (> default) again

Can you do a patch without the tabs (or with, if the file has them)?  That would also make it a lot easier to review...

Comment 17

[zug_treno]

Attachment: Simple patch to follow user prefs (> default) again, without tabs

Same patch as before, but with tabs replaced by spaces.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 304832 [details] [diff] [review]
Simple patch to follow user prefs (> default) again, without tabs

>Index: docshell/shistory/src/nsSHistory.cpp
>+static PRInt32  gDefaultHistoryMaxSize;

Why bother having a static for this instead of just a stack local?

With that just put on the stack, looks reasonable.

Comment 19

[zug_treno]

Attachment: Simple patch to follow user prefs (> default) again, with local variable

Made defaultHistoryMaxSize a local variable.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 304945 [details] [diff] [review]
Simple patch to follow user prefs (> default) again, with local variable

>Index: docshell/shistory/src/nsSHistory.cpp
>+    PRInt32  defaultHistoryMaxSize;

Please init this to 50 to start with.

With that change, r=bzbarsky.

Comment 21

[zug_treno]

Attachment: Simple patch to follow user prefs (> default) again, init variable to 50

Initializes defaultHistoryMaxSize to 50.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 305047 [details] [diff] [review]
Simple patch to follow user prefs (> default) again, init variable to 50

I can just r+sr this.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Er, except this would need approval to land on trunk at this point.

Comment 24

[Damon Sicore (:damons)]

Comment on attachment 305047 [details] [diff] [review]
Simple patch to follow user prefs (> default) again, init variable to 50

a1.9+=damons

Comment 25

[Reed Loden [:reed]]

Checking in docshell/shistory/src/nsSHistory.cpp;
/cvsroot/mozilla/docshell/shistory/src/nsSHistory.cpp,v  <--  nsSHistory.cpp
new revision: 1.85; previous revision: 1.84
done