Closed Bug 748802 Opened 12 years ago Closed 12 years ago

browser-thumbnails.js uses global private browsing state instead of per-window state

Categories

(Firefox :: Tabbed Browser, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 15

People

(Reporter: jdm, Assigned: raymondlee)

References

Details

(Whiteboard: [mentor=jdm][lang=js])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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).
Attachment #618558 - Flags: feedback?(ttaubert)
Depends on: 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.
Attachment #618558 - Flags: feedback?(ttaubert) → feedback+
Depends on: 749187
(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.
No longer depends on: 729865
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.
Attachment #618558 - Flags: review+
Attachment #618558 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/51db566d6138
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/51db566d6138
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: