Closed Bug 480260 Opened 13 years ago Closed 13 years ago

disable Clear Recent History when in Private Browsing mode

Categories

(Firefox :: Private Browsing, defect, P2)

3.5 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: mconnor, Assigned: beltzner)

References

(Depends on 1 open bug)

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 2 obsolete files)

spun off from bug 463607

We don't have a good safe/compatibility-friendly way of fixing this for 3.1, so we're going to just disable the feature in PB mode for now.
Flags: blocking-firefox3.1+
Priority: -- → P2
This patch disables the CRH command when we enter PB mode and disables it when we exit, just as we disable/enable the Import... command.

Funny thing was that users can also call CRH from their Preferences menu. I couldn't figure out an easy way to disable/enable that button (especially since you can invoke PB mode with the PrefPane open) without attaching an observer, and I didn't want to attach an observer, so instead I did the logical thing and questioned why the heck we still have a "Clear Now..." button there when we also have it in the menu. Gavin, Mossop, Johnath and I couldn't think of a solid reason, so I just removed it (and the associated command from privacy.js).

Let's see how mconnor feels about that!
Attachment #364466 - Flags: ui-review+
Attachment #364466 - Flags: review?(mconnor)
Attached patch Patch + testSplinter Review
This is the same patch as attachment 364466 [details] [diff] [review], only with a simple test added.

BTW, Beltzner, do you want the Clear Now button in the preferences window to be removed on trunk as well?
Attachment #364466 - Attachment is obsolete: true
Attachment #364492 - Flags: review?(mconnor)
Attachment #364466 - Flags: review?(mconnor)
(In reply to comment #2)
> This is the same patch as attachment 364466 [details] [diff] [review], only with a simple test 
> added.

Thanks, Ehsan! I was trying to teach myself how to write tests, so this is helpful and informative.

> BTW, Beltzner, do you want the Clear Now button in the preferences window to be
> removed on trunk as well?

I think so. Perhaps we should just remove the "branch only" from the comments and land this fix on both trunk and branch, and then fix bug 463607 on trunk only at some other time.

The decision to remove the "Clear Now..." button has also been approved by faaborg and mconnor, so I think it's valid on both trunk and branch.
(In reply to comment #3)
> Thanks, Ehsan! I was trying to teach myself how to write tests, so this is
> helpful and informative.

No, thank *you* for picking up my slack here.  :-)

> I think so. Perhaps we should just remove the "branch only" from the comments
> and land this fix on both trunk and branch, and then fix bug 463607 on trunk
> only at some other time.

Yes, I think that would be the correct thing to do.  This way we could land this on trunk first and let it bake a little before landing it on 1.9.1, in order to stick with branch tree rules.

> The decision to remove the "Clear Now..." button has also been approved by
> faaborg and mconnor, so I think it's valid on both trunk and branch.

OK.  We need a Litmus test for that then, setting in-litmus?.
Flags: in-litmus?
Summary: disable Clear Recent History when in Private Browsing mode (branch only) → disable Clear Recent History when in Private Browsing mode
Attached patch Patch + test (obsolete) — Splinter Review
Updated the patch based on the fix in bug 473860.
Attachment #364492 - Attachment is obsolete: true
Attachment #364511 - Flags: review?(mconnor)
Attachment #364492 - Flags: review?(mconnor)
Comment on attachment 364511 [details] [diff] [review]
Patch + test

we don't need to null check every single item, only things that aren't part of the hidden window on Mac.  I think the previous patch is probably okay, will review that.  (We actually discussed the null checks when beltzner was writing the patch yesterday)
Attachment #364511 - Attachment is obsolete: true
Attachment #364511 - Flags: review?(mconnor)
Comment on attachment 364492 [details] [diff] [review]
Patch + test

r=me on this iteration of the patch with the entities removed on trunk for clearDataNow.*

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#58
Attachment #364492 - Attachment is obsolete: false
Attachment #364492 - Flags: review+
Whiteboard: [has patch][has reviews][ready to land]
http://hg.mozilla.org/mozilla-central/rev/1e0d8ccb8abf
Assignee: ehsan.akhgari → beltzner
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][ready to land]
Whiteboard: [needs 1.9.1 landing]
(In reply to comment #4)
> > I think so. Perhaps we should just remove the "branch only" from the comments
> > and land this fix on both trunk and branch, and then fix bug 463607 on trunk
> > only at some other time.
> 
> Yes, I think that would be the correct thing to do.  This way we could land
> this on trunk first and let it bake a little before landing it on 1.9.1, in
> order to stick with branch tree rules.

(In reply to comment #8)
> http://hg.mozilla.org/mozilla-central/rev/1e0d8ccb8abf

This comment did land on mozilla-central... :-/
(In reply to comment #9)
> This comment did land on mozilla-central... :-/

Oh, that got fixed by
http://hg.mozilla.org/mozilla-central/rev/7972c87a6010 ;-<
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/52f1c949d697
Flags: in-testsuite+
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090302 Shiretoko/3.1b3pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090302 Shiretoko/3.1b3pre.

Verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090302 Minefield/3.2a1pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090302 Minefield/3.2a1pre.
Status: RESOLVED → VERIFIED
https://litmus.mozilla.org/show_test.cgi?id=7545 added to Litmus.
Flags: in-litmus? → in-litmus+
Marcia, why the following test wasn't updated?
https://litmus.mozilla.org/show_test.cgi?id=7412
Target Milestone: Firefox 3.1b3 → Firefox 3.2a1
Depends on: 1163630
You need to log in before you can comment on or make changes to this bug.