This should be an easy fix. The patch for bug 744743 uses gPrivateBrowsingUI.privateBrowsingEnabled; we want to change it to gPrivateBrowsing.privateWindow and ensure that the functionality remains the same. ttaubert should be able to present a series of steps to do so, but ideally we would actually have an automated test to verify this behaviour.

I suggest we change it when the per-window PB is probably implemented. I had a quick look and although gPrivateBrowsingUI.privateWindow returns the right value. Here are the steps 1) Go into PB mode, open a new tab and I can see that gPrivateBrowsingUI.privateWindow is the same as gPrivateBrowsingUI.privateBrowsingEnabled (I used dump statement in Thumbnails_shouldCapture() to check both values) 2) Open a new window, open a new tab, and gPrivateBrowsingUI.privateWindow = true and gPrivateBrowsingUI.privateBrowsingEnabled = false. It works fine in 2) but the per-window PB UI is not implemented so it might be better to wait and land it together with UI for per-window PB patch (bug 729865).

All code that relies on per-window PB should work without any modification with the existing private browsing mode. This means that we're able to land the per-window work as it occurs, instead of in one big commit that introduces the per-window mode. I would prefer to land this sooner rather than later for this reason.

(In reply to Raymond Lee [:raymondlee] from comment #1) > 2) Open a new window, open a new tab, and gPrivateBrowsingUI.privateWindow = > true and gPrivateBrowsingUI.privateBrowsingEnabled = false. Should be gPrivateBrowsingUI.privateWindow = * false * and gPrivateBrowsingUI.privateBrowsingEnabled = * true * (In reply to Josh Matthews [:jdm] from comment #2) > All code that relies on per-window PB should work without any modification > with the existing private browsing mode. This means that we're able to land > the per-window work as it occurs, instead of in one big commit that > introduces the per-window mode. I would prefer to land this sooner rather > than later for this reason. If capturing thumbnails in step 2) in comment 1 is not a concern, I think that's fine.

Yes, it is possible to override the global private browsing mode on a per-docshell basis; that is legal behaviour that we're not concerned about at this time.

(In reply to Josh Matthews [:jdm] from comment #4) > Yes, it is possible to override the global private browsing mode on a > per-docshell basis; that is legal behaviour that we're not concerned about > at this time. I'm not sure what this means. If I understand Raymond correctly, he didn't override anything, he just opened a new window in global private browsing mode and gPrivateBrowsingUI.privateWindow lied to him.

Comment on attachment 618558 [details] [diff] [review] v1 Review of attachment 618558 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me assuming the per-window private browsing mode works as expected.

(In reply to Dão Gottwald [:dao] from comment #5) > (In reply to Josh Matthews [:jdm] from comment #4) > > Yes, it is possible to override the global private browsing mode on a > > per-docshell basis; that is legal behaviour that we're not concerned about > > at this time. > > I'm not sure what this means. If I understand Raymond correctly, he didn't > override anything, he just opened a new window in global private browsing > mode and gPrivateBrowsingUI.privateWindow lied to him. If that's correct, that is bad. I've filed bug 749187 to investigate any potential problems here; I just read Raymond's use of = as assignment, not equality.

(In reply to Josh Matthews [:jdm] from comment #7) > If that's correct, that is bad. I've filed bug 749187 to investigate any > potential problems here; I just read Raymond's use of = as assignment, not > equality. Sorry, let me clarify. If I go into private browsing mode, the existing window would have gPrivateBrowsingUI.privateWindow == true and gPrivateBrowsingUI.privateBrowsingEnabled == true. If I open another window, the new window would gPrivateBrowsingUI.privateWindow == false and gPrivateBrowsingUI.privateBrowsingEnabled == true.

Oh, right. We don't have any code which would set the private docshell flag on newly opened windows. :( Let's take this to bug 749187.

Raymond, I have a patch in bug 749187. Please use that if you need to develop on top of it.

(In reply to Ehsan Akhgari [:ehsan] from comment #10) > Raymond, I have a patch in bug 749187. Please use that if you need to > develop on top of it. Thanks Ehsan! When a new window is opened in PB mode, the gPrivateBrowsingUI.privateWindow is the same as gPrivateBrowsingUI.privateBrowsingEnabled now.

We should land this patch after bug 749187 has landed.

Comment on attachment 618558 [details] [diff] [review] v1 Review of attachment 618558 [details] [diff] [review]: ----------------------------------------------------------------- r=me.