Closed Bug 1192120 Opened 4 years ago Closed 4 years ago

Warnings when opening downloads in private browsing

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file, 1 obsolete file)

WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
getData@resource:///modules/DownloadsCommon.jsm:231:9
DownloadsPlacesView@chrome://browser/content/downloads/allDownloadsViewOverlay.js:533:25
init@chrome://browser/content/downloads/contentAreaDownloadsView.js:9:16
onload@about:downloads:1:1
WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
getIndicatorData@resource:///modules/DownloadsCommon.jsm:253:9
DownloadsPlacesView@chrome://browser/content/downloads/allDownloadsViewOverlay.js:538:3
init@chrome://browser/content/downloads/contentAreaDownloadsView.js:9:16
onload@about:downloads:1:1
WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
init@chrome://browser/content/downloads/contentAreaDownloadsView.js:11:10
onload@about:downloads:1:1
Attachment #8644765 - Flags: review?(mconley)
Attachment #8644765 - Attachment is obsolete: true
Attachment #8644765 - Flags: review?(mconley)
Attachment #8644773 - Flags: review?(mconley)
Comment on attachment 8644773 [details] [diff] [review]
Fix warnings that show up when opening the downloads window in private browsing mode

Review of attachment 8644773 [details] [diff] [review]:
-----------------------------------------------------------------

I think you're missing one more in DownloadsCommon.jsm, in getSummary.

r=me with that fixed.
Attachment #8644773 - Flags: review?(mconley) → review+
Comment on attachment 8644773 [details] [diff] [review]
Fix warnings that show up when opening the downloads window in private browsing mode

Review of attachment 8644773 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/DownloadsCommon.jsm
@@ +227,5 @@
>     * @param aWindow
>     *        The browser window which owns the download button.
>     */
>    getData(aWindow) {
> +    if (PrivateBrowsingUtils.isContentWindowPrivate(aWindow)) {

Hm, why are we using PrivateBrowsingUtils.isContentWindowPrivate if aWindow is a browser XUL window?
In fact, looks like DownloadsCommon isn't in the warning stacks.
Flags: needinfo?(mconley)
Nevermind, didn't see it at first.
So, it looks like the argument to the DownloadsCommon functions can be the browser window or the content window for the in-content views.
Hmm, does that mean that my patch is wrong?
https://hg.mozilla.org/mozilla-central/rev/ae5f5a18ddd0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Looking at the code in PrivateBrowsingUtils, it looks like we actually converging on the same codepath that we were before (using this.privateContextFromWindow(aWindow).usePrivateBrowsing).

The only difference is that it looks like isContentWindowPrivate has documentation that states that that method is only to be used from framescripts.

Why is that requirement there, ehsan?
Flags: needinfo?(mconley) → needinfo?(ehsan)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> Looking at the code in PrivateBrowsingUtils, it looks like we actually
> converging on the same codepath that we were before (using
> this.privateContextFromWindow(aWindow).usePrivateBrowsing).
> 
> The only difference is that it looks like isContentWindowPrivate has
> documentation that states that that method is only to be used from
> framescripts.
> 
> Why is that requirement there, ehsan?

Bill added it in bug 1069059 and you reviewed.  I was never consulted.  :-)
Flags: needinfo?(ehsan) → needinfo?(wmccloskey)
Well, the rules are supposed to be as follows:
- If you're in a frame script, use isContentWindowPrivate.
- If you're not, and you have a <browser> element, use isBrowserPrivate.
- Otherwise, you need to have a chrome window and use isWindowPrivate.
The idea is to avoid people passing around generic DOM windows without really knowing whether it's a content window or a chrome window since that style is really bad for e10s.

In this case, about:downloads is a content DOM window that's loaded in the chrome process, which the rules weren't really designed for. I guess using isContentWindowPrivate is okay. However, I still feel like it would be nicer to pass something different around than a DOM window. In the code I've looked at so far, it seems like it would be pretty easy to find the <browser> element or the XUL window. The <browser> element is a little nicer in case we ever want to do per-tab private browsing I guess (people have told me that's something we might want to do).
I don't think we have any plans for ever doing a per-tab PB mode.

Besides that, is there anything else to be done in this bug?
(In reply to Ehsan Akhgari (don't ask for review please) from comment #15)
> I don't think we have any plans for ever doing a per-tab PB mode.

OK, good. It's much easier to just check the private browsing state on the window.

> Besides that, is there anything else to be done in this bug?

No, guess not.
Flags: needinfo?(wmccloskey)
Thanks!
You need to log in before you can comment on or make changes to this bug.