Last Comment Bug 748802 - browser-thumbnails.js uses global private browsing state instead of per-window state
: browser-thumbnails.js uses global private browsing state instead of per-windo...
Status: RESOLVED FIXED
[mentor=jdm][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Firefox 15
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on: 749187
Blocks: PBnGen 744743
  Show dependency treegraph
 
Reported: 2012-04-25 08:48 PDT by Josh Matthews [:jdm]
Modified: 2012-05-18 18:13 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (982 bytes, patch)
2012-04-25 22:25 PDT, Raymond Lee [:raymondlee]
ehsan: review+
ttaubert: feedback+
Details | Diff | Splinter Review
Patch for checkin (1.25 KB, patch)
2012-05-01 18:43 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-04-25 08:48:27 PDT
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.
Comment 1 Raymond Lee [:raymondlee] 2012-04-25 22:25:55 PDT
Created attachment 618558 [details] [diff] [review]
v1

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).
Comment 2 Josh Matthews [:jdm] 2012-04-25 23:15:06 PDT
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.
Comment 3 Raymond Lee [:raymondlee] 2012-04-25 23:27:24 PDT
(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.
Comment 4 Josh Matthews [:jdm] 2012-04-25 23:53:18 PDT
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 Dão Gottwald [:dao] 2012-04-26 04:03:38 PDT
(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 Tim Taubert [:ttaubert] 2012-04-26 07:04:33 PDT
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.
Comment 7 Josh Matthews [:jdm] 2012-04-26 07:37:53 PDT
(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.
Comment 8 Raymond Lee [:raymondlee] 2012-04-26 09:01:27 PDT
(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 :Ehsan Akhgari 2012-04-26 09:14:53 PDT
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 :Ehsan Akhgari 2012-04-26 14:53:58 PDT
Raymond, I have a patch in bug 749187.  Please use that if you need to develop on top of it.
Comment 11 Raymond Lee [:raymondlee] 2012-04-26 19:29:04 PDT
(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.
Comment 12 Raymond Lee [:raymondlee] 2012-04-26 19:30:19 PDT
We should land this patch after bug 749187 has landed.
Comment 13 :Ehsan Akhgari 2012-04-30 12:54:55 PDT
Comment on attachment 618558 [details] [diff] [review]
v1

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

r=me.
Comment 14 Raymond Lee [:raymondlee] 2012-05-01 18:43:07 PDT
Created attachment 620162 [details] [diff] [review]
Patch for checkin
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-05-18 08:45:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/51db566d6138
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-05-18 18:13:18 PDT
https://hg.mozilla.org/mozilla-central/rev/51db566d6138

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