Last Comment Bug 795065 - Add privacy status to nsDownload
: Add privacy status to nsDownload
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla18
Assigned To: Josh Matthews [:jdm]
:
:
Mentors:
Depends on: 797524 798280 803661 814264 821328 830066 830281 1013234
Blocks: 722859 794606 799131
  Show dependency treegraph
 
Reported: 2012-09-27 12:16 PDT by Josh Matthews [:jdm]
Modified: 2014-07-05 09:53 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Add privacy status to nsDownload. s (39.37 KB, patch)
2012-09-27 14:32 PDT, Josh Matthews [:jdm]
bzbarsky: superreview+
Details | Diff | Splinter Review
Add privacy status to nsDownload. (47.93 KB, patch)
2012-10-01 09:36 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Add privacy status to nsDownload. s (48.45 KB, patch)
2012-10-01 18:52 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Add privacy status to nsDownload. s (39.37 KB, patch)
2012-10-02 13:34 PDT, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Add privacy status to nsDownload. s (55.90 KB, patch)
2012-10-02 13:38 PDT, Josh Matthews [:jdm]
mak77: review+
Details | Diff | Splinter Review
SeaMonkey bustage fix. (1.53 KB, patch)
2012-10-06 09:18 PDT, Philip Chee
mak77: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-09-27 12:16:14 PDT

    
Comment 1 Josh Matthews [:jdm] 2012-09-27 14:32:32 PDT
Created attachment 665658 [details] [diff] [review]
Add privacy status to nsDownload.  s
Comment 2 Josh Matthews [:jdm] 2012-09-27 14:33:03 PDT
Comment on attachment 665658 [details] [diff] [review]
Add privacy status to nsDownload.  s

Better documentation this time?
Comment 3 Josh Matthews [:jdm] 2012-09-27 14:36:34 PDT
Note to self: rev nsIDownload iid.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-09-27 19:39:52 PDT
Comment on attachment 665658 [details] [diff] [review]
Add privacy status to nsDownload.  s

sr=me
Comment 5 Josh Matthews [:jdm] 2012-09-27 22:41:07 PDT
Looping in comm-central folk for another API change.
Comment 6 neil@parkwaycc.co.uk 2012-09-28 03:02:56 PDT
I think we only use this in one of our tests (we add a fake download to see whether our sanitiser can remove it); fortunately toolkit's contentAreaUtils handles all the download/persist stuff for us.
Comment 7 Josh Matthews [:jdm] 2012-09-28 14:08:33 PDT
Note to self - update NS_DOWNLOAD_CID when revving.
Comment 8 :Paolo Amadini 2012-09-30 08:23:25 PDT
Comment on attachment 665658 [details] [diff] [review]
Add privacy status to nsDownload.  s

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

Since I have to look at the dependent patches also, before reviewing the PWPBM-enabled download manager, I'll make a first-pass review here, hoping to give useful input and speed up the process.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +974,3 @@
>    if (!dl)
>      return NS_ERROR_OUT_OF_MEMORY;
> +  dl->mPrivate = mInPrivateBrowsing;

Note...

@@ +1338,5 @@
>    if (!dl)
>      return NS_ERROR_OUT_OF_MEMORY;
>  
>    // give our new nsIDownload some info so it's ready to go off into the world
>    dl->mTarget = aTarget;

...how...

@@ +2163,5 @@
>                             mLastUpdate(PR_Now() - (uint32_t)gUpdateInterval),
>                             mResumedAt(-1),
>                             mSpeed(0),
>                             mHasMultipleFiles(false),
> +                           mPrivate(aPrivacyContext && aPrivacyContext->UsePrivateBrowsing()),

...none of the other properties of nsDownload are initialized in the constructor, initializing mPrivate after construction looks more consistent and you needn't even pass nsnull arbitrarily above.

::: toolkit/components/downloads/nsIDownloadManager.idl
@@ +110,5 @@
>     *
> +   * @param aPrivacyContext A context that will be used to determine the privacy
> +   *                        status of the new download. May be null if there is
> +   *                        no logical context (ie. a source window or document)
> +   *                        that can be associated with this operation.

From the comment, it's not clear what happens when aPrivacyContext is null. It should at least reply to the questions:

- Is the download considered private?
- How can I start a download and decide its privacy status when I have no context to start with?

::: toolkit/content/contentAreaUtils.js
@@ +388,5 @@
>    var targetFileURL = makeFileURI(persistArgs.targetFile);
>  
> +  var privacyContext = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                             .getInterface(Ci.nsIWebNavigation)
> +                             .QueryInterface(Ci.nsILoadContext);

I get that here we're using the load context of the chrome window because it stores the same flag as all the content window it contains (_setPerWindowPBFlag, http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#122, right?).

However this is not the original load context of the page we're saving. It works, what concerns me is that it's not clear from the nsITransfer interface that we can use any load context, as long as it has the correct PBM flag. So, we can't and shouldn't infer other things from the load context (like if the original document was chrome or content window).

Given that, I'd feel safer if we had an explicit aPrivate flag on nsITransfer and AddDownload, to prevent future misuse. Also, it's much easier to handle and to explain :-)

With a boolean flag we'll not be able to associate the download with a specific window in the future, but doing that would've not been safe in any case, given how we use the parameter now and how add-ons may use it. We could just add a window parameter if and when the need comes.
Comment 9 Josh Matthews [:jdm] 2012-10-01 09:36:12 PDT
Created attachment 666591 [details] [diff] [review]
Add privacy status to nsDownload.

Here's the patch with Paolo's comments addressed. I won't obsolete the old one in case Marco has in-progress splinter comments.
Comment 10 Josh Matthews [:jdm] 2012-10-01 11:45:22 PDT
https://tbpl.mozilla.org/?tree=Try&rev=79d7b30c53c8
Comment 11 Marco Bonardo [::mak] 2012-10-01 12:29:57 PDT
Comment on attachment 665658 [details] [diff] [review]
Add privacy status to nsDownload.  s

looking directly the new version ( I appreciate the care though, so many times I ended up commenting an old version of a patch :) )
Comment 12 Marco Bonardo [::mak] 2012-10-01 14:08:55 PDT
Comment on attachment 666591 [details] [diff] [review]
Add privacy status to nsDownload.

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

it looks ok, though I would like to have a second look once all the patches are updated and questions answered

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +1328,5 @@
>  
> +#if !(defined(MOZ_PER_WINDOW_PRIVATE_BROWSING)) && defined(DEBUG)
> +  // This code makes sure that in global private browsing mode, the flag
> +  // passed to us matches the global PB mode.  This can be removed when
> +  // per-window private browsing has been turned on.

In other patches (the history ones) you are totally removing global pb support... so I'm sort of confused on the current target, if we keep supporting it that patch will break us, otherwise this code sounds no more useful... please clarify, since if we keep it working and allow to switch it on and off the history patch should have all the ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING stuff...

@@ +1334,5 @@
> +    do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
> +  if (pbService) {
> +    bool inPrivateBrowsing = false;
> +    if (NS_SUCCEEDED(pbService->GetPrivateBrowsingEnabled(&inPrivateBrowsing))) {
> +      MOZ_ASSERT(inPrivateBrowsing == aIsPrivate);

nit: fwiw, you may use MOZ_ASSERT_IF

@@ +2168,5 @@
>    , nsIWebProgressListener2
>  )
>  
> +nsDownload::nsDownload()
> +                         : mDownloadState(nsIDownloadManager::DOWNLOAD_NOTSTARTED),

please revert the indentation change here

::: toolkit/content/contentAreaUtils.js
@@ +260,5 @@
>   * @param aReferrer
>   *        the referrer URI object (not URL string) to use, or null
>   *        if no referrer should be sent.
> + * @param aInitiatingDocument
> + *        The document from which the save was initiated.

So, is it optional? cause it looks like it is from the code below... you should specify [optional], what it is used for and what happens by default...
But actually, is there a specific reason this must be optional? Some case where we don't know the document? cause otherwise I'd just make it mandatory...

@@ +374,5 @@
>   *        "text/plain" is meaningful.
>   * @param persistArgs.bypassCache
>   *        If true, the document will always be refetched from the server
> + * @param persistArgs.initiatingWindow
> + *        The window from which the save operation was initiated.

ditto
Comment 13 Josh Matthews [:jdm] 2012-10-01 14:41:08 PDT
>::: toolkit/components/downloads/nsDownloadManager.cpp
>@@ +1328,5 @@
>>  
>> +#if !(defined(MOZ_PER_WINDOW_PRIVATE_BROWSING)) && defined(DEBUG)
>> +  // This code makes sure that in global private browsing mode, the flag
>> +  // passed to us matches the global PB mode.  This can be removed when
>> +  // per-window private browsing has been turned on.
>
>In other patches (the history ones) you are totally removing global pb support... so I'm sort 
>of confused on the current target, if we keep supporting it that patch will break us, 
>otherwise this code sounds no more useful... please clarify, since if we keep it working and 
>allow to switch it on and off the history patch should have all the ifdef 
>MOZ_PER_WINDOW_PRIVATE_BROWSING stuff...

This is a temporary check to help catch regressions while we are still using the global PB service in Firefox. It's been useful in other areas of the code; I suppose I can re-add the checks in the history patch on a similar basis.
Comment 14 Josh Matthews [:jdm] 2012-10-01 18:52:19 PDT
Created attachment 666796 [details] [diff] [review]
Add privacy status to nsDownload.  s

In fact, the assertion was very useful and caught some badness that was happening.
Comment 15 Josh Matthews [:jdm] 2012-10-01 18:53:45 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7055863e298b
Comment 16 Marco Bonardo [::mak] 2012-10-02 06:00:45 PDT
(In reply to Josh Matthews [:jdm] from comment #13)
>  while we are still using
> the global PB service in Firefox.

This is what concerns me, your other patches don't allow us to keep using global PB, so we need a final decision, either we keep them alongside or we throw away global PB, there's no way to do half and half.
Comment 17 Josh Matthews [:jdm] 2012-10-02 07:53:28 PDT
(In reply to Marco Bonardo [:mak] from comment #16)
> (In reply to Josh Matthews [:jdm] from comment #13)
> >  while we are still using
> > the global PB service in Firefox.
> 
> This is what concerns me, your other patches don't allow us to keep using
> global PB, so we need a final decision, either we keep them alongside or we
> throw away global PB, there's no way to do half and half.

I don't understand your concern. All of the per-window PB changes are designed to continue working alongside the global service (which sets the privacy information of the root docshell of each window and propagates through children), and we can change the front-end to start doing per-window and everything should just work (at which point we'll take out the debug asserts).
Comment 18 Marco Bonardo [::mak] 2012-10-02 12:32:14 PDT
(In reply to Josh Matthews [:jdm] from comment #17)
> I don't understand your concern.

Could be I didn't express properly.
So I'm concerned about this:
#if !(defined(MOZ_PER_WINDOW_PRIVATE_BROWSING))

This means PER_WINDOW_PRIVATE_BROWSING can be disabled at build time... If this happens global PB is the only remaining PB mode. Though in bug 723005 you removed any global PB block from history, so if per window PB is disabled, global PB will just not work.
Comment 19 Josh Matthews [:jdm] 2012-10-02 12:38:25 PDT
That's just a build flag we're using to allow us to make certain all-or-nothing changes before we actually reach the point where we're ready to turn on per-window PB for desktop Firefox. We're not releasing any builds with that turned on, it's not documented anywhere, and it's going away.
Comment 20 Josh Matthews [:jdm] 2012-10-02 12:42:39 PDT
Actually, I still am having trouble understanding your concern. Here's the situation:
* MOZ_PER_WINDOW_PRIVATE_BROWSING is a build flag that is disabled by default
* Global private browsing is the only exposed way of using private browsing in the browser
* These assertion blocks are turned on by default. If someone builds with MOZ_PER_WINDOW_PRIVATE_BROWSING enabled, the assertions disappear. The code in (for example) the history changes doesn't stop checking the calling sites - ideally there is no change in behaviour, because all problematic calls to history-related functions have been caught via the assertions and fixed at the callers.
Comment 21 Marco Bonardo [::mak] 2012-10-02 12:45:50 PDT
(In reply to Josh Matthews [:jdm] from comment #20)
> all problematic calls to
> history-related functions have been caught via the assertions and fixed at
> the callers.

Yes my fear was that MOZ_PER_WINDOW_PRIVATE_BROWSING was also disabling the callers checks about private window, but looks like it's not the case. So there should be no problem.  So per window data is available regardless this PER_WINDOW_PB... just that the name of the config is sort of confusing :)
Comment 22 Josh Matthews [:jdm] 2012-10-02 13:34:20 PDT
Created attachment 667139 [details] [diff] [review]
Add privacy status to nsDownload.  s

I'm not sure how I missed so many call sites for the internalSave stuff, but this makes try happy.
Comment 23 Josh Matthews [:jdm] 2012-10-02 13:38:01 PDT
Created attachment 667140 [details] [diff] [review]
Add privacy status to nsDownload.  s

Sorry, too many trees and versions of patches.
Comment 24 Josh Matthews [:jdm] 2012-10-02 13:48:57 PDT
Comment on attachment 667140 [details] [diff] [review]
Add privacy status to nsDownload.  s

>+                          Math.round(Date.now() * 1000), null, persist, isPrivate);

I'll fix this before landing.

>+  var aInitiatingDoc        = params.initiatingDoc ? params.initiatingDoc : null;

You may be surprised by this, given that I made the other similar values required. However, I audited all the call sites of openLinkIn and openLink, and the times in which this information is required (for "save") are quite rare.
Comment 25 Marco Bonardo [::mak] 2012-10-03 02:17:33 PDT
Comment on attachment 666796 [details] [diff] [review]
Add privacy status to nsDownload.  s

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

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +972,5 @@
>    // We have a download, so lets create it
>    nsRefPtr<nsDownload> dl = new nsDownload();
>    if (!dl)
>      return NS_ERROR_OUT_OF_MEMORY;
> +  dl->mPrivate = mInPrivateBrowsing;

Paolo has a point about not needing to do complicate initialization in the constructor, though I'd still prefer if mPrivate had a default false value in the constructor initializers than a random value.

::: toolkit/components/downloads/test/unit/test_sleep_wake.js
@@ +142,5 @@
>                           nsIWBP.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;
>    let dl = dm.addDownload(nsIDM.DOWNLOAD_TYPE_DOWNLOAD,
>                            createURI("http://localhost:4444/resume"),
>                            createURI(destFile), null, null,
> +                          Math.round(Date.now() * 1000), null, persist, null);

should this be false rather than null?

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1946,3 @@
>    rv = aTransfer->Init(mSourceUrl, target, EmptyString(),
> +                       mMimeInfo, mTimeDownloadStarted, mTempFile, this,
> +                      channel && NS_UsePrivateBrowsing(channel));

wrong indentation
Comment 26 Marco Bonardo [::mak] 2012-10-03 02:25:38 PDT
Comment on attachment 667140 [details] [diff] [review]
Add privacy status to nsDownload.  s

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

yes, the risk is to have missed some callpoints, though even if I'd spend a full day searching mxr to ensure that, it's likely I'd still miss some, so no point in doing that, at least fixing them should be easy.

::: browser/base/content/utilityOverlay.js
@@ +224,5 @@
>    var aInBackground         = params.inBackground;
>    var aDisallowInheritPrincipal = params.disallowInheritPrincipal;
>    // Currently, this parameter works only for where=="tab" or "current"
>    var aIsUTF8               = params.isUTF8;
> +  var aInitiatingDoc        = params.initiatingDoc ? params.initiatingDoc : null;

why the check? at a maximum it's undefined, not a problem for your usage

::: mobile/android/chrome/content/browser.js
@@ +501,5 @@
>             contentDisposition = "";
>             type = "";
>          }
> +        ContentAreaUtils.internalSave(aTarget.currentURI.spec, null, null, contentDisposition, type, false, "SaveImageTitle", null, aTarget.ownerDocument.documentURIObject,
> +                                      aTarget.ownerDocument, true, null);

nit: reindent this properly?
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-10-03 19:00:45 PDT
https://hg.mozilla.org/mozilla-central/rev/eff426a003e1
Comment 29 Philip Chee 2012-10-05 10:10:51 PDT
For applications like Thunderbird (or Sunbird) where there are no PB windows what sort of document do you pass to saveURL() or internalSave()?
Comment 30 Josh Matthews [:jdm] 2012-10-05 10:12:38 PDT
If there are no plans for ever implementing something based on PB mode, it doesn't matter what document is passed.
Comment 31 Philip Chee 2012-10-06 09:18:12 PDT
Created attachment 668789 [details] [diff] [review]
SeaMonkey bustage fix.

In SeaMonkey I get this error message:

> Sat Oct 06 2012 23:34:48
> Error: ReferenceError: Ci is not defined
> Source file: chrome://global/content/contentAreaUtils.js
> Line: 434

contentAreaUtils.js should not assume that Ci/Cc/Cr is defined in the global scope since it's used in more than Firefox.
Comment 32 Marco Bonardo [::mak] 2012-10-08 06:49:37 PDT
Comment on attachment 668789 [details] [diff] [review]
SeaMonkey bustage fix.

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

ops, thanks for the fix.
Comment 33 Philip Chee 2012-10-08 09:05:30 PDT
Thanks. Setting checkin-needed because I don't have -inbound locally.
Comment 34 Ryan VanderMeulen [:RyanVM] 2012-10-08 18:02:32 PDT
(In reply to Philip Chee from comment #33)
> Thanks. Setting checkin-needed because I don't have -inbound locally.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c33b5e11209a
Comment 35 Ed Morley [:emorley] 2012-10-09 07:32:29 PDT
https://hg.mozilla.org/mozilla-central/rev/c33b5e11209a
Comment 36 Ed Morley [:emorley] 2012-10-09 07:58:33 PDT
(In reply to Ed Morley [:edmorley UTC+1] from comment #35)
> https://hg.mozilla.org/mozilla-central/rev/c33b5e11209a

This part landed on mozilla19.
Comment 37 Philip Chee 2012-10-10 15:38:04 PDT
Comment on attachment 668789 [details] [diff] [review]
SeaMonkey bustage fix.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 795065
User impact if declined: SeaMonkey errors when trying to save a page:
<https://bugzilla.mozilla.org/show_bug.cgi?id=799529#c2>

Save Page as works with 'Text', but not with the 'HTML' (Web page) options.
Download Manager says:  Unable to save, Not started

> Error: ReferenceError: Ci is not defined
> Source file: chrome://global/content/contentAreaUtils.js
> Line: 434

Line 434:
> .QueryInterface(Ci.nsIInterfaceRequestor)
 
 
Save Image As is not working at all.

> Error: TypeError: aInitiatingDocument is undefined
> Source file: chrome://global/content/contentAreaUtils.js
> Line: 340

Line 340:
> initiatingWindow  : aInitiatingDocument.defaultView

Testing completed (on m-c, etc.): mozilla-central, comm-central.
Risk to taking this patch (and alternatives if risky): no risk bustage fix.
String or UUID changes made by this patch: None.
Comment 38 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-10 16:02:59 PDT
Comment on attachment 668789 [details] [diff] [review]
SeaMonkey bustage fix.

approving low-risk, bustage fix for 17.0 on Beta.
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-10-11 18:14:40 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef941370f8d2
Comment 40 JoeG 2012-10-18 01:11:09 PDT
Please see Bug 802472 and the Mozilla forum discussion: http://forums.mozillazine.org/viewtopic.php?f=23&t=2576141&p=12386465#p12386465

Several (all?) of the screenshot extensions aren't working. Bug 795065 was marked as fixed on 11 Oct, and the screenshot issue apparently started as a result.
Comment 41 neil@parkwaycc.co.uk 2012-10-19 12:45:00 PDT
Comment on attachment 667140 [details] [diff] [review]
Add privacy status to nsDownload.  s

> function ViewSourceSavePage()
> {
>   internalSave(window.content.location.href.substring(12), 
>                null, null, null, null, null,
>-               "SaveLinkTitle", null, null, null, gPageLoader);
>+               "SaveLinkTitle", null, null, gDocument, null, gPageLoader);
> }
Unfortunately view source doesn't have a gDocument variable. Bug coming up.
Comment 42 David Adler 2012-11-01 20:46:47 PDT
This change breaks add-ons which call saveURI(), such as Thumbnail Zoom Plus' "Save Enlarged Image As..." command.  

Are add-on developers expected to add the argument to their code in saveURI calls (and also nsITransfer init() calls), or should I file a bug about this add-on incompatibility?

David Adler (developer of Thumbnail Zoom Plus)
Comment 43 Philip Chee 2012-11-02 02:37:58 PDT
As I understand it, this is a deliberate change in the API. Going forward AMO will require extension authors to respect the private browsing status of Firefox so you might as well start passing the right parameters.

Phil Chee (developer of Flashblock and others)
Comment 44 David Adler 2012-11-02 14:11:24 PDT
Is there a doc anywhere explaining motivation for the change?  EG is this to support a future feature where some windows can be in Private Browsing and others not?  

More to the point, how can I test to verify that my add-on is properly honoring private status?  The various docs which discuss how to use saveURI haven't been updated yet.

I could simply pass false to transfer.init() and null to persist.saveURI() but would that be correct?

The code below seems to work in my add-on but I don't know if it is all necessary (win is the html doc's window, not the chrome window):

  let privacyContext = win.QueryInterface(Ci.nsIInterfaceRequestor)
                            .getInterface(Ci.nsIWebNavigation)
                            .QueryInterface(Ci.nsILoadContext);
  let isPrivate = privacyContext.usePrivateBrowsing;

  let transfer = Cc["@mozilla.org/transfer;1"].createInstance(Ci.nsITransfer);
  transfer.init(source, target, "", null, null, null, persist, isPrivate);
  persist.saveURI(source, null, null, null, null, aFile, privacyContext);
Comment 45 Josh Matthews [:jdm] 2012-11-02 14:22:23 PDT
Yes, this change was a requirement for the upcoming work to support concurrent private and public windows. You can test it by performing actions in private browsing that trigger downloads via your add-on, and checking the disk cache (about:cache) for any revealing traces of the files downloaded.

Your code looks correct to me, with the exception of the transfer.init call which is quite incorrect. init takes a load context only.

You can make the code more maintainable by using the PrivateBrowsingUtils module (resource://gre/modules/PrivateBrowsingUtils.jsm) to both obtain the load context (PrivateBrowsingUtils.loadContextFromWindow(win)) and the privacy status you care about (PrivateBrowsingUtils.isWindowPrivate(win)).
Comment 46 Josh Matthews [:jdm] 2012-11-02 14:23:38 PDT
Sorry, that should have been PrivateBrowsing.privacyContextFromWindow, not loadContextFromWindow. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PrivateBrowsingUtils.jsm
Comment 47 David Adler 2012-11-02 17:46:41 PDT
I'll use PRivateBrowsingUtils; thanks for the tip.

Re transfer.init(), it seems to expect a boolean; note that this is an nsITransfer, not an nsITransferable.  From http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsITransfer.idl#14 

48      * @param aIsPrivate Used to determine the privacy status of the new transfer.
49      *                   If true, indicates that the transfer was initiated from
50      *                   a source that desires privacy.
51      */
52     void init(in nsIURI aSource,
53               in nsIURI aTarget,
54               in AString aDisplayName,
55               in nsIMIMEInfo aMIMEInfo,
56               in PRTime startTime,
57               in nsIFile aTempFile,
58               in nsICancelable aCancelable,
59               in boolean aIsPrivate);
Comment 48 Josh Matthews [:jdm] 2012-11-03 08:29:24 PDT
Ack, I'm sorry. I constantly get those two interfaces confused. Carry on.
Comment 49 David Adler 2012-11-04 13:46:53 PST
In case it helps anyone else, this is what I end up with (including ability to work with older Firefox versions including 3.6):

try {
  Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
} catch (e) {
  // old Firefox versions (e.g. 3.6) didn't have PrivateBrowsingUtils.
}
...

  _saveImage : function(win, imageURL, aFile) {
    this._logger.trace("_saveImage");

    let ioService =
      Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
    let persist =
      Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"].
        createInstance(Ci.nsIWebBrowserPersist);
    let source = ioService.newURI(imageURL, "UTF8", null);
    let target = ioService.newFileURI(aFile);

    if (win && "undefined" != typeof(PrivateBrowsingUtils) &&
        PrivateBrowsingUtils.privacyContextFromWindow) {
      var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(win);
      var isPrivate = privacyContext.usePrivateBrowsing;
    } else {
      // older than Firefox 19 or couldn't get window.
      var privacyContext = null;
      var isPrivate = false;
    }
      
    this._logger.debug("_saveImage: privacyContext=" + privacyContext + "; isPrivate=" + isPrivate);

    // set persist flags
    persist.persistFlags =
      (Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES |
       Ci.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);

    // Create a 'transfer' object and set it as the progress listener.
    // This causes the downloaded image to appear in the Firefox
    // "Downloads" dialog.
    let transfer = Cc["@mozilla.org/transfer;1"].createInstance(Ci.nsITransfer);
    transfer.init(source, target, "", null, null, null, persist, isPrivate);
    persist.progressListener = transfer;
    
    // save image to the file
    persist.saveURI(source, null, null, null, null, aFile, privacyContext);
  }
Comment 50 Marco Bonardo [::mak] 2012-11-05 02:26:51 PST
I think would be useful for everyone to have this conversation in mozilla.dev.extensions and I suspect there should be more insight there for developers and what they need to do.
Comment 51 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-21 18:05:10 PST
Comment on attachment 667140 [details] [diff] [review]
Add privacy status to nsDownload.  s

>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js

>@@ -104,17 +104,18 @@ function openUILink(url, event, aIgnoreB

>+      initiatingDoc: event.target.ownerDocument

requiring a non-null "event" for openUILink is going to cause lots of extension bustage (and caused bustage in Firefox itself, see bug 805523). Passing null if no event is specified seems like the better call here - it means failing to properly handle PB mode in the rare instances where we'll use the "save" action, but that should be a very small minority of cases, and we can add a warning for that or something. I'll file a bug.

Note You need to log in before you can comment on or make changes to this bug.