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)
Toolkit
Form Manager
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)
5.57 KB,
patch
|
Dolske
:
review+
beltzner
:
approval1.9.2+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Comment 2•15 years ago
|
||
The test will just return if both of the the related preferences do not exist.
Assignee | ||
Comment 3•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #409621 -
Flags: review?(dolske) → review-
Comment 4•15 years ago
|
||
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.
Assignee | ||
Comment 5•15 years ago
|
||
(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)
Updated•15 years ago
|
Attachment #411582 -
Flags: review?(dolske) → review+
Comment 6•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/096f8ec5e218
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Comment 7•15 years ago
|
||
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?
Updated•15 years ago
|
Attachment #411582 -
Flags: approval1.9.2? → approval1.9.2+
Comment 8•15 years ago
|
||
Comment on attachment 411582 [details] [diff] [review] v.2 consolidate tests a192=beltzner
Comment 9•15 years ago
|
||
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.
status1.9.2:
--- → final-fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•