Closed Bug 453440 Opened 16 years ago Closed 16 years ago

Extend "Clear Private Data" dialog with time period option

Categories

(Firefox :: Settings UI, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: bugzilla, Assigned: johnath)

References

(Depends on 1 open bug, )

Details

(Keywords: user-doc-complete, verified1.9.1)

Attachments

(4 files, 7 obsolete files)

Steps to reproduce:
1. go to /Tools/Clear Private Data.../
2. Click on "Clear Private Data Now"

The above will clear ALL data that you selected. It would be nice if you could define a time period of what to clear.

Google Chrome offers a "Clear data from this period" dropdown where you can select a time period of what to delete.

Default option could be "Everything" with other time frame options such as "this session only", "last day" etc.
OS: Windows Vista → All
Hardware: PC → All
Version: 3.0 Branch → unspecified
Full privacy UI mockup is here, which includes this window
http://people.mozilla.com/~faaborg/files/shiretoko/privacy_i2.png

Assigning to johnath since he is listed as the owner at
https://wiki.mozilla.org/Firefox3.1/Features/Beta_2_Additions
Assignee: nobody → johnath
So, not all aspects of private data have time-period controls for eviction, in particular, as far as I can tell:

- Browser History is fully date-scopable
- Download History looks like I can iterate through and do something smart
- The cache service doesn't have a date-based eviction API
- Cookies don't report their when-was-I-set date, as far as I can tell
- Offline app storage just has a global clear function
- Form history is per-entry or globally clearable, but not in a date-specific way
- Passwords aren't currently stored with creation times, as far as I know
- Authenticated sessions don't have a date-specific API, just a global clear function.

This is not to say that we can't add date tracking to some of these (it just means a new field in the password db, for instance) but that is a lot of potential dependency work before this bug could be realized as designed.

So what we need to figure out (really quickly if we want it done for string freeze) is whether we want:

1) to defer this, and do it right "later"
2) to do this now for the things that already support it and design the interface to make that clearer
3) to do this now for the things that already support it, leave the interface design as-is, but risk confusing/surprising/deceiving users, and potentially treat the other requirements as "bugs" in the implementation.

I think my preference is to do #2, but I'm keen to get Alex's thoughts here on keeping things clear while doing so.
(In reply to comment #3)
> - Download History looks like I can iterate through and do something smart

I think you'd be better off working with the DB table if that's possible.

> - The cache service doesn't have a date-based eviction API
> - Cookies don't report their when-was-I-set date, as far as I can tell

We filed bug 411952 along the way of implementing the private browsing mode, but it was never fixed due to the fact that it was removed from the blocker list of the PB bug.  I actually have a patch there with r+ awaiting bz's sr.

> - Authenticated sessions don't have a date-specific API, just a global clear
> function.

Thinking of it, I don't know how much sense does time-based clearing make for authenticated sessions.  Do we really want to clear the active logins in the last hour?
Depends on: 411952
No, that's why I prefer #2, because I think the pleasant thing to do here is to reflect the fact that some things, like visits, are inherently temporal in a way that other things, like authenticated sessions, aren't so much.  I think Alex is getting at that with the History/Data divide, so I'm hoping we can iterate there towards something that really works.

A middle ground might be to just have "This Session" vs. "Everything" because then we could, for instance, clear all authenticated sessions either way, and eliminate that point of ambiguity.  It sort of sucks in the history case because there we DO have more control, but that might just argue for a design which calls those out separately, or for different UIs, as some of alex's mockups have favoured.
The time scope should only apply to history, not user data.  The idea being
that history is stuff that Firefox implicitly creates (like a cookie) while
user data is stuff the user explicitly creates (like a bookmark) So to break
apart your list:

History (needs to clear by time):
- Browser History is fully date-scopable
- Download History looks like I can iterate through and do something smart
- The cache service doesn't have a date-based eviction API
- Cookies don't report their when-was-I-set date, as far as I can tell
- Form history is per-entry or globally clearable, but not in a date-specific
way
- Authenticated sessions don't have a date-specific API, just a global clear
function.

Data (does not need to clear by time):
- Offline app storage just has a global clear function
- Passwords aren't currently stored with creation times, as far as I know

If the user is wondering "which is better, clear recent history or private
browsing?" I would like for the answer to be "they are equally effective at
clearing your history."  If we start by having one better than the other, than
people won't view this feature as the type of "undo" that we mean it to be, but
more of a "kind of undo."  Or to put it another way, I think we should be able
to say:

   Clear Recent History

instead of 

   Clear Recent "History"

I would probably prefer we do this right later, along with the other stuff like
the theme change for private browsing in 3.2, and to completely finish bug
462041 for 3.1
Attached patch WIP (obsolete) — Splinter Review
Does most of the dialog reorg and adds the strings, plus a new pref to remember how long we choose to clear for.

Doesn't have the implementation of the various clearing regimens - mconnor is working on some of those APIs, I believe.

Next step is actually wiring it up.
Depends on: 462964
Attached patch More WIP (obsolete) — Splinter Review
Proper timespans; cookies and history wired up; still need downloads and an answer on form data, even if we decide to handle that in a follow-up
Attachment #345499 - Attachment is obsolete: true
Attached image Appearance with WIP (obsolete) —
Attached patch With tests (obsolete) — Splinter Review
This is now ready for review, a few notes:

 - This relies on Shawn's reviewed, waiting-for-landing, patch in bug 462964 to add remove functionality to nsIDownloadManager
 - This does not include dolske's work in bug 463154 to make form history time indexable - as discussed with mconnor, that should be beta-blocking followup.
 - This re-arranges the fields as anticipated by bug 460341, this patch will resolve that bug as well
Attachment #346169 - Attachment is obsolete: true
Attachment #346382 - Flags: ui-review?(beltzner)
Attachment #346382 - Flags: review?(mconnor)
Comment on attachment 346382 [details] [diff] [review]
With tests

Reviewing the dialog only ..:

 |                                                     |
 | History --------------------------------------------|
 |                                                     |
 |  [x] Visited Pages       [ ] Cookies                | [1]
 |  [x] Download List       [ ] Form & Search Entries  |
 |  [x] Web Cache                                      | [2]
 |                                                     |
 |  Remove [ my entire history ]                       | [3]
 |                                                     |
 | Data -----------------------------------------------|
 |  [ ] Saved Passwords     [ ] Offline Website Data   |
 |                                                     |
 |                ( Cancel )  (( Clear Private Data )) | [4]
 '-----------------------------------------------------'

[1] made some string changes here since we didn't need to be repeating the word "History" over and over again; "Download List" is there instead of "Downloads" or "Downloaded Files" to avoid potential misinterpretation of "delete the files themselves"

[2] Active Logins, Authenticated Sessions, whatever you call them, they're not that meaningful to users. Yes, it's a pain that there's no way to log out of HTTP-Authenticated Sites, but for people who really care we can write an add-on, and for people who are just looking for a quick way of doing it there's always Quit & Save State. It's confusing and should be dropped.

[3] The time scale only affects history, and so it should be associated with that groupbox. Further, it should be written in clear task form. Don't forget the empty entity after the drop-down for localizers who might not subscribe to our superior subject-verb-object structure. The complete drop down would be:

    my entire history
    history from today
    the last 4 hours
    the last 2 hours
    the last hour

[4] Remove the "Now" from "Clear Private Data Now"; it was a poor way of tracking the difference between instant apply and non instant apply
Attachment #346382 - Flags: ui-review?(beltzner) → ui-review-
Blocks: 463177
I've also filed bug 463186, bug 463180 and bug 463187 which will likely be solved by this patch, and are kit and kaboodle with the changes to the Clear Private Dialog function that wholistically make sense with the design posited in comment 12
No longer blocks: 463177
Attached patch Now with more changes! (obsolete) — Splinter Review
Don't review the tests yet - I just realized that they will fail between midnight and 4am, when the "clear the last 4 hours" setting also, implicitly, clears all of today's history.  Gotta clean that up.
Attachment #346382 - Attachment is obsolete: true
Attachment #346415 - Flags: review?(mconnor)
Attachment #346382 - Flags: review?(mconnor)
Flags: blocking-firefox3.1+
Comment on attachment 346415 [details] [diff] [review]
Now with more changes!

Clearing review request - new patch incoming shortly
Attachment #346415 - Flags: review?(mconnor)
Attached patch Okay. Review this one. (obsolete) — Splinter Review
Attachment #346415 - Attachment is obsolete: true
Attachment #346498 - Flags: ui-review?(beltzner)
Attachment #346498 - Flags: review?(mconnor)
Attachment #346498 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 346498 [details] [diff] [review]
Okay.  Review this one.

Nits:
 - looks like there's now more blank space than there used to be, we should file a follow up polish bug in the various theme components about getting that theming right
 - same again with the alignment between "Remove" and the drop-down with options
Carrying forward ui-r, just need review and we can boat this bass.
Attachment #346498 - Attachment is obsolete: true
Attachment #346534 - Flags: ui-review+
Attachment #346534 - Flags: review?(mconnor)
Attachment #346498 - Flags: review?(mconnor)
Attached image Appearance on Mac
Attachment #346170 - Attachment is obsolete: true
Blocks: 463180
Status: NEW → ASSIGNED
Johnathan - the tests here don't seem to make sure that the cache is cleared.
(In reply to comment #20)
> Johnathan - the tests here don't seem to make sure that the cache is cleared.

The tests added here relate to the timespan-specific logic added by the bug - they're not really claiming to be all-encompassing CPD tests...
Comment on attachment 346534 [details] [diff] [review]
With UI-nits, more string changes

Ok, as discussed just now, going to refactor this in a few key ways:

* instead of passing ignoreTimespan, we'll pass range,
* getClearRange will only return a value if .ignoreTimespan is false, otherwise will be null
* As discussed, passwords and offline web data should return false in canClear if range is non-null

>-        const cc = Components.classes;
>-        const ci = Components.interfaces;
>-        var cacheService = cc["@mozilla.org/network/cache-service;1"]
>-                             .getService(ci.nsICacheService);
>+        const Cc = Components.classes;
>+        const Ci = Components.interfaces;
>+        var cacheService = Cc["@mozilla.org/network/cache-service;1"]
>+                             .getService(Ci.nsICacheService);

use the other, preferred style for this, please.

>+Sanitizer.getClearRange = function() {
>+  var ts = Sanitizer.prefs.getIntPref("timeSpan");
>+  if(ts === Sanitizer.TIMESPAN_EVERYTHING)
>+    return null;

nit: space before (

>+  // PRTime is microseconds while JS time is milliseconds
>+  var endDate = Date.now() * 1000;
>+  switch (ts) {
>+    case Sanitizer.TIMESPAN_HOUR :
>+      var startDate = endDate - 3600*1000000;
>+      break;
>+    case Sanitizer.TIMESPAN_2HOURS :
>+      startDate = endDate - 2*3600*1000000;
>+      break;
>+    case Sanitizer.TIMESPAN_4HOURS :
>+      startDate = endDate - 4*3600*1000000;
>+      break;

check if these get optimized away, otherwise just expand them out (and comment what the calc is, if you want)

getting there, but I'll do one more pass after you refactor the sanitize bits
Attachment #346534 - Flags: review?(mconnor) → review-
Blocks: 463343
Attached patch With nitsSplinter Review
(In reply to comment #22)
> Ok, as discussed just now, going to refactor this in a few key ways:
> 
> * instead of passing ignoreTimespan, we'll pass range,

In fact, we'll set that on the items and stop passing anything, as discussed.

> * getClearRange will only return a value if .ignoreTimespan is false, otherwise
> will be null
> * As discussed, passwords and offline web data should return false in canClear
> if range is non-null

As discussed just now, I have broken out the question of how these fields/prefs should behave to bug 463343

> >-        const cc = Components.classes;
> >-        const ci = Components.interfaces;
> >-        var cacheService = cc["@mozilla.org/network/cache-service;1"]
> >-                             .getService(ci.nsICacheService);
> >+        const Cc = Components.classes;
> >+        const Ci = Components.interfaces;
> >+        var cacheService = Cc["@mozilla.org/network/cache-service;1"]
> >+                             .getService(Ci.nsICacheService);
> 
> use the other, preferred style for this, please.

Done.

> >+Sanitizer.getClearRange = function() {
> >+  var ts = Sanitizer.prefs.getIntPref("timeSpan");
> >+  if(ts === Sanitizer.TIMESPAN_EVERYTHING)
> >+    return null;
> 
> nit: space before (

Grr!  Nice catch.  Done.

> >+  // PRTime is microseconds while JS time is milliseconds
> >+  var endDate = Date.now() * 1000;
> >+  switch (ts) {
> >+    case Sanitizer.TIMESPAN_HOUR :
> >+      var startDate = endDate - 3600*1000000;
> >+      break;
> >+    case Sanitizer.TIMESPAN_2HOURS :
> >+      startDate = endDate - 2*3600*1000000;
> >+      break;
> >+    case Sanitizer.TIMESPAN_4HOURS :
> >+      startDate = endDate - 4*3600*1000000;
> >+      break;
> 
> check if these get optimized away, otherwise just expand them out (and comment
> what the calc is, if you want)

Expanded and commented.
Attachment #346534 - Attachment is obsolete: true
Attachment #346602 - Flags: ui-review+
Attachment #346602 - Flags: review?(mconnor)
Attachment #346602 - Flags: review?(mconnor) → review+
http://hg.mozilla.org/mozilla-central/rev/7fc3096ef585
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: user-doc-needed
Target Milestone: --- → Firefox 3.1b2
Version: unspecified → Trunk
This has automated tests and I don't think the changes here need litmus tests, but I'm setting in-litmus? since existing CPD tests may need to be rewritten given the interface changes.
Flags: in-testsuite+
Flags: in-litmus?
Blocks: 463177
No longer blocks: 463117
There is a good chunk of Litmus test cases that will have to be modified due to this new functionality, as well as the renaming of the feature. This is on my list of things to complete.
I can verify this bug in general, but what I am seeing in the UI does not match exactly what beltzer details in Comment 12. There are six entries under History, and they are not rendered in the same order he has in that diagram. I guess we should go by https://bugzilla.mozilla.org/attachment.cgi?id=346535?
https://litmus.mozilla.org/show_test.cgi?id=7401 added to Litmus BFT and FFT as a general test, more will be added later.
Flags: in-litmus? → in-litmus+
I am reopening this bug because the options in the drop-list are not respected and don't appear to work. 

I used a test profile, no add-ons, that had some history.
I then browsed to a few sites to generate some 'new' history.
Using CPD I chose first option, 'clear the last hour'
I then checked my History to find it ALL missing. 

Tested using changeset:
http://hg.mozilla.org/mozilla-central/rev/18fec592b35b
and hourly build:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081107 Minefield/3.1b2pre Firefox/3.0 ID:20081107001016
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 463607
I can confirm with windows build built from http://hg.mozilla.org/mozilla-central/rev/18fec592b35b
(In reply to comment #29)
> I am reopening this bug because the options in the drop-list are not respected
> and don't appear to work. 

GRR!

I tested this extensively both manually and with automated tests, and still this crept through.

When we moved range setting to be a global thing instead of being checked on each item, we exposed a bug that I should have caught - sanitize.xul does it's own little song and dance, calls its own sanitize method instead of using the one in sanitize.js.  That one wasn't setting time ranges on anything, which wasn't a problem when each clear operation was checking hte time range on its own, but is a problem when they are counting on it to be set elsewhere.

Stevee, littlemutt, while I get mconnor to review this, can you confirm that this makes life better?  I have tested that, but I am suffering currently from a lack of confidence in my own testing abilities!  :\
Attachment #346902 - Flags: review?(mconnor)
Johnathan, don't beat up on yourself too much.  I will be happy to test once you push this fix..
Comment on attachment 346902 [details] [diff] [review]
Smack sanitize.xul into line

Oy.
Attachment #346902 - Flags: review?(mconnor)
Attachment #346902 - Flags: review+
Attachment #346902 - Flags: approval1.9.1b2+
Jonathan: In doing further testing I also saw that we blow away all history when it is imported from another browser. Can we make sure that doesn't happen when this fix lands?
(In reply to comment #34)
> Jonathan: In doing further testing I also saw that we blow away all history
> when it is imported from another browser. Can we make sure that doesn't happen
> when this fix lands?
That sounds like it is a bug in places - not related to clear private data.
I cloned moz-central, applied attachment 346902 [details] [diff] [review] and built my tree and I still saw the problem (Clearing last hour of history cleared everything). However I'm a total newfish at building and applying patches, so I'm guessing i screwed something up somewhere :-)
http://hg.mozilla.org/mozilla-central/rev/6626d9bddd68
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Using hourly build: 
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081108 Minefield/3.1b2pre Firefox/3.0 ID:20081108002132

I still see the problem remaining, all history is cleared when using 'clear last hour'.

I also noted a crash when trying to clear just a few mins of history on a clean profile:
1.  New profile
2.  surf around a few sites
3.  Clear Private Data
4.  Last hour
5.  note Crash

while useless because its an hourly build, here is crash report:
http://crash-stats.mozilla.com/report/index/cc657a35-ad7c-11dd-8a93-001a4bd43e5c
Status: RESOLVED → VERIFIED
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Okay, so now there's something weird going on because I can't reproduce what you're seeing. Here's what I did:

1) Created a new profile
2) Copied over places.sqlite and friends from my normal profile, since it's full of long-term and recent visit information
3) Started up with new profile
4) Checked the library to make sure that all the visits show up, they do
5) Delete the last hour - confirm that only the last hour is deleted
6) Ditto last two hours, last 4, and today

But obviously you're seeing different behaviour, so I wonder what's different between our scenarios here?

One thing that comes to mind as a sort of a longshot:  when I was testing before, instead of copying sqlite, I would just play clock tricks.  So I would visit some sites in a testing profile, and then move my clock ahead by a few hours and see if the dialog behaved correctly.  What I quickly learned was that my first approach - just changing to a later timezone, didn't work.  Since the times are all stored in microseconds-since-epoch, they are timezone-corrected, so jumping forward 4 timezones still had me deleting everything when I chose 1 hour, since the visits were "really" less than an hour ago.

You're probably not doing this, but if you were, that would explain the behaviour.  If you're not doing this though, then we need to get to the bottom of it here, because the latest patch should have fixed things, I thought.

The only other thing that springs to mind is that we're not on the same code.  Assuming your nightly really does have the fix, I'm pulling a fresh trunk and rebuilding to confirm that I am also up to date.
(In reply to comment #39)
> 1) Created a new profile
> 2) Copied over places.sqlite and friends from my normal profile, since it's
> full of long-term and recent visit information
> 3) Started up with new profile
> 4) Checked the library to make sure that all the visits show up, they do
> 5) Delete the last hour - confirm that only the last hour is deleted
> 6) Ditto last two hours, last 4, and today
> ...
> The only other thing that springs to mind is that we're not on the same code. 
> Assuming your nightly really does have the fix, I'm pulling a fresh trunk and
> rebuilding to confirm that I am also up to date.

Using the latest vanilla pull from trunk, and the above STR, I get the correct behaviour.  So let's get to the bottom of why you don't.  Might be worth spinning into its own bug at this point (and I feel like the crash is a wholly other question, though maybe the two are related).

Any of the other lurkers on this bug have experience they can share one way or the other?
OK, now I'm more confused than ever. 

I have been testing with any of several profiles I keep around just for testing and most have limited history, but only a few days worth in some cases only yesterday and today.  I am not doing a 'time-tricks' just using the browsers normally. 

I just tried copying over my regular places.sqlite to a test profile.  Checked the history was there in both the sidebar and in the Library.  

Looked in the Library and added the columns to display date/time.  All was there as expected.  I then opened a new tab and verified that the history was there by typing in a site I frequent and there it appeared OK. 

I then clicked Tools->Clear Recent History
Chose to clear the last 2 hrs
All history again was gone.
Sidebar - empty
Library - empty
Locationbar - favorite sites that displayed OK above are gone. 

Now the weird part... The sqlite file on disk did not change size, its the same size.  If my history was indeed being cleared as it appears, then why didn't the file on disk go away or at least change size significantly ?  

I'm trying to look with sqlitebrowser - but I have to close Minefield I guess to look.  A quick look (thought I'm not totally sure of how to use the tool) at the TestProfile places.sqlite file appears to not have anything left even though the filesize on disk is the same as the good profile I copied from.

Will post hack shortly.
Indeed, in the good profile when looking at moz.historyvisits I see a table of visits.  Looking at the testprofile same area after using Clear Recent Histroy, last 2 hrs, the table is empty.  

Totally at a loss here.
SQLite isn't going to make the file go smaller, but it will reuse that space when you add more entries.
I just downloaded the zip windows build from latest-mozilla-central. According to about:buildconfig it was built from changeset http://hg.mozilla.org/mozilla-central/rev/17747605490c and the build id is 20081108033239

I created a new profile & copied over my places.sqlite from my normal profile with 90 days history.
Went to clear recent history and choose last hour and can confirm all history was deleted. Nothing in library,sidebar etc. Hope this is more useful that just a me too comment.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081108 Minefield/3.1b2pre ID:20081108033239

Johnathan I'm seeing the same as Jim and Luke.. I make a new profile, I copy my places.sqlite into the new profile, start firefox, Clear Recent History, choose to clear the last hour, and after it finishes churning my history is totally empty.

I've had my places.sqlite since places first landed (I have history going back to 22nd Feb 2008, and my history is set to 360 days), so maybe there's something going on there; certainly my places.sqlite will have evolved through all landings that changed the structure of the file.
I assume you're not seeing anything in the error console?  Are we seeing a platform-specific thing?  I'm on Mac and it seems to be working, are you all on windows?  I don't know what code here would even BE platform specific...

What value does your privacy.sanitize.timeSpan pref have?
Steve and myself are on Windows.  Steve is XP, I'm Vista HP SP1 

Appears the pref is set at the default value of 0 (zero)

privacy.sanitize.timeSpan;0

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081108 Minefield/3.1b2pre Firefox/3.0 ID:20081108033239
Debugging with Jim on IRC, and confirmed that what he does in the dialog doesn't seem to alter the timespan pref.  I confirm that for me, the timspan pref DOES get set.

I'd like to get to the bottom of why that's happening, but it also suggests that explicitly applying the prefs before the sanitize() call might mitigate the problem.
Get this: none of the prefs are being applied for Jim on windows - and of course it's platform-specific, since Mac applies prefs instantly, and prefwindows know that.

The code that I replaced in attachment 346902 [details] [diff] [review] secretly had this knowledge - it's iterating through the dialog elements instead of worrying about the prefs themselves - and it wraps it in a try catch in case anything goes wrong.

Replacing it so that Sanitizer did all the work is a better choice overall, centralize the sanitize code and all that, but it means that the prefs haven't been applied yet when that code executes.  Sigh.

I'll attach a patch here that explicitly sets the prefs *before* we sanitize and throw it at tryserver.
And I even restored the error logging, for good measure.  Throwing this one at try-server so that we can get some clarity ahead of time.
Attachment #347119 - Flags: review?(mconnor)
Can't you just set instantapply=true on the relevant <preference> elements?
Gavin - that would mean that they change even if you cancel the dialog, no?  I mean, maybe not a big deal for these particular prefs, but sort of weird just the same...?
Ah, right. I think ideally what we want is to move the prefpane binding's writePreferences implementation to the <preferences> binding, in which case we could just call that in this dialog rather than duplicating the code. I don't want to scope creep this bug unnecessarily, so feel free to file a followup, but I think it should be pretty simple to do.
As in comment 47 I too am using windows (XP), and in build 20081108033239 when clearing the last hour of history the entire history was being cleared and privacy.sanitize.timeSpan was still the default 0. 

Using the windows zip try-server build I can confirm just the last hour of history is cleared and now privacy.sanitize.timeSpan is set to 1. If I set to clear the last 2 hours privacy.sanitize.timeSpan is set to 2, last 4 hours, 3, set to clear all today's history it is set to 4.
Confirming Luke's findings, the feature now is working as intended.

Good job Johnathan
Thanks for the help, guys.  I'll try to get this landed today if I can find mconnor or gavin for a review.
(In reply to comment #54)
> Ah, right. I think ideally what we want is to move the prefpane binding's
> writePreferences implementation to the <preferences> binding, in which case we
> could just call that in this dialog rather than duplicating the code. I don't
> want to scope creep this bug unnecessarily, so feel free to file a followup,
> but I think it should be pretty simple to do.

Filed bug 464015
Attachment #347119 - Flags: review?(mconnor) → review+
Comment on attachment 347119 [details] [diff] [review]
Explicitly set prefs before invoking sanitize()

>diff --git a/browser/base/content/sanitize.xul b/browser/base/content/sanitize.xul

>+        updatePrefs : function ()
>+        {
>+          var tsPref = document.getElementById("privacy.sanitize.timeSpan");
>+          Sanitizer.prefs.setIntPref("timeSpan", tsPref.value);

I was going to suggest you do:
tsPref.valueFromPreferences = tsPref.value;
here and for the other prefs as well, but then you'd want to do the same batching tricks savePreferences does to avoid flushing the file out on each set, and at that point we might as well just fix bug 464015. Using Sanitizer.prefs is a bit confusing since it's not obvious that it's just setting the same pref's value, but I guess I can live with this until bug 464015 is fixed.

>+          var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefService)
>+                            .getBranch(null);
>+          var sanitizePreferences = document.getElementById("sanitizePreferences");

You can use sanitizePreferences.rootBranch and avoid getting the pref branch manually.
Resolving FIXED for the last time?

http://hg.mozilla.org/mozilla-central/rev/bcbde22c762f
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
I think this might've caused oranges on moz2-darwin8-slave01

Log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1226353266.1226361378.8939.gz

The failures are;
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | Pretend visit to today.com should now be deleted
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_sanitize-timespans.js | 'Today' download should now be deleted
I backed this out (as changeset d18a673eb7fe ) due the oranges described above.
  --> Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Well now I'm confused.  I obviously ran these tests locally before checking in, and they ran clean for me.  They also ran clean for at least one cycle on all of the other boxes, and I think they've been running clean since they landed - it's just this latest landing that seemed to trigger something, but I don't see how that would be. 

In FACT, this latest patch doesn't even touch code that's involved in the test.  At all.  So this is kind of crazy.  The box that barfed has been misbehaving today, but I don't really know how this test fail could be misbehavior either.

I'm a touch baffled.  I checked with sdwilsh to make sure that the history deletion calls were synchronous, that there wasn't any risk of a race condition, and he confirms they're synchronous...

Um.
Whiteboard: [patch done, tests causing orange]
Talked it through with bsmedberg, who has been managing the tree today.  Given that the tests passed on all boxes except the one mac box he considers sufficiently insane at the moment to have taken it off of tinderbox, but *more importantly*, given that the patch that was backed out does not touch the tests in question, this is re-queued for landing.

Marking checkin-needed in case some kind soul is around when bsmedberg calls my name from the holy scroll, and I'm not.  Hopefully though, I'll be there to answer the call myself.
Keywords: checkin-needed
Re-re-(re-)landed.  Every active box tests green on the sanitize tests (on all browser-chrome tests).  If those tests fail again, it is not this bug, please file a new one.  :)

http://hg.mozilla.org/mozilla-central/rev/5865da85d4dd
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [patch done, tests causing orange]
Depends on: 465952
Keywords: verified1.9.1
Keywords: fixed1.9.1
Keywords: verified1.9.1
sorry for the inconvenience.  i meant to detect any bugs before the branch merge that were still RESO fixed, but had a verified1.9.1 keyword next to them.
Keywords: verified1.9.1
also has been verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090113 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
No longer depends on: 465995
You need to log in before you can comment on or make changes to this bug.