Closed
Bug 723353
Opened 13 years ago
Closed 13 years ago
Add chrome window API for per-window private browsing
Categories
(Firefox :: Private Browsing, defect)
Tracking
()
RESOLVED
FIXED
Firefox 14
People
(Reporter: jdm, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 2 obsolete files)
5.31 KB,
patch
|
jdm
:
review+
Gavin
:
feedback+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Reporter | ||
Comment 2•13 years ago
|
||
Reporter | ||
Updated•13 years ago
|
Attachment #599915 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #599927 -
Flags: review?
Comment 3•13 years ago
|
||
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•13 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.
Comment 5•13 years ago
|
||
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•13 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.
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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•13 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.
Comment 10•13 years ago
|
||
(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•13 years ago
|
Component: General → Private Browsing
QA Contact: general → private.browsing
Assignee | ||
Comment 11•13 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•13 years ago
|
||
Assignee: nobody → ehsan
Attachment #599927 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #599927 -
Flags: review?
Attachment #608564 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-try]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-try] → [autoland-try:-b do -p all -u all -t none]
Updated•13 years ago
|
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
Comment 13•13 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•13 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•13 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•13 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•13 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Comment 17•13 years ago
|
||
Try was happy with my patch!
Comment 18•13 years ago
|
||
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•13 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)
Comment 20•13 years ago
|
||
(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•13 years ago
|
||
Testing the underlying docshell flag.
Reporter | ||
Comment 22•13 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•13 years ago
|
||
Flags: in-testsuite+
Target Milestone: --- → Firefox 14
Comment 24•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 25•12 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.
Description
•