Closed Bug 525700 Opened 15 years ago Closed 15 years ago

Satchel should consider history_expire_days_min in expiry calculation

Categories

(Toolkit :: Form Manager, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta3-fixed

People

(Reporter: MattN, Assigned: MattN)

Details

(Keywords: dataloss)

Attachments

(1 file, 2 obsolete files)

Satchel currently uses the pref browser.history_expire_days to determine how many days to keep form history entries (if browser.formfill.expire_days is not set).  This setting is no longer set in the interface as browser.history_expire_days_min is used now.  This means that if the user changes the number of days to keep their browsing history in the preferences, it won't be reflected in the form history expiry calculation.

Current calculation:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/src/nsStorageFormHistory.cpp#471

Expected results:
browser.history_expire_days_min should be taken into account otherwise there will appear to be dataloss if the user expects the form history to be around for history_expire_days_min days
Satchel can do like Places does at: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#2081

If history_expire_days < history_expire_days_min then use the value of history_expire_days_min
The test will just return if both of the the related preferences do not exist.
Assignee: nobody → mmn100+bmo
Status: NEW → ASSIGNED
Attachment #409616 - Flags: review?(dolske)
v.1 did not reflect the recent change to clearUserPref from bug 524995
Attachment #409616 - Attachment is obsolete: true
Attachment #409621 - Flags: review?(dolske)
Attachment #409616 - Flags: review?(dolske)
Attachment #409621 - Flags: review?(dolske) → review-
Comment on attachment 409621 [details] [diff] [review]
v.1.1 Update to reflect clearUserPref change (bug 524995)

r+ on the .cpp change, but I think the test change should be rolled into test_expire.js instead of added as a separate test. Best to have all the expire tests in one place.

I think it would be sufficient to just add a test that sets browser.history_expire_days_min to a large value, trigger an expiration, and make sure nothing went away. [Ie, right before the tests that trigger an expiration expecting the default prefs to cause something to be removed.]

AIUI, Places is going to unify these prefs in the next release, so the test doesn't need to be exhaustive.
(In reply to comment #4)
> r+ on the .cpp change, but I think the test change should be rolled into
> test_expire.js instead of added as a separate test. Best to have all the 
> expire tests in one place.
Tests are moved to test_expire.js

> I think it would be sufficient to just add a test that sets
> browser.history_expire_days_min to a large value, trigger an expiration, and
> make sure nothing went away. [Ie, right before the tests that trigger an
> expiration expecting the default prefs to cause something to be removed.]

I also kept the test case for when history is disabled, as that is a somewhat special case
Attachment #409621 - Attachment is obsolete: true
Attachment #411582 - Flags: review?(dolske)
Attachment #411582 - Flags: review?(dolske) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/096f8ec5e218
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment on attachment 411582 [details] [diff] [review]
v.2 consolidate tests

Would like to take this on 1.9.2 as well, it's low-risk and code here has good test coverage.
Attachment #411582 - Flags: approval1.9.2?
Attachment #411582 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 411582 [details] [diff] [review]
v.2 consolidate tests

a192=beltzner
Pushed to 192: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dc6ab8b03d3f

Note that bug 520165 will essentially back this out on trunk when it lands, because the Places prefs for expiring by age-in-days will be going away. But that won't land on 192, so this is fine there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: