Closed Bug 817477 Opened 7 years ago Closed 7 years ago

Remove support for global private browsing mode

Categories

(Firefox :: Private Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21
Tracking Status
relnote-firefox --- -

People

(Reporter: Ehsan, Assigned: Ehsan)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Once we ship per-window private browsing support, we need to remove the code exclusively used for global PB mode.  This bug tracks this project.

This basically means removing anything hidden behind #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING (including removing that flag too.)

I'll be working on this on my globalpbdeath github branch.  You can follow the progress here: <https://github.com/ehsan/mozilla-central/compare/globalpbdeath>.  Once we're ready to land this, I'll attach a folded patch.
Assignee: nobody → ehsan
sec-review?:I'm concerned that since we are still not multi-process that we need to be concerned that data from PB could leak across when we have one PB window and one standard window. Just want to ensure we have proper eyes looking at this.
Flags: sec-review?
removing flag after discussion with dveditz
Flags: sec-review?
FWIW this bug is just about removing code that is not even getting built.
Can't wait to remove a bunch of code from Panorama :)
(In reply to comment #4)
> Can't wait to remove a bunch of code from Panorama :)

I'm planning to post a patch maybe in a couple of weeks.  I'm basically waiting to make sure that there aren't any huge blockers that prevent us from shipping this in 20.
Depends on: 806723
Depends on: 806743
Depends on: 806737
Blocks: 676989
Attached patch Patch (v1) (obsolete) — Splinter Review
Brace yourselves, this is not a small patch!

Most of this is just code removal.  Please take a bit extra care on the places that I have rewritten existing code to do proper indentation, or to simplify it.  I'm currently doing the last set of try run on this patch and I think it should be fully green.

https://tbpl.mozilla.org/?tree=Try&rev=6d99ad0d2a21
Attachment #706922 - Flags: review?(josh)
Attached patch Patch (v2)Splinter Review
Fixed a small mistake which caused a chrome test to be run as a mochitest-plain.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=3fc71fd1ecf9
Attachment #706922 - Attachment is obsolete: true
Attachment #706922 - Flags: review?(josh)
Attachment #706949 - Flags: review?(josh)
Comment on attachment 706949 [details] [diff] [review]
Patch (v2)

Mike, can you please go over the build system bits here?  I don't have anything specific for you to look at but I mentioned this to gps today and he asked me to get a rubber-stamp from you.
Attachment #706949 - Flags: review?(mh+mozilla)
Comment on attachment 706949 [details] [diff] [review]
Patch (v2)

Review of attachment 706949 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

::: browser/components/privatebrowsing/test/browser/global/Makefile.in
@@ -46,5 @@
> -		title.sjs \
> -		$(NULL)
> -
> -# Turn off private browsing tests that perma-timeout on Linux.
> -ifneq (Linux,$(OS_ARCH))

Nit: OS_TARGET

::: browser/components/privatebrowsing/test/browser/obsolete/Makefile.in
@@ -28,5 @@
> -		browser_privatebrowsing_viewsource.js \
> -		$(NULL)
> -
> -# Turn off private browsing tests that perma-timeout on Linux.
> -ifneq (Linux,$(OS_ARCH))

Likewise
Attachment #706949 - Flags: review?(mh+mozilla) → review+
Please remove now-unused strings such as privateBrowsingYesTitle from the properties and dtd files.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(In reply to Mike Hommey [:glandium] from comment #9)
> Comment on attachment 706949 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 706949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me
> 
> ::: browser/components/privatebrowsing/test/browser/global/Makefile.in
> @@ -46,5 @@
> > -		title.sjs \
> > -		$(NULL)
> > -
> > -# Turn off private browsing tests that perma-timeout on Linux.
> > -ifneq (Linux,$(OS_ARCH))
> 
> Nit: OS_TARGET

I'm removing this file.  Do you want me to address this before removing the file?  ;-)

> ::: browser/components/privatebrowsing/test/browser/obsolete/Makefile.in
> @@ -28,5 @@
> > -		browser_privatebrowsing_viewsource.js \
> > -		$(NULL)
> > -
> > -# Turn off private browsing tests that perma-timeout on Linux.
> > -ifneq (Linux,$(OS_ARCH))
> 
> Likewise

Ditto!
Erf, as I was looking at the files one by one, I hadn't realized they were file removals.
(In reply to Dão Gottwald [:dao] from comment #10)
> Please remove now-unused strings such as privateBrowsingYesTitle from the
> properties and dtd files.

Thanks for the reminder.  I thought I had caught them all, but I found some more and removed them:

diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties
index 638c91b..0d37768 100644
--- a/browser/locales/en-US/chrome/browser/browser.properties
+++ b/browser/locales/en-US/chrome/browser/browser.properties
@@ -282,18 +282,6 @@ safebrowsing.reportedAttackSite=Reported Attack Site!
 safebrowsing.notAnAttackButton.label=This isn't an attack site…
 safebrowsing.notAnAttackButton.accessKey=A
 
-# Private Browsing Confirmation dialog
-# LOCALIZATION NOTE (privateBrowsingMessage): %S will be replaced
-# by the name of the application.
-# LOCALIZATION NOTE (privateBrowsingYesTitle, privateBrowsingNoTitle, privateBrowsingNeverAsk):
-# Access keys are specified by prefixing the desired letter with an ampersand.
-privateBrowsingDialogTitle=Start Private Browsing
-privateBrowsingMessageHeader=Would you like to start Private Browsing?
-privateBrowsingMessage=%S will save your current tabs for when you are done with your Private Browsing session.
-privateBrowsingYesTitle=&Start Private Browsing
-privateBrowsingNoTitle=&Cancel
-privateBrowsingNeverAsk=&Do not show this message again
-
 # Ctrl-Tab
 # LOCALIZATION NOTE (ctrlTab.showAll.label): #1 represents the number
 # of tabs in the current browser window. It will always be 2 at least.
diff --git a/browser/locales/en-US/chrome/browser/taskbar.properties b/browser/locales/en-US/chrome/browser/taskbar.properties
index 6d50e96..987d5cc 100644
--- a/browser/locales/en-US/chrome/browser/taskbar.properties
+++ b/browser/locales/en-US/chrome/browser/taskbar.properties
@@ -6,10 +6,6 @@ taskbar.tasks.newTab.label=Open new tab
 taskbar.tasks.newTab.description=Open a new browser tab.
 taskbar.tasks.newWindow.label=Open new window
 taskbar.tasks.newWindow.description=Open a new browser window.
-taskbar.tasks.enterPrivacyMode.label=Enter private browsing
-taskbar.tasks.enterPrivacyMode.description=Start private browsing. The current session will be saved.
-taskbar.tasks.exitPrivacyMode.label=Quit private browsing
-taskbar.tasks.exitPrivacyMode.description=Quit private browsing and restore the previous session.
 taskbar.tasks.newPrivateWindow.label=New private window
 taskbar.tasks.newPrivateWindow.description=Open a new window in private browsing mode.
 taskbar.frequent.label=Frequent
Comment on attachment 706949 [details] [diff] [review]
Patch (v2)

Review of attachment 706949 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/Makefile.in
@@ +41,5 @@
>  		gZipOfflineChild.html^headers^ \
>  		gZipOfflineChild.cacheManifest \
>  		gZipOfflineChild.cacheManifest^headers^ \
>  		test_bug787619.html \
> +		privateBrowsingMode.js \

This was in a non-gtk2 block before. Will this be a problem?

::: browser/components/sessionstore/test/Makefile.in
@@ -146,5 @@
> -else
> -MOCHITEST_BROWSER_FILES += \
> -	browser_248970_a.js \
> -	browser_248970_b.js \
> -	browser_248970_b_perwindowpb.js \

This appears to be in the wrong list. I'm not sure why it hasn't been breaking Birch, though.

::: dom/tests/browser/Makefile.in
@@ +27,4 @@
>  
>  ifeq (Linux,$(OS_ARCH))
>  MOCHITEST_BROWSER_FILES += \
> +  browser_ConsoleStoragePBTest_perwindowpb.js \

This looks like it should be in the list above, not just the linux one.

::: toolkit/components/passwordmgr/test/test_prompt.html
@@ -1119,5 @@
> -proxyAuthinfo.password = "";
> -proxyAuthinfo.realm    = "Proxy Realm";
> -proxyAuthinfo.flags    = Ci.nsIAuthInformation.AUTH_PROXY;
> -
> -if (pb) {

We shouldn't get rid of this whole test. Just get rid of this block.

::: toolkit/components/social/MozSocialAPI.jsm
@@ -44,5 @@
>    try {
>      let window = doc.defaultView;
> -    if (!window
> -#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING
> -        || !PrivateBrowsingUtils.isWindowPrivate(window)

I presume this is being dealt with in the open bugs about the social API?
Attachment #706949 - Flags: review?(josh) → review+
(In reply to Josh Matthews [:jdm] from comment #14)
> Comment on attachment 706949 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 706949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/Makefile.in
> @@ +41,5 @@
> >  		gZipOfflineChild.html^headers^ \
> >  		gZipOfflineChild.cacheManifest \
> >  		gZipOfflineChild.cacheManifest^headers^ \
> >  		test_bug787619.html \
> > +		privateBrowsingMode.js \
> 
> This was in a non-gtk2 block before. Will this be a problem?

No, but I'll move it for hygiene reasons.  Thanks for catching it!

> ::: browser/components/sessionstore/test/Makefile.in
> @@ -146,5 @@
> > -else
> > -MOCHITEST_BROWSER_FILES += \
> > -	browser_248970_a.js \
> > -	browser_248970_b.js \
> > -	browser_248970_b_perwindowpb.js \
> 
> This appears to be in the wrong list. I'm not sure why it hasn't been
> breaking Birch, though.

Hmm, yeah...  I'll correct this.

> ::: dom/tests/browser/Makefile.in
> @@ +27,4 @@
> >  
> >  ifeq (Linux,$(OS_ARCH))
> >  MOCHITEST_BROWSER_FILES += \
> > +  browser_ConsoleStoragePBTest_perwindowpb.js \
> 
> This looks like it should be in the list above, not just the linux one.

Indeed.

> ::: toolkit/components/passwordmgr/test/test_prompt.html
> @@ -1119,5 @@
> > -proxyAuthinfo.password = "";
> > -proxyAuthinfo.realm    = "Proxy Realm";
> > -proxyAuthinfo.flags    = Ci.nsIAuthInformation.AUTH_PROXY;
> > -
> > -if (pb) {
> 
> We shouldn't get rid of this whole test. Just get rid of this block.

This test has effectively been removed for a while...  But you're right, we should just remove the global PB parts.

> ::: toolkit/components/social/MozSocialAPI.jsm
> @@ -44,5 @@
> >    try {
> >      let window = doc.defaultView;
> > -    if (!window
> > -#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING
> > -        || !PrivateBrowsingUtils.isWindowPrivate(window)
> 
> I presume this is being dealt with in the open bugs about the social API?

Yes, that has been fixed in bug 808215.
Unfortunately we can't enable test_prompt.html because of bug 806737.
Sorry, that was supposed to be bug 835904.
https://hg.mozilla.org/mozilla-central/rev/6aaf13ffc716
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
We're noting PWPB in the FF20 notes, the removal of global PB is less user facing so no need to note.
(In reply to Alex Keybl [:akeybl] from comment #19)
> We're noting PWPB in the FF20 notes, the removal of global PB is less user
> facing so no need to note.

Agreed.
You need to log in before you can comment on or make changes to this bug.