480260 - disable Clear Recent History when in Private Browsing mode

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Mike Connor [:mconnor]]

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.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: disable CRH in onEnterPrivateBrowsing, enable in onExitPrivateBrowsing

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!

Comment 2

[(no longer active)]

Attachment: Patch + test

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?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

(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.

Comment 4

[(no longer active)]

(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?.

Comment 5

[(no longer active)]

Attachment: Patch + test

Updated the patch based on the fix in bug 473860.

Comment 6

[Mike Connor [: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)

Comment 7

[Mike Connor [: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

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1e0d8ccb8abf

Comment 9

[Serge Gautherie (:sgautherie)]

(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... :-/

Comment 10

[Serge Gautherie (:sgautherie)]

(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 ;-<

Comment 11

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/52f1c949d697

Comment 12

[Marcia Knous [:marcia]]

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.

Comment 13

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7545 added to Litmus.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marcia, why the following test wasn't updated?
https://litmus.mozilla.org/show_test.cgi?id=7412