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.

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.

removing flag after discussion with dveditz

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.

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

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

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.

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

Please remove now-unused strings such as privateBrowsingYesTitle from the properties and dtd files.

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

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

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.