723353 - Add chrome window API for per-window private browsing

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Josh Matthews [:jdm]]

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]]

Attachment: Add chrome window API to manipulate per-window private browsing mode.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Add chrome window API to manipulate per-window private browsing mode.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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]]

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 for replies!)]

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]]

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: [e-mail]]]

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: [e-mail]]]

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]]

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: [e-mail]]]

(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

[(no longer active)]

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

[(no longer active)]

Attachment: Patch (v1)

Comment 13

[Mozilla RelEng Bot]

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]]

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

[(no longer active)]

(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]

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/[e-mail]-a54f5a5cbe6d

Comment 17

[(no longer active)]

Try was happy with my patch!

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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

[(no longer active)]

Comment on attachment 608564 [details] [diff] [review]
Patch (v1)

(In reply to Gavin Sharp (use [e-mail] 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: [e-mail]]]

(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

[(no longer active)]

Testing the underlying docshell flag.

Comment 22

[Josh Matthews [:jdm]]

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 23

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36586538ef3b

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/36586538ef3b

Comment 25

[[github robot]]

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