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)
Tracking
()
RESOLVED
FIXED
Firefox 15
People
(Reporter: jdm, Assigned: raymondlee)
References
Details
(Whiteboard: [mentor=jdm][lang=js])
Attachments
(1 file, 1 obsolete file)
1.25 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Reporter | ||
Comment 4•12 years ago
|
||
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.
Comment 5•12 years ago
|
||
(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 6•12 years ago
|
||
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+
Reporter | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Comment 9•12 years ago
|
||
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.
Comment 10•12 years ago
|
||
Raymond, I have a patch in bug 749187. Please use that if you need to develop on top of it.
Assignee | ||
Comment 11•12 years ago
|
||
(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
Assignee | ||
Comment 12•12 years ago
|
||
We should land this patch after bug 749187 has landed.
Comment 13•12 years ago
|
||
Comment on attachment 618558 [details] [diff] [review] v1 Review of attachment 618558 [details] [diff] [review]: ----------------------------------------------------------------- r=me.
Attachment #618558 -
Flags: review+
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #618558 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 16•12 years ago
|
||
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.
Description
•