Closed Bug 723353 Opened 11 years ago Closed 11 years ago
Add chrome window API for per-window private browsing
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.
Attachment #599915 - Attachment is obsolete: true
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).
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.
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.
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.
Component: General → Private Browsing
QA Contact: general → private.browsing
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...
Whiteboard: [autoland-try] → [autoland-try:-b do -p all -u all -t none]
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
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 on attachment 608564 [details] [diff] [review] Patch (v1) >+ * For now the getter returns nsIPrivateBrowsingService.privateBrowsingEnabled, This doesn't appear to be true.
(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. :-)
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://firstname.lastname@example.org
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+
Comment on attachment 608564 [details] [diff] [review] Patch (v1) (In reply to Gavin Sharp (use email@example.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...
Testing the underlying docshell flag.
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+
Target Milestone: --- → Firefox 14
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.