The global PB service is going away. nsExternalAppHelperService needs some way of determining which files are temporary for a PB window instance, and deal with them somehow.

We can use the notification in bug 725210 to expunge all current temporary private files in nsExternalAppHelperService::Observe. With regards to nsExternalAppHelper::OpenWithApplication, we can probably store the private status of the related nsIRequest before calling OpenWithApplication. That leaves nsExternalHelperAppService::DeleteTemporaryFileOnExit, which is called in these locations: http://mxr.mozilla.org/mozilla-central/ident?i=DeleteTemporaryFileOnExit Many of those have ways of accessing a source of privacy information, as far as I can tell, so we can probably pass in a flag to DeleteTemporaryFileOnExit.

This API is intended to be used by the callers when they want to delete a temporary file created in private browsing mode as soon as possible.

Comment on attachment 618145 [details] [diff] [review] Part 1: Add the deleteTemporaryPrivateFileWhenPossible API r=me

Comment on attachment 618148 [details] [diff] [review] Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API r=me

Comment on attachment 618154 [details] [diff] [review] Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication >+ ctx->GetUsePrivateBrowsing(&inPrivateBrowsing); inPrivateBrowsing = ctx->UsePrivateBrowsing(); please. r=me

Attachment 618157 [details] [diff] is the last patch for this bug. :-)

Comment on attachment 618155 [details] [diff] [review] Part 6: Delete the temporary private files when the last private browsing window is closed r=me

Comment on attachment 618157 [details] [diff] [review] Part 7: Remove the usage of the global private browsing in the external app helper service since it is no longer needed r=me

Addressed the review comment.

Comment on attachment 618161 [details] [diff] [review] Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication Review of attachment 618161 [details] [diff] [review]: ----------------------------------------------------------------- ::: uriloader/exthandler/nsExternalHelperAppService.cpp @@ +2235,5 @@ > + NS_ASSERTION(mRequest, "This should never be called with a null request"); > + nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest); > + if (channel) { > + nsCOMPtr<nsILoadContext> ctx; > + NS_QueryNotificationCallbacks(channel, ctx); This won't work in multiprocess mode. Could you file a followup to make this use the machinery in bug 722845 when it lands?

However, let me be the first to say \o/

Comment on attachment 618147 [details] [diff] [review] Part 2: Make the download manager aware of the deleteTemporaryPrivateFileWhenPossible API This just moves the problematic checking of the global PB state to a different place, right? Is there another bug on making the download manager support per-window private browsing?

Comment on attachment 618152 [details] [diff] [review] Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API >diff --git a/toolkit/components/viewsource/content/viewSourceUtils.js b/toolkit/components/viewsource/content/viewSourceUtils.js >+ if (aDocument) { Are there cases where aDocument can be null that we need to worry about? >+ webNavigation.QueryInterface(Components.interfaces.nsILoadContext); This seems to be the viewSource window's docShell, so I don't think using it's PB state is relevant.

(In reply to Josh Matthews [:jdm] from comment #17) > Comment on attachment 618161 [details] [diff] [review] > Part 5: Use the channel's private browsing flag to determine how to handle > the temporary file in nsExternalAppHandler::OpenWithApplication > > Review of attachment 618161 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: uriloader/exthandler/nsExternalHelperAppService.cpp > @@ +2235,5 @@ > > + NS_ASSERTION(mRequest, "This should never be called with a null request"); > > + nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest); > > + if (channel) { > > + nsCOMPtr<nsILoadContext> ctx; > > + NS_QueryNotificationCallbacks(channel, ctx); > > This won't work in multiprocess mode. Could you file a followup to make this > use the machinery in bug 722845 when it lands? Filed bug 748890.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19) > Comment on attachment 618147 [details] [diff] [review] > Part 2: Make the download manager aware of the > deleteTemporaryPrivateFileWhenPossible API > > This just moves the problematic checking of the global PB state to a > different place, right? Is there another bug on making the download manager > support per-window private browsing? Yeah, that's bug 722859.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20) > Comment on attachment 618152 [details] [diff] [review] > Part 4: Make the view source window aware of the > deleteTemporaryPrivateFileWhenPossible API > > >diff --git a/toolkit/components/viewsource/content/viewSourceUtils.js b/toolkit/components/viewsource/content/viewSourceUtils.js > > >+ if (aDocument) { > > Are there cases where aDocument can be null that we need to worry about? No, I just imitated the existing null check.

This version of the patch uses the docshell of the correct document.

Gavin: review ping!

Comment on attachment 618399 [details] [diff] [review] Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API Forwarding the request to Mossop. I'm not sure who should review this, and Gavin implied on IRC that it's not him.

Comment on attachment 618399 [details] [diff] [review] Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API Review of attachment 618399 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/viewsource/content/viewSourceUtils.js @@ +330,5 @@ > + .QueryInterface(Components.interfaces.nsILoadContext) > + .usePrivateBrowsing; > + > + let helperService = Components.classes["@mozilla.org/uriloader/external-helper-app-service;1"] > + .getService(Components.interfaces.nsPIExternalAppLauncher); Line up the .'s here

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa83d71fe7ee https://hg.mozilla.org/integration/mozilla-inbound/rev/717f908c990a https://hg.mozilla.org/integration/mozilla-inbound/rev/86762d8c4de9 https://hg.mozilla.org/integration/mozilla-inbound/rev/66dfa9606626 https://hg.mozilla.org/integration/mozilla-inbound/rev/bde34cebdcdc https://hg.mozilla.org/integration/mozilla-inbound/rev/3ceeeaf0519f https://hg.mozilla.org/integration/mozilla-inbound/rev/a6175c97f365

Oops, resolved too soon! :-)

Sorry, backed out for: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=a6175c97f365 eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=13076404&tree=Mozilla-Inbound#error0 https://hg.mozilla.org/integration/mozilla-inbound/rev/b912941bed45

nsILocalFile is dead! https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1b57174183 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd64403cf172 https://hg.mozilla.org/integration/mozilla-inbound/rev/de044548f718 https://hg.mozilla.org/integration/mozilla-inbound/rev/660e21737556 https://hg.mozilla.org/integration/mozilla-inbound/rev/4cec29fdbcf4 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdfc3bda7be https://hg.mozilla.org/integration/mozilla-inbound/rev/481df5e313c5

https://hg.mozilla.org/mozilla-central/rev/ff1b57174183 https://hg.mozilla.org/mozilla-central/rev/dd64403cf172 https://hg.mozilla.org/mozilla-central/rev/de044548f718 https://hg.mozilla.org/mozilla-central/rev/660e21737556 https://hg.mozilla.org/mozilla-central/rev/4cec29fdbcf4 https://hg.mozilla.org/mozilla-central/rev/4bdfc3bda7be https://hg.mozilla.org/mozilla-central/rev/481df5e313c5

Comment on attachment 618161 [details] [diff] [review] Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication >+ // See whether the channel has been opened in private browsing mode >+ bool inPrivateBrowsing = false; >+ NS_ASSERTION(mRequest, "This should never be called with a null request"); Actually this happens whenever you turn off Always Ask for a particular download type...

(In reply to comment #34) > Comment on attachment 618161 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=618161 > Part 5: Use the channel's private browsing flag to determine how to handle the > temporary file in nsExternalAppHandler::OpenWithApplication > > >+ // See whether the channel has been opened in private browsing mode > >+ bool inPrivateBrowsing = false; > >+ NS_ASSERTION(mRequest, "This should never be called with a null request"); > Actually this happens whenever you turn off Always Ask for a particular > download type... Can you please file a bug about that?