Last Comment Bug 748477 - Switch chrome privateWindow getter to check currentTab and remove setter
: Switch chrome privateWindow getter to check currentTab and remove setter
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
: Firefox 16
Assigned To: Saurabh Anand [:sawrubh]
: :Ehsan Akhgari
Depends on:
Blocks: PBnGen
  Show dependency treegraph
Reported: 2012-04-24 11:36 PDT by Josh Matthews [:jdm]
Modified: 2012-06-08 07:49 PDT (History)
4 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (4.15 KB, patch)
2012-06-07 17:46 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v2 (4.15 KB, patch)
2012-06-07 18:42 PDT, Saurabh Anand [:sawrubh]
ehsan: review+
Details | Diff | Splinter Review

Description User image Josh Matthews [:jdm] 2012-04-24 11:36:05 PDT
Having discussed the ramifications of bug 723353 some more, Ehsan and I have decided that we should eliminate the setter and make the getter operate on the current tab instead of the entire window. Even though we are not planning to ship with a per-tab PB implementation, we don't want to deny addon authors the ability to create one. This means that our code that uses the JS API to determine PB status must not break in the presence of a per-tab implementation, hence the switch to currentTab. Since the prospect of an asymmetric API that checks the current tab but sets the whole window is unappealing, we'll remove the setter and manipulate docshells when necessary in the chrome code. This will require updating a couple tests that use this new setter.
Comment 1 User image Saurabh Anand [:sawrubh] 2012-06-07 16:39:48 PDT
Please assign this to me.
Comment 2 User image Saurabh Anand [:sawrubh] 2012-06-07 17:46:12 PDT
Created attachment 631231 [details] [diff] [review]
Patch v1
Comment 3 User image :Ehsan Akhgari 2012-06-07 18:31:53 PDT
Comment on attachment 631231 [details] [diff] [review]
Patch v1

Looks great, but you have a small mistake: you need to flip the order of arguments passed to setPrivateWindow in the test files as well.
Comment 4 User image Saurabh Anand [:sawrubh] 2012-06-07 18:42:45 PDT
Created attachment 631239 [details] [diff] [review]
Patch v2
Comment 5 User image :Ehsan Akhgari 2012-06-07 18:44:08 PDT
Comment on attachment 631239 [details] [diff] [review]
Patch v2

Looks great!  r=me
Comment 7 User image 2012-06-08 00:34:00 PDT
Comment on attachment 631239 [details] [diff] [review]
Patch v2

>+    return gBrowser.selectedTab.linkedBrowser
>+                               .docShell.QueryInterface(Ci.nsILoadContext)
>+                               .usePrivateBrowsing;
I hate to spoil your fun but gBrowser has a .docShell property...
Comment 8 User image Saurabh Anand [:sawrubh] 2012-06-08 01:00:09 PDT
@Neil But if I use |gBrowser.docShell|, wouldn't that refer to the docShell of the current window, while we need to use the use the PB mode flag per tab, for which we need to access the docShell of each tab.
Comment 9 User image Graeme McCutcheon [:graememcc] 2012-06-08 04:18:11 PDT

(Merged by Ed Morley)
Comment 10 User image :Ehsan Akhgari 2012-06-08 07:49:49 PDT
That docShell property is defined here: <> and it seems to do what we want.  Saurabh, do you want to file a follow-up bug to fix this?  :-)

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