The global service is going away. The Download Manager needs to use PB information from each request to decide what to do with it.

I expect we'll need to have both databases active at the same time, much like the cookie database.

This patch passes all of the download manager tests under toolkit/components/downloads. For every part of the API that manipulates the list of current or past downloads, a corresponding function that operates on the list of private downloads in the same way is introduced. Marco, what do you think of this in general? Is it ok to provide this duplicate API and modify consumers to decide which one to use?

Comment on attachment 594516 [details] [diff] [review] Make the download manager act on the private browsing status of each request WIP Paolo may probably give feedback earlier, so flagging him in the meanwhile. More eyes are better than less.

Comment on attachment 594516 [details] [diff] [review] Make the download manager act on the private browsing status of each request WIP Thank you very much, Josh, for working on this in your free time! I think this portion of the PB-per-window feature needs a bit more of design before starting to work on actual code. The solution of duplicating each API call doesn't seem ideal to me by instinct, though it might even turn out to be the way to go. We need more information. * Which are the current consumers of the API? Remember that the Download Manager back-end lives in Toolkit so it can be used by products other than Firefox. This is what I know: - AddDownload is called in a few places (e.g. uriloader/exthandler) - The Downloads window allows viewing and handling downloads globally - The same does the Firefox Downloads Panel (coming soon) https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager - Some add-ons add new downloads and can view them, probably. - Other places / products? * What kind of backwards compatibility support do we want to provide? I'd say that while we don't want to break downloads API consumers, we must ensure that they don't operate on the wrong database by mistake or by simple "lazyness" (leaving the current calls in place). * Which features do we need and which we don't need anymore for the Firefox downloads backend? One interesting thing about Firefox (not sure about other API consumers) is that, with the Downloads Panel in place, we don't really need disk persistence to "downloads.sqlite" anymore: download history is handled by Places and the Library window. We didn't want to change the back-end while working on the Panel, but it's something to keep in mind now that we're considering doing some major changes in behavior. * What would the interaction with downloads in a mixed-mode browsing session look like? What about applications other than Firefox? By the way, all this Q&A belongs to the Downloads section of the feature page: https://wiki.mozilla.org/Per-window_Private_Browsing I think that when we have enough information on the feature page, we should also get an early feedback from Firefox UX (https://developer.mozilla.org/En/Developer_Guide/Requesting_feedback_and_ui-review_for_desktop_Firefox_front-end_changes) and work with super-reviewers also for the API changes. Marco, if other questions come to your mind, feel free to add :-) Josh, a suggestion: link the feature page in the URL field of bug 463027, and put the link to your user repo (as well as to the meta-bug) in a "Bugs list and patch queue" section of the feature page. This will make it easier to find the feature page for people working on reviewing the bugs, and to find your repo for people who look at the feature page and are interested in testing or helping.

Comment on attachment 594516 [details] [diff] [review] Make the download manager act on the private browsing status of each request WIP Review of attachment 594516 [details] [diff] [review]: ----------------------------------------------------------------- hm, yeah, sadly I have to agree that duplicating each single api sounds like opening a beer can with an axe. This is likely more an argument for a super reviewer though, but at a first glance I wonder if there has been a discussion in m.d.platform re the approach to follow globally. There are many components involved in storing data and duping all of their APIs may be an overkill. Did we evaluate the simple "separate instance with temporary profile" option for per window PB? Are there m.d.platform discussions we may look into to gather more information? Can we bring the discussion on this duplicate-apis approach to the ng?

Current proposed API changes are being discussed on mozilla.dev.platform at the link in the URL field, for those following along at home.

Checkpointing my work. There's a leak in xpcshell tests that I need to track down, and I haven't tested any actual PB behaviour yet.

Another WIP, this time passing all xpcshell tests. Next stop, browser-chrome.

For the record, I went with Ehsan's suggestion that GetID continue to work for non-private downloads and assert if called on a private one. This means that apart from changing calls to addDownload, which unchanged will error on the missing privacyContext argument, the entire API should Just Work for existing consumers.

My mistake, here's the patch that appears to be passing all tests.

Comment on attachment 627176 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. PAolo, could you please do a first-pass review here?

(In reply to Marco Bonardo [:mak] from comment #14) > PAolo, could you please do a first-pass review here? Will do! Josh, just note it may be a up to a couple of weeks until I can look into this patch in detail. At first glance, I see we're keeping full compatibility with the old listing API for non-private downloads, and that's great! We're also introducing a new set of APIs for consumers wanting to list and manage private downloads. That's fine, but we'll also need a new API for making download handling asynchronous, and I'd like that to be the same new API, so that we need to update consumers only once. By the way, do you already have an idea about how we should change existing consumers (e.g. the Downloads Panel and the Downloads window) to work with per-window PBM?

Note we're also introducing a Downloads view in the Library window (bug 675902), currently in a preliminary design phase.

Paolo, let me know if there's anything I can do to make this review easier (eg. breaking it up somehow).

Comment on attachment 627176 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. I've looked into this in detail, but, as you suggested, before talking extensively about the code itself, let's try to split this into general steps. I think the approach of having an on-disk database and a memory database accessible at the same time is good, since consumers can continue to use the on-disk database at all times and not worry about private downloads at all. Firstly, to support multiple concurrent databases we need a different ID, and I think using GUIDs is the way to go, in line with what we're doing in Places. Let's get that done first. You should file a separate bug blocking this one. We should probably proceed like for Places, providing new APIs and gradually deprecating existing ones. Let's create a new getDownloadByGUID function in nsIDownloadManager, and add methods to nsIDownload directly, to cancel, resume, and so on. We'll keep the equivalent nsIDownloadManager methods that cannot work by GUID around for some time. As a quick code note, I'm pretty sure you don't need struct SearchData, it's easier to duplicate code in the entry point functions and have them call into a third, common one. You may want to look into this while you split the patch. After this step we can look into multiple database support and compatibility. The current version won't work, as we're sending out global notifications for downloads that may not be in the views, thus API consumers will break. If we want to stay compatible, we should probably send global notifications for public downloads only (and we should also deprecate global notifications in favor of specific subscriptions that can serve either private or public). This assumes that existing download views, if unmodified, won't ever see private downloads, as you already discussed with Ehsan if I remember correctly. Let me know if you have any question!

Also, getDownloadByGUID should return the nsIDownload through a callback function, even if not strictly asynchronously at first. Feel free to do that in a future iteration of the patch, however.

(In reply to comment #18) > This assumes that existing download views, if unmodified, won't ever see > private downloads, as you already discussed with Ehsan if I remember correctly. That's correct.

Checkpointing the work. I haven't updated any of the JS users from the old patch yet.

Lots of tests passing; still need to do a try run.

Try run was green: https://tbpl.mozilla.org/?tree=Try&rev=3e97d303c65f

Comment on attachment 649428 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. I think this is ready for your comments, Paolo.

Comment on attachment 649428 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. This still doesn't account for the fact that we need to notify download listeners and global download topic observers only for non-private downloads. We should probably add a method, in addition to AddListener, that should allow the caller to receive events for all download types. Also, removed downloads are notified using a global observer notification, and this also needs an alternative for private downloads (might be a notification with another name, but could also be a specific listener method). On another topic, I still have to figure out if it's appropriate for nsITransfer to have a dependency to nsILoadContext. Inter-module depencencies are hard to get right, I'd like to have the opinion of a super-reviewer here. If you'd like me to go through the patch in more detail, just let me know and I can do that, though you might already need to rework some parts based on the two issues above.

I almost forgot to mention, I'd also recommend we are prepared to handle the case the global private browsing service is still in use. We can also land the back-end changes and the corresponding front-end all at once, but being backwards-compatible would simplify the migration process.

I believe I addressed the issues you brought up. I introduced a new interface that receives all notifications; the old interface is only triggered for public downloads, and the global observer notifications similarly so, while an identical but differently-named one is fired for all downloads.

https://tbpl.mozilla.org/?tree=Try&rev=afbcd2b21053, for the record.

Comment on attachment 664777 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. 722859 - Support concurrent private/public downloads in the download manager WIP. * * Benjamin, how do you feel about the addition of nsILoadContext to nsITransfer::Init here?

Comment on attachment 664777 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. 722859 - Support concurrent private/public downloads in the download manager WIP. * * I am not the right person to have an opinion.

Comment on attachment 664777 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. 722859 - Support concurrent private/public downloads in the download manager WIP. * * I'd like to see some documentation in the IDL about what the new argument means, exactly.

I think I'm going to move the nsITransferable changes to another bug, since several other bugs I'm working on can make immediate use of them.

Here's a nicer version that strips out all of the changes from bug 795065. I won't obsolete the old version in case you've been making comments on it.

Comment on attachment 664777 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. 722859 - Support concurrent private/public downloads in the download manager WIP. * * Is this still waiting on sr from me? On which part, given the transferable bits moving to other bugs?

Comment on attachment 664777 [details] [diff] [review] Support concurrent private/public downloads in the download manager WIP. 722859 - Support concurrent private/public downloads in the download manager WIP. * * I don't believe the SR is necessary any longer.

Comment on attachment 666060 [details] [diff] [review] Support concurrent public/private downloads in the download manager. Review of attachment 666060 [details] [diff] [review]: ----------------------------------------------------------------- We're getting closer, my main concerns are about keeping compatibility when the patch lands. I've not looked into the tests in detail. It's also true that, if we want to create a patch that can land without breaking existing consumers, it probably means that we should also run it against the existing, unchanged tests. So, you could separate the updated tests in a new patch and, initially, test the code with and without this tests-only patch. ::: browser/base/content/sanitize.js @@ +279,5 @@ > } > else { > // Clear all completed/cancelled downloads > dlMgr.cleanUp(); > + dlMgr.cleanUpPrivate(); This will force remove-download listeners to reload the full downloads list twice, not a big deal though. ::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadmonitor.js @@ +85,5 @@ > if (file.exists()) > file.remove(false); > > Services.obs.removeObserver(promptObserver, "common-dialog-loaded", false); > + attemptFinish(); I don't see this function defined. ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +146,5 @@ > { > + nsresult rv = PauseAllDownloads(mCurrentDownloads, aSetResume); > + nsresult rv2 = PauseAllDownloads(mCurrentPrivateDownloads, aSetResume); > + if (NS_FAILED(rv)) > + return rv; Could just be NS_ENSURE_SUCCESS. @@ +1094,5 @@ > > rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("id"), aID); > NS_ENSURE_SUCCESS(rv, rv); > > + return GetDownloadFromDB(mDBConn, stmt, retVal); This isn't compatible with the global PBM service, right? That is, GetDownloadFromDB will never find a result from the private database. We should find a way to land this patch without breaking current consumers. Maybe we can still listen to the global PBM notifications and enable lookup by ID in the private database when it's enabled? Also, we could switch old-style listeners to be notified only of private downloads instead of non-private downloads. Or do you think you're close enough to making a "leap" for which we enable per-window PBM all together? @@ +1790,5 @@ > if (dl) > return NS_ERROR_FAILURE; > > + bool isPrivate = false; > + nsresult rv = GetDownloadFromDB(aGUID, getter_AddRefs(dl)); If this fails, we might as well abort... @@ +1796,5 @@ > + isPrivate = dl->mPrivate; > + } > + > + RemoveDownloadByGUID(aGUID, mDBConn); > + RemoveDownloadByGUID(aGUID, mPrivateDBConn); ...so that if we succeed we know for sure whether the download is private here. @@ +2047,4 @@ > nsDownloadManager::AddListener(nsIDownloadProgressListener *aListener) > { > + nsCOMPtr<nsIPrivacyAwareDownloadProgressListener> listener = > + do_QueryInterface(aListener); I don't think we need a new interface here. Two new methods should suffice (AddPrivacyAwareListener and RemovePrivacyAwareListener), as we already pass an nsIDownload to all the methods of the existing interface from which it can be determined whether the download is private. @@ +2319,5 @@ > + InitMemoryDB(); > + for (PRInt32 i = 0; i < mCurrentPrivateDownloads.Count(); i++) { > + mCurrentPrivateDownloads[i]->Cancel(); > + } > + mCurrentPrivateDownloads.Clear(); InitMemoryDB() should logically follow mCurrentPrivateDownloads.Clear(). Canceling might also have consequences on the database. @@ +2627,5 @@ > // Only send the dl-start event to downloads that are actually starting. > + if (oldState == nsIDownloadManager::DOWNLOAD_QUEUED) { > + if (!mPrivate) > + mDownloadManager->SendEvent(this, "dl-start"); > + mDownloadManager->SendEvent(this, "dl-start-unique"); We can do without the "dl-start-unique" notifications and such. In fact, listening to "dl-start" is obsoleted (though still in use ) and should be replaced with proper listeners when per-window PBM support is implemented. @@ +3002,5 @@ > > NS_IMETHODIMP > nsDownload::GetId(uint32_t *aId) > { > + MOZ_ASSERT(!mPrivate); I _guess_ this should not be an assert, since this property can be called from third-party code. In any case we may want to use the ID of a private download, if we work with global PBM mode still enabled. ::: toolkit/components/downloads/test/unit/test_memory_db_support.js @@ -2,5 @@ > - * License, v. 2.0. If a copy of the MPL was not distributed with this > - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > - > -// This tests the switching of the download manager database types between disk > -// and memory based databases. This feature was added in bug 457110. Again, we might want to keep this test until global PBM is removed (though we would only change which database is returned to the consumers by the DBConnection property).

Comment on attachment 674289 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Information up front: * Fennec wants to ship PB mode in 19, which requires the download manager changes * This patch is ~40kb lighter and requires very few test changes - I've reworked it so that there should be absolutely no behavioural differences for existing consumers. See the uses of the mPrivateBrowsingCompatMode variable for more information. * The download manager changes are often quite similar to previous patch, so here's the summary of what to look for: - functions that deal with integer IDs check for the presence of the global PB mode and operate on the corresponding database - similarly for the direct database accessors, active download list, etc. - with the compat mode enabled, the download removed notification will pass integer IDs. When it's disabled, it passes GUIDs instead. - there's no differentiation between kinds of download listeners. It's assumed that if a consumer knows enough to use AddPrivacyAwareListener, we can disable the compat mode. That should cover it, I think.

https://tbpl.mozilla.org/?tree=Try&rev=0d25df5c03bf which should actually build this time.

Comment on attachment 674289 [details] [diff] [review] Support concurrent private/public downloads in the download manager. This approach creates inconsistencies that get us into a situation where we have different interfaces depending on whether we have per-window PBM support enabled in the application, or even worse changing the interface when we use a per-window PBM function at least once (by calling AddPrivacyAwareListener). What we should do instead: * If the host application is built with global PBM: - Existing functions and notifications continue to work as they do now. Access by GUID is optional. Access by ID works on private or public downloads depending on the current global mode. - In addition, downloads are categorized with the mPrivate attribute. - Privacy aware listeners receive the same notifications as the normal ones. * If the host application is built with per-window PBM: - Existing functions can work on public or private downloads, but access by GUID is always needed to work on private downloads. Access by ID works on public downloads only. - Existing notifications only deal with public downloads. - Privacy aware listeners receive notifications for all the downloads. So, if an add-on isn't updated, it just continues to work on top of the existing API, seeing public downloads only. Both old add-ons and add-ons updated to be aware of private downloads can exist at the same time. I know that Mobile starts with no current PBM support at all, so it's easy to fall in the trap of designing a different Toolkit interface to start with, but this will make it hard to reconcile with Desktop later when we add per-window PBM there also.

Thanks Paolo, that was very good feedback and it makes sense. I have made the changes necessary to implement the design you describe, which did not require much extra work. Marco, should I attach the updated patch or wait until you review the existing one?

If you addressed paolo's feedback already I prefer working on an up-to-date patch than an old version

Comment on attachment 678315 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Review of attachment 678315 [details] [diff] [review]: ----------------------------------------------------------------- So, this is lots of changes. Globally it looks sane, so I don't have blocking comments. Though due to its size I prefer getting another pass from Paolo, and then will just do a quick final check. Sorry for bouncing but so many changes are a bit scary to just read once and sign-off ::: browser/base/content/sanitize.js @@ +276,5 @@ > > // Queue up any active downloads that started in the time span as well > + var dls = [dlMgr.activeDownloads, dlMgr.activePrivateDownloads]; > + for (var idx in dls) { > + var dlsEnum = dls[idx]; for (let dlsEnum of [dlMgr.activeDownloads, dlMgr.activePrivateDownloads]) (I wonder why someone in the past wanted to use so cryptic var names...) @@ +279,5 @@ > + for (var idx in dls) { > + var dlsEnum = dls[idx]; > + while(dlsEnum.hasMoreElements()) { > + var dl = dlsEnum.next(); > + if(dl.startTime >= this.range[0]) missing whitespace after while and if @@ +292,5 @@ > > // Queue up all active ones as well > + var dls = [dlMgr.activeDownloads, dlMgr.activePrivateDownloads]; > + for (var idx in dls) { > + var dlsEnum = dls[idx]; for (let dlsEnum of [dlMgr.activeDownloads, dlMgr.activePrivateDownloads]) @@ +293,5 @@ > // Queue up all active ones as well > + var dls = [dlMgr.activeDownloads, dlMgr.activePrivateDownloads]; > + for (var idx in dls) { > + var dlsEnum = dls[idx]; > + while(dlsEnum.hasMoreElements()) { missing space @@ +303,2 @@ > // Remove any queued up active downloads > + dlsToRemove.forEach(function(dl) { add space after anonymous "function" ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +267,2 @@ > hasMore) { > + nsCString downloadGuid; This can arguably be an autostring due to its size @@ +369,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + rv = InitStatements(mPrivateDBConn, getter_AddRefs(mUpdatePrivateDownloadStatement), > + getter_AddRefs(mGetPrivateIdsForURIStatement)); > + NS_ENSURE_SUCCESS(rv, rv); ugh, this statements code sucks, there is clearly the need to swap this module to use a StatementCache, but should be done apart. I don't want to slow down this effort by requesting to make internal statements management proper, so could you please file a bug in toolkit/download manager to make use of StatementCache and remove all the crazy cached statements properties and initialization from this component? Just to clarify, a StatementCache is an helper object that lazy initializes statements, keeps them alive, and finalizes them, so you get lazy init for free, and just need 1 object per connection. It's some basic refactoring. @@ +888,5 @@ > > nsresult > nsDownloadManager::InitDB() > { > + nsresult rv = InitMemoryDB(); There starts to be some confusion between "memory" and "private", I think we should swap everything to use "private", the fact it's a memory database is a mere implementation detail at this point @@ +1077,5 @@ > rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("guid"), aGUID); > NS_ENSURE_SUCCESS(rv, rv); > + > + rv = GetDownloadFromDB(mDBConn, stmt, retVal); > + if (rv == NS_ERROR_NOT_AVAILABLE) { These changes definitely need some better comments about what's happening and why, I'd like people looking at this to figure out the big shape without having to rely on the bug or having to read the whole code... @@ +1095,5 @@ > > nsresult > nsDownloadManager::GetDownloadFromDB(PRUint32 aID, nsDownload **retVal) > { > + NS_ASSERTION(mPrivateBrowsingCompatMode, "Using integer IDs without compat mode enabled"); why is this a non-aborting assertion (MOZ_ASSERT)? @@ +1671,5 @@ > > NS_IMETHODIMP > nsDownloadManager::GetDownload(uint32_t aID, nsIDownload **aDownloadItem) > { > + NS_ASSERTION(mPrivateBrowsingCompatMode, "Using integer IDs without compat mode enabled"); why is this a non-aborting assertion (MOZ_ASSERT)? not going to repeat, as a rule of thumb we want all assertions to be aborting, since it's the only way we notice them. @@ +1916,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + return mObserverService->NotifyObservers(id, > + "download-manager-remove-download", > + nullptr); just for readability, I'd like a visible return NS_OK; to clarify this is an early bailout (I actually missed the return on first reading this) Also, I thought we were notifying old ids only for non-private downloads, this seems to notify for both, how can a legacy listener distinguish private id from non-private id since these are going to overlap? (maybe I just missed some of the previous discussions?) @@ +1938,5 @@ > +{ > + NS_ASSERTION(mPrivateBrowsingCompatMode, "Using integer IDs without compat mode enabled"); > + > + nsDownload *dl = FindDownload(aID); > + NS_ASSERTION(!dl, "Can't call RemoveDownload on a download in progress!"); please convert this to an aborting MOZ_ASSERT as well @@ +2007,5 @@ > +nsDownloadManager::RemoveDownloadsByTimeframe(int64_t aStartTime, > + int64_t aEndTime) > +{ > + ::RemoveDownloadsByTimeframe(mDBConn, aStartTime, aEndTime); > + ::RemoveDownloadsByTimeframe(mPrivateDBConn, aStartTime, aEndTime); I think the coding style here is to prepend :: only for system or third party libraries, not for local helpers Btw, why is this now ignoring errors? afaict it was not before @@ +2109,5 @@ > > NS_IMETHODIMP > +nsDownloadManager::GetCanCleanUp(bool *aResult) > +{ > + return ::GetCanCleanUp(mDBConn, aResult); ditto on coding style @@ +2115,5 @@ > + > +NS_IMETHODIMP > +nsDownloadManager::GetCanCleanUpPrivate(bool *aResult) > +{ > + return ::GetCanCleanUp(mPrivateDBConn, aResult); as well as here @@ +2187,3 @@ > { > + for (int32_t i = mPrivacyAwareListeners.Count() - 1; i >= 0; --i) > + mPrivacyAwareListeners[i]->OnDownloadStateChange(aOldState, aDownload); please always brace loops @@ +2194,2 @@ > for (int32_t i = mListeners.Count() - 1; i >= 0; --i) > mListeners[i]->OnDownloadStateChange(aOldState, aDownload); ditto, while here :) A comment above explaining that early return would be appreciated @@ +2206,5 @@ > { > + for (int32_t i = mPrivacyAwareListeners.Count() - 1; i >= 0; --i) > + mPrivacyAwareListeners[i]->OnProgressChange(aProgress, aRequest, aCurSelfProgress, > + aMaxSelfProgress, aCurTotalProgress, > + aMaxTotalProgress, aDownload); especially bad with multiline :) @@ +2215,4 @@ > for (int32_t i = mListeners.Count() - 1; i >= 0; --i) > mListeners[i]->OnProgressChange(aProgress, aRequest, aCurSelfProgress, > aMaxSelfProgress, aCurTotalProgress, > aMaxTotalProgress, aDownload); and here @@ +2227,4 @@ > { > + for (int32_t i = mPrivacyAwareListeners.Count() - 1; i >= 0; --i) > + mPrivacyAwareListeners[i]->OnStateChange(aProgress, aRequest, aStateFlags, aStatus, > + aDownload); and here @@ +2235,3 @@ > for (int32_t i = mListeners.Count() - 1; i >= 0; --i) > mListeners[i]->OnStateChange(aProgress, aRequest, aStateFlags, aStatus, > aDownload); and here @@ +2343,1 @@ > currDownloadCount--; also brace the loop please @@ +2411,3 @@ > else if (strcmp(aTopic, "alertclickcallback") == 0) { > + //XXXjdm This won't make sense when clicking a notification related to > + // private downloads when per-window mode is enabled. XXX comments are bad, please file a bug and report the bug number here. @@ +2436,5 @@ > + RemoveAllDownloads(mCurrentPrivateDownloads); > + InitMemoryDB(); > + } else if (strcmp(aTopic, "last-pb-context-exiting") == 0) { > + if (!mCurrentPrivateDownloads.Count()) > + return NS_OK; please add some comments on what's up and the expected behavior for these 2 notifications. @@ +2440,5 @@ > + return NS_OK; > + > + nsCOMPtr<nsISupportsPRBool> cancelDownloads = do_QueryInterface(aSubject, &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + //TODO: When enabling per-window mode, change strings to be sensible Please file a bug and report bug # here. nit: missing space after // ::: toolkit/components/downloads/nsIDownload.idl @@ +87,5 @@ > */ > readonly attribute nsIMIMEInfo MIMEInfo; > > /** > + * The id of the download that is stored in the database - not globally unique. The cryptic addition doesn't tell much, it adds confusion probably... It should state when to (and when not to) rely on this. ::: toolkit/components/downloads/nsIDownloadManager.idl @@ +271,5 @@ > > /** > + * Adds a listener to the download manager. > + */ > + void addPrivacyAwareListener(in nsIDownloadProgressListener aListener); Should better explain the difference between privacy aware listener and common ones. ::: toolkit/forgetaboutsite/ForgetAboutSite.jsm @@ +102,5 @@ > getService(Ci.nsIDownloadManager)) { > // Active downloads > + let dls = [dm.activeDownloads, dm.activePrivateDownloads]; > + for (var idx in dls) { > + let enumerator = dls[idx]; for (let of)

>@@ +1095,5 @@ >> >> nsresult >> nsDownloadManager::GetDownloadFromDB(PRUint32 aID, nsDownload **retVal) >> { >> + NS_ASSERTION(mPrivateBrowsingCompatMode, "Using integer IDs without compat mode enabled"); > >why is this a non-aborting assertion (MOZ_ASSERT)? > >@@ +1671,5 @@ >> >> NS_IMETHODIMP >> nsDownloadManager::GetDownload(uint32_t aID, nsIDownload **aDownloadItem) >> { >> + NS_ASSERTION(mPrivateBrowsingCompatMode, "Using integer IDs without compat mode enabled"); > >why is this a non-aborting assertion (MOZ_ASSERT)? >not going to repeat, as a rule of thumb we want all assertions to be aborting, since it's the >only way we notice them. I made these non-aborting at the special request of Paolo, since it's quite easy to imagine them being triggered by download-related addons that are not immediately updated after the introduction of per-window PB.

>Also, I thought we were notifying old ids only for non-private downloads, this seems to notify >for both, how can a legacy listener distinguish private id from non-private id since these are >going to overlap? (maybe I just missed some of the previous discussions?) In the case of RemoveDownload where we use the observer service, we're in a bind. The code as written will use integer IDs for as long as we use the compat mode (ie. when we land per-window support but don't actually have per-window mode enabled in Firefox) to allow the existing download manager to continue functioning without requiring any modifications (ie. it will see notifications for both private and public downloads, with integer IDs for both). When per-window mode is enabled in Firefox, the compat mode is turned off and all observer notifications pass GUIDs instead - we can't restrict this to only public downloads or anything like that, since observer notifications are global, so we try to limit the damage by passing a GUID which won't be understood by unmodified consumers.

I've addressed Marco's comments with the exception of the assertions.

Also note that the bugzilla interdiff appears to work well for this latest patch.

(In reply to Josh Matthews [:jdm] from comment #46) > I made these non-aborting at the special request of Paolo, since it's quite > easy to imagine them being triggered by download-related addons that are not > immediately updated after the introduction of per-window PB. I wonder how many add-on developers work on debug builds. I know they should but I'm not really optimist. Btw Paolo is far more expert on that, so I trust his choice. (In reply to Josh Matthews [:jdm] from comment #47) > In the case of RemoveDownload where we use the observer service, we're in a > bind. The code as written will use integer IDs for as long as we use the > compat mode (ie. when we land per-window support but don't actually have > per-window mode enabled in Firefox) to allow the existing download manager > to continue functioning without requiring any modifications (ie. it will see > notifications for both private and public downloads, with integer IDs for > both) May you improve commenting in that area of the code then, to make ti clear why we do this as you did here.

Comment on attachment 680014 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Review of attachment 680014 [details] [diff] [review]: ----------------------------------------------------------------- Some changes should still be made, but they're very well defined - so this is r+ for me with those addressed. Thank you for the very good work here, and for your patience! ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +1744,5 @@ > +nsDownloadManager::IsInGlobalPrivateBrowsing() > +{ > + bool inPrivateBrowsing = false; > +#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > + if (mPrivateBrowsingCompatMode) { I find it quite confusing that there are both MOZ_PER_WINDOW_PRIVATE_BROWSING and mPrivateBrowsingCompatMode, there's even a redundancy here. Please remove mPrivateBrowsingCompatMode, and use only #ifdef/#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING throughout the file. @@ +1793,5 @@ > > NS_IMETHODIMP > nsDownloadManager::CancelDownload(uint32_t aID) > { > + NS_ASSERTION(mPrivateBrowsingCompatMode, "Using integer IDs without compat mode enabled"); Well, I don't remember mentioning we should use assertions here :-) But, an #ifdef'd NS_WARNING here might be fine, since the ID-based API will automatically become deprecated when per-window PBM is enabled. Marco is the expert on the deprecation strategies - let us know if you have any better suggestion, otherwise an NS_WARNING in debug builds is just fine for me. (This change is global for all the other ID-based entry points.) @@ +1916,5 @@ > + RemoveDownloadByGUID(aGUID, mDBConn); > + } > + > + // Notify the UI with the topic and id to retain compatibility with clients > + // that don't understand GUIDs. This part should be changed to use the same logic as the PrivacyAwareListeners - "download-manager-remove-download" should only ever be invoked with an ID (i.e. for public downloads, and for private downloads only if per-window private browsing is disabled). Another notification ("download-manager-remove-download-guid" or similar name) should be sent for all download types. Ideally, instead of a global observer notification this would be a dedicated method on the registered PrivacyAwareListeners. I'm not sure how hard it is to update the callers with this change. If it's not too difficult, let's do it, otherwise let's go for the new observer notification. @@ +1945,5 @@ > +} > + > +NS_IMETHODIMP > +nsDownloadManager::RemoveDownload(uint32_t aID) > +{ And, we should send all the removal notifications consistently, for both IDs and GUIDs, regardless of whether the caller used the ID-based API or the GUID-based API for making the change. @@ +2372,5 @@ > // If we don't need to cancel all the downloads on quit, only count the ones > // that aren't resumable. > + if (GetQuitBehavior() != QUIT_AND_CANCEL) { > + for (int32_t i = currDownloadCount - 1; i >= 0; --i) { > + if (currDownloads[i]->IsResumable() && !currDownloads[i]->mPrivate) { So, !currDownloads[i]->mPrivate is either always true or always false throughout the loop, right? I think it's clearer as an outer condition. @@ +2379,5 @@ > + } > + > +#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING > + // This won't double count any downloads, since we default to the > + // number of public downloads when per-window is enabled. We need a better overall comment about what currDownloadCount ends up containing, for both possible MOZ_PER_WINDOW_PRIVATE_BROWSING cases (it's used in the quit notification, right?).

(In reply to Paolo Amadini [:paolo] from comment #51) > ::: toolkit/components/downloads/nsDownloadManager.cpp > @@ +1744,5 @@ > > +nsDownloadManager::IsInGlobalPrivateBrowsing() > > +{ > > + bool inPrivateBrowsing = false; > > +#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > > + if (mPrivateBrowsingCompatMode) { > > I find it quite confusing that there are both > MOZ_PER_WINDOW_PRIVATE_BROWSING and mPrivateBrowsingCompatMode, there's even > a redundancy here. > > Please remove mPrivateBrowsingCompatMode, and use only #ifdef/#ifndef > MOZ_PER_WINDOW_PRIVATE_BROWSING throughout the file. Unless you absolutely insist upon this, I would not like to make this change. I think it is clearer to use a member variable when there are this many places in which the behaviour can differ, because #ifdef blocks are harder to reason about (especially when some are ifdef and some are ifndef). > @@ +1916,5 @@ > > + RemoveDownloadByGUID(aGUID, mDBConn); > > + } > > + > > + // Notify the UI with the topic and id to retain compatibility with clients > > + // that don't understand GUIDs. > > This part should be changed to use the same logic as the > PrivacyAwareListeners - "download-manager-remove-download" should only ever > be invoked with an ID (i.e. for public downloads, and for private downloads > only if per-window private browsing is disabled). > > Another notification ("download-manager-remove-download-guid" or similar > name) should be sent for all download types. Ideally, instead of a global > observer notification this would be a dedicated method on the registered > PrivacyAwareListeners. I'm not sure how hard it is to update the callers > with this change. If it's not too difficult, let's do it, otherwise let's go > for the new observer notification. I guess you're suggesting adding a method to nsIDownloadListener? I would prefer to not do that at this point, since it is an uncertain amount of work required to update the callers (digging into more code I have never looked at before). I will add the new observer notification instead.

https://tbpl.mozilla.org/?tree=Try&rev=8bb3dc74689d I'll post the updated patch once some greens start appearing.

Now watching https://tbpl.mozilla.org/?tree=Try&rev=0ddae3b282a3 instead.

(In reply to Josh Matthews [:jdm] from comment #52) > (In reply to Paolo Amadini [:paolo] from comment #51) > > ::: toolkit/components/downloads/nsDownloadManager.cpp > > @@ +1744,5 @@ > > > +nsDownloadManager::IsInGlobalPrivateBrowsing() > > > +{ > > > + bool inPrivateBrowsing = false; > > > +#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > > > + if (mPrivateBrowsingCompatMode) { > > > > I find it quite confusing that there are both > > MOZ_PER_WINDOW_PRIVATE_BROWSING and mPrivateBrowsingCompatMode, there's even > > a redundancy here. > > > > Please remove mPrivateBrowsingCompatMode, and use only #ifdef/#ifndef > > MOZ_PER_WINDOW_PRIVATE_BROWSING throughout the file. > > Unless you absolutely insist upon this, I would not like to make this > change. I think it is clearer to use a member variable when there are this > many places in which the behaviour can differ, because #ifdef blocks are > harder to reason about (especially when some are ifdef and some are ifndef). If you're concerned about being able to use a clearer syntax, it's fine for me to define a constant (it should just be clear that it cannot change during program execution). In any case it should have a more specific name (like "use per window private browsing") and the same value as MOZ_PER_WINDOW_PRIVATE_BROWSING (not inverted). That seems like a small change but really makes things simpler for people reading the code, it was a bit difficult for me to follow the logic without looking at how mCompat was initialized (and I had to check again a few times just to be sure when reading some passages, like the one I mentioned). > I guess you're suggesting adding a method to nsIDownloadListener? I would > prefer to not do that at this point, since it is an uncertain amount of work > required to update the callers (digging into more code I have never looked > at before). I will add the new observer notification instead. That's OK, you know the effort and goals better than me here.

(In reply to Paolo Amadini [:paolo] from comment #55) > (In reply to Josh Matthews [:jdm] from comment #52) > > (In reply to Paolo Amadini [:paolo] from comment #51) > > > ::: toolkit/components/downloads/nsDownloadManager.cpp > > > @@ +1744,5 @@ > > > > +nsDownloadManager::IsInGlobalPrivateBrowsing() > > > > +{ > > > > + bool inPrivateBrowsing = false; > > > > +#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > > > > + if (mPrivateBrowsingCompatMode) { > > > > > > I find it quite confusing that there are both > > > MOZ_PER_WINDOW_PRIVATE_BROWSING and mPrivateBrowsingCompatMode, there's even > > > a redundancy here. > > > > > > Please remove mPrivateBrowsingCompatMode, and use only #ifdef/#ifndef > > > MOZ_PER_WINDOW_PRIVATE_BROWSING throughout the file. > > > > Unless you absolutely insist upon this, I would not like to make this > > change. I think it is clearer to use a member variable when there are this > > many places in which the behaviour can differ, because #ifdef blocks are > > harder to reason about (especially when some are ifdef and some are ifndef). > > If you're concerned about being able to use a clearer syntax, it's fine for > me > to define a constant (it should just be clear that it cannot change during > program execution). In any case it should have a more specific name (like > "use per window private browsing") and the same value as > MOZ_PER_WINDOW_PRIVATE_BROWSING (not inverted). > > That seems like a small change but really makes things simpler for people > reading the code, it was a bit difficult for me to follow the logic without > looking at how mCompat was initialized (and I had to check again a few times > just to be sure when reading some passages, like the one I mentioned). This is a good idea. I made the change, and I think it's much clearer what is intended now. I'm running the patch through try once more, and then I'll send it to Marco. I also consolidated all of the download removal notification code into one function, so all download removals should now trigger the appropriate notifications.

Try is green except for one mistake in a test which I've corrected.

https://tbpl.mozilla.org/?tree=Try&rev=8b7910efe416 is the try run, for future reference.

Comment on attachment 681026 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Stephen, would you like to sign off on the new text for the warning dialog that pops up if you try to leave private browsing while downloads are still in progress?

Comment on attachment 681026 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Review of attachment 681026 [details] [diff] [review]: ----------------------------------------------------------------- This needs a SR. ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +1116,3 @@ > { > + if (gUsePerWindowPrivateBrowsing) > + NS_WARNING("Using integer IDs without compat mode enabled"); It's usually dangerous to if () SOME_DEBUG_MACRO cause the macro may end up being /* nothing */ and your if will then apply to the next operation... Not this case since luckily NS_WARNING uses do {} while (0), but still dangerous habit. We have NS_WARN_IF_FALSE for this. Though with this condition it may be unreadable (double negation) and we lack NS_WARN_IF_TRUE. Either leave as is (since this is luckily working) or invert the condition as NS_WARN_IF_FALSE(gUseGlobalPrivateBrowsing). I feel like both approaches have pro and cons. This happens multiple times. @@ +1295,5 @@ > //////////////////////////////////////////////////////////////////////////////// > //// nsIDownloadManager > > NS_IMETHODIMP > +nsDownloadManager::GetActivePrivateDownloadCount(PRInt32* aResult) no PR types please @@ +1790,5 @@ > if (dl->mGUID == aGUID) > return dl; > } > > + for (PRInt32 i = mCurrentPrivateDownloads.Count() - 1; i >= 0; --i) { no PR types please @@ +2004,5 @@ > + } > + > + mObserverService->NotifyObservers(guid, > + "download-manager-remove-download-guid", > + nullptr); nsIDownloadManager.idl must be updated to tell we are always sending this notification, currently it just speaks of download-manager-remove-download (and if we consider download-manager-remove-download deprecated the idl should also report that) @@ +2512,5 @@ > + // and recreate a blank one. > + PauseAllDownloads(mCurrentPrivateDownloads, true); > + RemoveAllDownloads(mCurrentPrivateDownloads); > + InitPrivateDB(); > + } else if (strcmp(aTopic, "last-pb-context-exiting") == 0) { nit: I honestly prefer if you add a newline before else if, than after it. as it is now totally breaks my code reading capability (I'm visually associating the else if to the previous code...) ::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties @@ +36,5 @@ > leavePrivateBrowsingCancelDownloadsAlertTitle=Cancel All Downloads? > leavePrivateBrowsingCancelDownloadsAlertMsg=If you leave the Private Browsing mode now, 1 download will be canceled. Are you sure you want to leave the Private Browsing mode? > leavePrivateBrowsingCancelDownloadsAlertMsgMultiple=If you leave the Private Browsing mode now, %S downloads will be canceled. Are you sure you want to leave the Private Browsing mode? > +leavePrivateBrowsingWindowsCancelDownloadsAlertMsg=If you close all Private Browsing windows now, 1 download will be canceled. Are you sure you want to leave the Private Browsing mode? > +leavePrivateBrowsingWindowsCancelDownloadsAlertMsgMultiple=If you close all Private Browsing windows now, %S downloads will be canceled. Are you sure you want to leave the Private Browsing mode? ignoreme-whine: sigh :( bug 463102 Could you please add a LOCALIZATION NOTE above all of the PB strings that points to the fact we don't have proper plural forms support in cpp and the bug number above?

Comment on attachment 681026 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Boris, would you mind reviewing the API changes?

Comment on attachment 681026 [details] [diff] [review] Support concurrent private/public downloads in the download manager. The IDL could use some more comments about two things: 1) What the "private" versions of the methods mean. 2) Whether the "non-private" versions operate on all downloads or only on non-private downloads. sr=me with that.

Comment on attachment 681026 [details] [diff] [review] Support concurrent private/public downloads in the download manager. Review of attachment 681026 [details] [diff] [review]: ----------------------------------------------------------------- Strings look good to me.

Backed out for compilation failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=00cc23d1f72b Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/00cc23d1f72b

(In reply to Ed Morley [:edmorley UTC+0] from comment #66) > Backout: > https://hg.mozilla.org/integration/mozilla-inbound/rev/00cc23d1f72b Bah, copy paste is hard https://hg.mozilla.org/integration/mozilla-inbound/rev/dad2675c03d1

Relanded in a buildable state: https://hg.mozilla.org/integration/mozilla-inbound/rev/f077ad362ca3

Unfortunately burning still, backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/df662e61a310

Today is totally not my day: https://hg.mozilla.org/integration/mozilla-inbound/rev/b72fbbbfc671