The default bug view has changed. See this FAQ.

Add chrome window API for per-window private browsing

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Private Browsing
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 14
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 599915 [details] [diff] [review]
Add chrome window API to manipulate per-window private browsing mode.
(Reporter)

Updated

5 years ago
Blocks: 729865
(Reporter)

Updated

5 years ago
Blocks: 729867
(Reporter)

Comment 2

5 years ago
Created attachment 599927 [details] [diff] [review]
Add chrome window API to manipulate per-window private browsing mode.
(Reporter)

Updated

5 years ago
Attachment #599915 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #599927 - Flags: review?
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).
(Reporter)

Comment 4

5 years ago
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.
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.
(Reporter)

Comment 6

5 years ago
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.
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?
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.
(Reporter)

Comment 9

5 years ago
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.
(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.
(Assignee)

Updated

5 years ago
Component: General → Private Browsing
QA Contact: general → private.browsing
(Assignee)

Comment 11

5 years ago
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...
(Assignee)

Comment 12

5 years ago
Created attachment 608564 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Attachment #599927 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #599927 - Flags: review?
Attachment #608564 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try]
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-try:-b do -p all -u all -t none]

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
(Assignee)

Updated

5 years ago
Blocks: 723003

Comment 13

5 years ago
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
(Reporter)

Comment 14

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

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

This doesn't appear to be true.
(Assignee)

Comment 15

5 years ago
(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

5 years ago
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

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Assignee)

Comment 17

5 years ago
Try was happy with my patch!
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.
Attachment #608564 - Flags: review?(gavin.sharp) → feedback+
(Assignee)

Comment 19

5 years ago
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.)
Attachment #608564 - Flags: review?(josh)
(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...
(Assignee)

Comment 21

5 years ago
Testing the underlying docshell flag.
(Reporter)

Comment 22

5 years ago
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.
Attachment #608564 - Flags: review?(josh) → review+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/36586538ef3b
Flags: in-testsuite+
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/36586538ef3b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Blocks: 722853
(Reporter)

Updated

5 years ago
Blocks: 748604

Comment 25

5 years ago
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
You need to log in before you can comment on or make changes to this bug.