Last Comment Bug 723353 - Add chrome window API for per-window private browsing
: Add chrome window API for per-window private browsing
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: Firefox 14
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on: 722840
Blocks: sdk-pwpb PBnGen 722853 723003 729865 729867
  Show dependency treegraph
 
Reported: 2012-02-01 16:43 PST by Josh Matthews [:jdm] (away until 9/3)
Modified: 2012-08-28 09:55 PDT (History)
7 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add chrome window API to manipulate per-window private browsing mode. (276 bytes, patch)
2012-02-23 01:11 PST, Josh Matthews [:jdm] (away until 9/3)
no flags Details | Diff | Splinter Review
Add chrome window API to manipulate per-window private browsing mode. (1.93 KB, patch)
2012-02-23 02:21 PST, Josh Matthews [:jdm] (away until 9/3)
no flags Details | Diff | Splinter Review
Patch (v1) (5.31 KB, patch)
2012-03-22 19:04 PDT, :Ehsan Akhgari
josh: review+
gavin.sharp: feedback+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (away until 9/3) 2012-02-01 16:43:52 PST
Ehsan suggested browser.xul and tabbrowser.xml as good places for it to reside. We'll want a high-level method for chrome windows that determines if any contained tabs contain docshells that are in PB mode, and a method to toggle all contained tabs' docshells' PB mode.
Comment 1 Josh Matthews [:jdm] (away until 9/3) 2012-02-23 01:11:02 PST
Created attachment 599915 [details] [diff] [review]
Add chrome window API to manipulate per-window private browsing mode.
Comment 2 Josh Matthews [:jdm] (away until 9/3) 2012-02-23 02:21:42 PST
Created attachment 599927 [details] [diff] [review]
Add chrome window API to manipulate per-window private browsing mode.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-23 10:32:22 PST
It seems a little odd that privateMode getter returns true if only one of the child browsers is in privateMode. I would kind of expect it to be all-or-nothing.

A .privateMode getter in browser.xml would simplify this patch a little (and would be useful in its own right).
Comment 4 Josh Matthews [:jdm] (away until 9/3) 2012-02-23 10:57:28 PST
While there are no plans to provide a UI for per-tab private mode, we want to support it architecturally. If we're providing a simple mechanism to determine whether a window is "private" or not, I don't think all or nothing is good enough.
Comment 5 :Felipe Gomes (needinfo me!) 2012-02-23 13:36:31 PST
Does the setter recursively set usePrivateBrowsing for all the child docshells?

I find it a little unsettling when a getter/setter have different behavior, it feels it's asking to being used incorrectly. I believe that since there won't be UI for per-tab PB, anyPrivateBrowsing should be the tabbrowser's public function that you'll want people to use as that will make its meaning explicit. Then setAllPB() should also be a public function and you can get rid of the property.

And if you want you could implement per-browser getter/setter in browser.xml, to expose per-browser functionality for add-ons/future.
Comment 6 Josh Matthews [:jdm] (away until 9/3) 2012-02-23 13:47:31 PST
Yes, the usePrivateBrowsing attribute is inherited by all child docshells. I understand your concerns, and I guess exposing separate methods instead of a spooky getter/setter is sensible.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-23 15:52:11 PST
If only one tab in a window is "private", then it seems misleading for that window's tabbrowser's "private" attribute to return true. Why is that ever useful behavior?
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-23 15:54:56 PST
Though if child docShells inherit from their rootTreeItem, then it seems like the tabbrowser property can just check that, and avoid iterating over its children entirely.
Comment 9 Josh Matthews [:jdm] (away until 9/3) 2012-02-23 17:04:17 PST
This API is designed for use by addons, primarily, and perhaps chrome code - I haven't dug into many of the other places where we currently check PB state. The goal is to pessimistically provide a quick way to check the privacy status of a window. If the results are mixed, it feels safer to me to call it a private window. If the relevant code cares about individual tabs instead of windows, they should just check that docshell instead.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-23 17:19:52 PST
(In reply to Josh Matthews [:jdm] from comment #9)
> If the results are mixed, it feels safer to me to call it a private window.

The opposite feels safer to me! But I have no idea what the common uses are for addons checking private browsing state... Some examples might help.
Comment 11 :Ehsan Akhgari 2012-03-22 18:30:42 PDT
Currently, when you set the usePrivateBrowsing flag on a docshell, that docshell and all of its children will be in private mode, but not any of its parents.  This provides platform level support for per-tab PB mode.

But I don't want Firefox code to provide any sort of support for per-tab PB mode.  Any add-on which wishes to do so can create their own APIs.  So I retract my suggestion about tabbrowser.xml being a good place for this API.  I think that we should put this in gPrivateBrowsingUI, living in browser.js.  That object would be accessible from the window object associated with browser.xul.

In the future, we might add a similar API to browser.xml/tabbrowser.xml if it proves to be useful as a shorthand to containingWindow.gPrivateBrowsingUI.privateWindow...
Comment 12 :Ehsan Akhgari 2012-03-22 19:04:14 PDT
Created attachment 608564 [details] [diff] [review]
Patch (v1)
Comment 13 Mozilla RelEng Bot 2012-03-22 19:07:37 PDT
Autoland Patchset:
	Patches: 608564
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=a54f5a5cbe6d
Try run started, revision a54f5a5cbe6d. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=a54f5a5cbe6d
Comment 14 Josh Matthews [:jdm] (away until 9/3) 2012-03-22 20:03:32 PDT
Comment on attachment 608564 [details] [diff] [review]
Patch (v1)

>+   * For now the getter returns nsIPrivateBrowsingService.privateBrowsingEnabled,

This doesn't appear to be true.
Comment 15 :Ehsan Akhgari 2012-03-22 20:21:00 PDT
(In reply to Josh Matthews [:jdm] from comment #14)
> Comment on attachment 608564 [details] [diff] [review]
> Patch (v1)
> 
> >+   * For now the getter returns nsIPrivateBrowsingService.privateBrowsingEnabled,
> 
> This doesn't appear to be true.

These values are the same for now.  :-)
Comment 16 Mozilla RelEng Bot 2012-03-23 03:03:37 PDT
Try run for a54f5a5cbe6d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a54f5a5cbe6d
Results (out of 222 total builds):
    exception: 1
    success: 185
    warnings: 22
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-a54f5a5cbe6d
Comment 17 :Ehsan Akhgari 2012-03-23 11:26:35 PDT
Try was happy with my patch!
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-27 12:16:40 PDT
Comment on attachment 608564 [details] [diff] [review]
Patch (v1)

If the setter isn't useful yet, why not just avoid adding it?

I don't really understand the nsPrivateBrowsingService.js change.
Comment 19 :Ehsan Akhgari 2012-03-27 14:13:02 PDT
Comment on attachment 608564 [details] [diff] [review]
Patch (v1)

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18)
> Comment on attachment 608564 [details] [diff] [review]
> Patch (v1)
> 
> If the setter isn't useful yet, why not just avoid adding it?

It is useful for testing.

> I don't really understand the nsPrivateBrowsingService.js change.

Requesting review from Josh on that.  That change makes sure that the docshell flag gets set where the keep_current_session pref is set (I caught it when I saw that my test changes are failing.)
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-27 14:15:56 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> It is useful for testing.

Testing what? You seem to only be using it in the tests for the setter...
Comment 21 :Ehsan Akhgari 2012-03-27 14:22:05 PDT
Testing the underlying docshell flag.
Comment 22 Josh Matthews [:jdm] (away until 9/3) 2012-03-27 15:28:50 PDT
Comment on attachment 608564 [details] [diff] [review]
Patch (v1)

This change is good, but please fix up the insane indentation on the |if (browserWindow)| condition up above. I spent 10 minutes looking at this code getting confused by that.
Comment 24 Ed Morley [:emorley] 2012-03-28 14:02:59 PDT
https://hg.mozilla.org/mozilla-central/rev/36586538ef3b
Comment 25 [github robot] 2012-08-28 09:55:40 PDT
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6865cdfae309f6449b054fa9ccd208330379a017
use same query path as in bug 723353

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