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)
Tracking
()
RESOLVED
FIXED
Firefox 3.6a1
People
(Reporter: dao, Assigned: dao)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 3 obsolete files)
19.49 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•15 years ago
|
||
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
Comment 2•15 years ago
|
||
This is probably more fallout from bug 431286, see the comments towards the bottom...
Comment 3•15 years ago
|
||
See also bug 465995.
Assignee | ||
Comment 4•15 years ago
|
||
(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.
Comment 5•15 years ago
|
||
(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.
Assignee | ||
Comment 6•15 years ago
|
||
(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.
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
(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.
Comment 9•15 years ago
|
||
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
Comment 10•15 years ago
|
||
(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?
Assignee | ||
Comment 11•15 years ago
|
||
(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.
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #374516 -
Flags: review?(mconnor)
Comment 13•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Attachment #374516 -
Flags: review?(mconnor) → review-
Assignee | ||
Comment 14•15 years ago
|
||
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).
Assignee | ||
Comment 15•15 years ago
|
||
yeah, comment 13 == comment 14
Comment 16•15 years ago
|
||
(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?
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 18•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Flags: blocking-firefox3.5?
Updated•15 years ago
|
Flags: blocking-firefox3.5? → blocking-firefox3.5+
Updated•15 years ago
|
Attachment #374519 -
Flags: review?(mconnor) → review?(johnath)
Updated•15 years ago
|
Assignee: nobody → dao
Updated•15 years ago
|
Attachment #374519 -
Flags: review?(johnath) → review+
Comment 19•15 years ago
|
||
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.
Updated•15 years ago
|
Whiteboard: [needs tests & landing]
Comment 20•15 years ago
|
||
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.
Comment 21•15 years ago
|
||
(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. :)
Updated•15 years ago
|
Whiteboard: [needs tests & landing] → [has patch][needs to update tests]
Comment 22•15 years ago
|
||
Dão, are you planning on updating the test or do you want me to?
Assignee | ||
Comment 23•15 years ago
|
||
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.
Assignee | ||
Comment 24•15 years ago
|
||
this shouldn't (and doesn't, for better or worse) affect the test results
Attachment #374519 -
Attachment is obsolete: true
Comment 25•15 years ago
|
||
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
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch][needs to update tests]
Assignee | ||
Comment 26•15 years ago
|
||
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
Assignee | ||
Comment 27•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c29fd3dafbe2
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs 191 landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•