Closed Bug 490030 Opened 15 years ago Closed 15 years ago

With privacy.cpd.downloads set to true and privacy.cpd.history set to false, "Browsing and Download History" is checked in the Clear Recent History dialog

Categories

(Firefox :: Private Browsing, defect)

3.5 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

With privacy.cpd.downloads set to true and privacy.cpd.history set to false (Firefox 3 provides the UI to get to this state), "Browsing and Download History" is checked in the Clear Recent History dialog. It's not clear to me what clearing will actually do now.
That's understandable, but I'm not sure what else can be done if there's only one checkbox for both.

If one is true, the combined checkbox is checked:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitizeDialog.js#242

And then on clear if one is true (and the combined checkbox is therefore checked), both are set to true and cleared, even if the other was originally false:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitizeDialog.js#258
This is probably more fallout from bug 431286, see the comments towards the bottom...
See also bug 465995.
(In reply to comment #1)
> That's understandable, but I'm not sure what else can be done if there's only
> one checkbox for both.
> 
> If one is true, the combined checkbox is checked:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitizeDialog.js#242

Check it only if both are true? Seems like that would prevent surprises & dataloss.
(In reply to comment #4)
> Check it only if both are true? Seems like that would prevent surprises &
> dataloss.

So the one that's true is cleared while the combined checkbox is not checked at all?  Isn't *that* dataloss?  The alternative, setting both to false and clearing neither if one is false, has the opposite problem of unwanted data retention.

The combined checkbox says "Browsing and Download History", so IMO it's clear that both will be... cleared.
(In reply to comment #5)
> (In reply to comment #4)
> > Check it only if both are true? Seems like that would prevent surprises &
> > dataloss.
> 
> So the one that's true is cleared while the combined checkbox is not checked at
> all?  Isn't *that* dataloss?  The alternative, setting both to false and
> clearing neither if one is false,

The alternative is what I'm suggesting. The checkbox isn't checked, and thus nothing will be cleared.

> has the opposite problem of unwanted data
> retention.

Right, but if my download history is still there surprisingly, I can fix that. If my history is gone surprisingly, it's just gone.
Suggestion: if privacy.cpd.downloads is true and privacy.cpd.history is false ignore the pref, as downloads does not imply history. However if the opposite is true, clear it all, because history does imply downloads.
(In reply to comment #7)
> Suggestion: if privacy.cpd.downloads is true and privacy.cpd.history is false
> ignore the pref, as downloads does not imply history. However if the opposite
> is true, clear it all, because history does imply downloads.

Seems fine to me. So basically just map privacy.cpd.history to "Browsing and Download History" and don't deal with privacy.cpd.downloads at all.
One other thing that *can* be done, so as not to render the downloads pref useless, is clear the download db if it is set to true, but not the history db. That's similar to what's done now, but is somewhat weird ui in this case. In any case, the "Browsing History and Downloads" checkbox definitely shouldn't be checked if the history pref is false, regardless of the downloads pref.

uiwanted, I guess.
Keywords: uiwanted
(In reply to comment #7)
There's been some very understandable confusion about what is and isn't history with this dialog refresh.  According to faaborg in bug 464204, "history" is more than browsing history, and the list of downloads *is* history.  The refresh follows that terminology.

We disagree about what to do; as I say, if we're combining the two prefs into one checkbox, I think it's fairly clear what will be cleared since the checkbox's label is "Browsing and Download History".

Maybe faaborg or mconnor could weigh in?
(In reply to comment #10)
> We disagree about what to do; as I say, if we're combining the two prefs into
> one checkbox, I think it's fairly clear what will be cleared since the
> checkbox's label is "Browsing and Download History".

The checkbox isn't even visible by default.
Attached patch ignore privacy.cpd.downloads (obsolete) — Splinter Review
Attachment #374516 - Flags: review?(mconnor)
(In reply to comment #12)
> Created an attachment (id=374516) [details]
> ignore privacy.cpd.downloads

Now I'm confused when I made sure "Browsing and Download History" is not checked but my downloads are cleared anyway.  At a different point in time I'm confused when I made sure it was checked but my downloads are not cleared.
Attachment #374516 - Flags: review?(mconnor) → review-
Comment on attachment 374516 [details] [diff] [review]
ignore privacy.cpd.downloads

I think I removed too much here... looks like that wouldn't read the downloads pref (expected), but also not clear downloads if the checkbox is checked (unexpected).
yeah, comment 13 == comment 14
(In reply to comment #1)
> That's understandable, but I'm not sure what else can be done if there's only
> one checkbox for both.
> 
> If one is true, the combined checkbox is checked:
...
(In reply to comment #13)
> (In reply to comment #12)
> Now I'm confused when I made sure "Browsing and Download History" is not
> checked but my downloads are cleared anyway.  At a different point in time I'm
> confused when I made sure it was checked but my downloads are not cleared.

I think we basically need to programmatically bind these two prefs.  So when
the dialog opens, if they have different settings, we turn them both off,
uncheck the box and worst case, our user furrows their brow or fails to delete
some data, but there's no surprising downloads dataloss.  After that point,
checking or unchecking the box toggles both prefs, and the sanitizer can go on
checking them both and imagining that they are independent (and maybe some day
they will be!)

If you are worried that users won't discover that downloads have been turned
off because they had history off, maybe auto-expand details in that edge case, but honestly I don't know if the "accidental failure to delete download history" really rises to the level necessary to merit it.

Stupid idea?
See comment 7, specifically the reason I think if privacy.cpd.history is checked that it should be respected regardless of privacy.cpd.downloads, is because that is exactly how 3.0.x behaves. If history is selected, history will always be deleted regardless of the downloads pref. if history was unchecked, downloads was accessible, something that can't be expressed in this new dialog.
Comment 16 seems to be equal to my first suggestion, but I still think comment 7 is preferable.
Attachment #374516 - Attachment is obsolete: true
Attachment #374519 - Flags: review?(mconnor)
Blocks: 481429
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5? → blocking-firefox3.5+
Attachment #374519 - Flags: review?(mconnor) → review?(johnath)
Assignee: nobody → dao
Attachment #374519 - Flags: review?(johnath) → review+
Comment on attachment 374519 [details] [diff] [review]
make privacy.cpd.history rule privacy.cpd.downloads

>-      <preference id="privacy.cpd.downloads"             name="privacy.cpd.downloads"             type="bool"/>
>+      <preference id="privacy.cpd.downloads"             name="privacy.cpd.downloads" disabled="true" type="bool"/>

Nit: I think personally I'd prefer disabled="true" at the end so that the formatting is preserved, but the disabled stands out clearly on the end.

>-    // First set the values of the separate history and downloads pref
>-    // elements based on the combined history-downloads checkbox.
>-    var combinedCbChecked =
>-      document.getElementById("history-downloads-checkbox").checked;
>-    var historyPref = document.getElementById("privacy.cpd.history");
>-    historyPref.value = !historyPref.disabled && combinedCbChecked;
>-    var downloadsPref = document.getElementById("privacy.cpd.downloads");
>-    downloadsPref.value = !downloadsPref.disabled && combinedCbChecked;
>+    // Keep the pref for the download history in sync with the history pref.
>+    document.getElementById("privacy.cpd.downloads").value =
>+      document.getElementById("privacy.cpd.history").value;

Are we covered by existing tests here to ensure that things behave correctly? Using behind-the-scenes pref-manipulation feels like the kind of thing we could surprise ourselves with in the future. It would be nice to see a test that adds some history and downloads, inits the prefs in various configurations, checks/unchecks the checkbox and watches for correct behaviour. The changes to pref names being discussed in bug 488706 are an example of the kind of thing I could see breaking us here, if we forgot to update this pref-syncing code.

r=me with a test that checks the possibilities, or with a pointer to tests that already cover this.
Whiteboard: [needs tests & landing]
browser/base/content/test/browser_sanitizeDialog.js is the test for the new CRH UI.  It specifically tests the combined history-downloads checkbox, and in fact with the patch here the test fails.  I don't think it should be too hard to update it to test the new semantics here.
(In reply to comment #20)
> browser/base/content/test/browser_sanitizeDialog.js is the test for the new CRH
> UI.  It specifically tests the combined history-downloads checkbox, and in fact
> with the patch here the test fails.  I don't think it should be too hard to
> update it to test the new semantics here.

In that case, r=me with appropriate test changes. :)
Whiteboard: [needs tests & landing] → [has patch][needs to update tests]
Dão, are you planning on updating the test or do you want me to?
I don't see why the test fails. My naive understanding of the code tells me it should pass. If you know what's going on, please go ahead, otherwise I'll dig a little deeper.
Attached patch patch + tests clean-up (obsolete) — Splinter Review
this shouldn't (and doesn't, for better or worse) affect the test results
Attachment #374519 - Attachment is obsolete: true
Aha, this is subtle.  Setting checkbox.checked = true doesn't cause the onsyncfrompreference event to fire, which implies that it doesn't keep the pref synced up.  This isn't a problem for the current code and test because the history-downloads checkbox doesn't map directly to one pref like it does with this patch; instead the separate history and downloads prefs are set manually in updatePrefs() based on checkbox.checked.

When I stop the test and click the checkbox the sync event is fired and the test passes.  So this new patch here changes the test so that it click()s the checkbox until it's in the desired state.

Since Johnathan preemptively r+'ed in comment 21, adding checkin-needed to this bug.
Attachment #377109 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [has patch][needs to update tests]
Thanks, Drew. click()ing twice shouldn't be necessary -- the prefs need to be in sync if the user doesn't click at all.

http://hg.mozilla.org/mozilla-central/rev/b07d2d603e4a
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Target Milestone: --- → Firefox 3.6a1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: