We need to support per-window private browsing mode in the new downloads panel. The use case that we want to support is rather simple: * Never add downloads initiated in a private window anywhere else (not in the Library window, nor in the downloads panel for other private windows * Always keep the downloads initiated in a private window visible in the download panel belonging to that window. That is, we should never set a maximum number of downloads to show in the private window's download panel, because there is nowhere else for that download to go. Hint: to get a per-window PB build (still in the experimental stages), add "export MOZ_PER_WINDOW_PRIVATE_BROWSING=1" in your .mozconfig (or add that variable with a value of 1 to browser/confvars.sh). Mike, do you think you're going to have the bandwidth to work on this?

(In reply to Ehsan Akhgari [:ehsan] from comment #0) > Mike, do you think you're going to have the bandwidth to work on this? I spoke to Josh about this last Friday. If you folks are targeting per-window PB for FF 20, then yes, I think I can handle this. It's more of an issue if you're going to try to get this going for FF 19, since I'm still trying to get Downloads Panel to spec for the non-PB case. And some little details still need to be nailed down for the spec, so the amount of work here is hard to estimate. So, if you're targeting FF 20, then I think I can handle it. -Mike

It turns out that I'm going to be targeting the backend work for 19, since mobile uses the download manager service.

(In reply to Josh Matthews [:jdm] from comment #2) > It turns out that I'm going to be targeting the backend work for 19, since > mobile uses the download manager service. When are you targeting to ship PB-per-window on Desktop?

(The whole thing - back-end, front-end... basically, user-ready).

Ehsan wants me to not rule out 19.

Ok, good to know. I'm meeting today with a few folks to nail down the rest of the Download Panel spec. I'll bring this up, and see where we get. -Mike

(In reply to comment #6) > Ok, good to know. > > I'm meeting today with a few folks to nail down the rest of the Download Panel > spec. I'll bring this up, and see where we get. Cool, please let us know what people thought. We can also get help from the appcoast folks, but I'm not sure if the current state of the panel is useful in order for them to start building on top of.

So the reasoning behind the user interaction that I described in comment 0 is the following: * I don't think that it's a good idea to mix your private and non-private downloads in the same view. It would be extremely confusing to the user if we did that, so I think we should avoid doing that. * I expect that once we ship the downloads panel, most users won't need to go to the Library view very often since most of the time, the stuff that you download recently is the most interesting stuff. Specifically in the case of private browsing windows, I think the majority of the time those windows will be rather short-lived compared to regular windows (the idea is that you open a window, shop for your engagement rings, and then you close it because you don't want your soon to be fiance to know about the engagement. ;-) Therefore, I don't think that it's worth spending a bunch of time designing a downloads view for the private downloads that is capable of tracking the history of all of your private downloads. * It's true that the downloads panel is designed as a temporary place for you to be notified about your downloads, and my idea about not removing any private downloads from the downloads panel view poses a challenge to that. So we need to find a way to fix it. I was thinking that maybe we can use a scrollable view of the downloads in that case, but Mike tells me that doing so might be challenging in terms of the design and implementation of the panel. What are the other options that we have? I guess keeping track of at most 3 of your private downloads per window could be one option, but that causes you to "lose" the first download if you ever download a fourth file. Is that going to be an acceptable compromise? If not, what other options do we have?

AFAIK Google Chrome shows two separate downloads views : one for normal session and on for private one. There is an icon (the guy with the hat) in the private mode next to each download launched in this mode.

(In reply to comment #9) > AFAIK Google Chrome shows two separate downloads views : one for normal session > and on for private one. There is an icon (the guy with the hat) in the private > mode next to each download launched in this mode. Hmm, right, but they only show those downloads if you open chrome://downloads in a private window. Our Library window is not associated with any browser window, so I don't think the same model necessarily makes sense for Firefox.

ehsan, shorlander and I were discussing the possibility of perhaps moving the Library window into a content tab, and only displaying pb-window downloads within pb-window Library tabs. I talked to fryn about this, who already had a patch started on this (bug 697359). He says that moving the Library into a content tab is trivial - the harder part is hooking up the transaction and undo manager and the history state manager to the HTML stuff.

(In reply to comment #11) > He says that moving the Library into a content tab is trivial - the harder part > is hooking up the transaction and undo manager and the history state manager to > the HTML stuff. What does that mean, exactly?

(In reply to Ehsan Akhgari [:ehsan] from comment #12) > (In reply to comment #11) > > He says that moving the Library into a content tab is trivial - the harder part > > is hooking up the transaction and undo manager and the history state manager to > > the HTML stuff. > What does that mean, exactly? I interpreted that as meaning that those managers will not work while we're in content, but I'll let him elaborate, in case I'm mistaken.

(In reply to comment #13) > (In reply to Ehsan Akhgari [:ehsan] from comment #12) > > (In reply to comment #11) > > > He says that moving the Library into a content tab is trivial - the harder part > > > is hooking up the transaction and undo manager and the history state manager to > > > the HTML stuff. > > What does that mean, exactly? > > I interpreted that as meaning that those managers will not work while we're in > content, but I'll let him elaborate, in case I'm mistaken. Hmm, OK. Can we do something a bit crazy here? Can we only show the Downloads panel if that tab is opened inside a PB window? That _could_ solve all of our concerns, it seems.

Hm - so Ehsan and I just discussed this, and here's what he's proposing: 1) Normal window's will view their Downloads using the current Library window with the up-and-coming Downloads item, as originally planned. 2) When private-browing windows view their downloads, instead of a new window opening, a tab is opened that shows *only* the downloaded items - basically, the Library window in a tab, but without the sidepanel or toolbar. What do you think of that, Stephen?

how does the current DM act on PB activation?

ah right, it has the temp table thing... So, at least Mano should be notified of the plan since may change some of the architectural choices in the Library view. (what, there's no way to set more than one needinfo?)

Just talked to shorlander - he's on board with this. Mano - does what we're describing sound possible? Not necessarily for 19 (Ehsan believes per-window PB is likely not to land until at least 20).

This will need additional mockups. It's a tricky part I think.

I don't believe this is blocking the Downloads Panel now, since PB-per-window isn't going to land for 19.

(In reply to Mike Conley (:mconley) from comment #20) > I don't believe this is blocking the Downloads Panel now, since > PB-per-window isn't going to land for 19. Hmm, we still need the download panel as a dependency here, don't we?

We do indeed.

we surely need to fix it when we ship both in the same release.

(In reply to Mike Conley (:mconley) from comment #18) > Just talked to shorlander - he's on board with this. > > Mano - does what we're describing sound possible? Not necessarily for 19 > (Ehsan believes per-window PB is likely not to land until at least 20). Mike, did you hear back from Mano about this?

(In reply to Ehsan Akhgari [:ehsan] from comment #24) > (In reply to Mike Conley (:mconley) from comment #18) > > Just talked to shorlander - he's on board with this. > > > > Mano - does what we're describing sound possible? Not necessarily for 19 > > (Ehsan believes per-window PB is likely not to land until at least 20). > > Mike, did you hear back from Mano about this? I don't want to speak for Mano here, but I do believe bug 675902 must land before this work can be done. It's pretty close, by my understanding.

So I have a number of patches for this bug. I couldn't actually test them locally since no matter what I do, I can't get my local builds to use the downloads panel. I'll ask help from Mike on this tomorrow.

This version of the patch actually builds on the tinderboxen.

Comment on attachment 685461 [details] [diff] [review] Part 1: Break up the DownloadsCommon.data singleton into two objects, one for normal and one for private downloads Review of attachment 685461 [details] [diff] [review]: ----------------------------------------------------------------- The global approach looks what was previously discussed and looks good. I'm still skimming through these many patches, so just feedbacking here and there for now, want to take a second look later. I'm flagging paolo in the meanwhile, more eyes are always welcome. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +196,2 @@ > */ > + get privateDownloadsData() PrivateDownloadsData, At first glance, I'd prefer if getData would be the only exposed way to get these out of the module scope. Shouldn't be too hard, the only change in this patch that looks problematic is DownloadsStartup, for which we may add .initializeAllDataLinks, terminateAllDataLinks, ensureAllPersistentDataLoaded to DownloadsCommon ::: browser/components/downloads/src/DownloadsStartup.js @@ +114,5 @@ > // from this observer function. We can't defer the execution of this > // step, to ensure that we don't lose events raised in the meantime. > + DownloadsCommon.downloadsData.initializeDataLink( > + aSubject.QueryInterface(Ci.nsIDownloadManager)); > + DownloadsCommon.privateDownloadsData.initializeDataLink(aSubject); no QI for privateDownloadsData? ::: browser/components/downloads/test/browser/browser_first_download_panel.js @@ +16,4 @@ > > // With this set to false, we should automatically open the panel > // the first time a download is started. > + DownloadsCommon.downloadsData.panelHasShownBefore = false; the test may just use .getData(window) ::: browser/components/downloads/test/browser/head.js @@ +222,5 @@ > }; > > // Start loading all the downloads from the database asynchronously. > + DownloadsCommon.downloadsData.ensurePersistentDataLoaded(false); > + DownloadsCommon.privateDownloadsData.ensurePersistentDataLoaded(false); ensurePersistentDataLoaded javadoc states "This method must only be called when Private Browsing Mode is disabled.", that javadoc should probably change. Btw, I don't think this generator may be used by a private window as is, but I see gen_openPanel has an aData argument that has not been used, we may use it to pass DownloadsCommon.getData(window) from where it's needed and use aData inside it... And the same could be done (adding aData) to gen_resetState

regular build: https://tbpl.mozilla.org/?tree=Try&rev=25f43dca4303 per-window PB build: https://tbpl.mozilla.org/?tree=Try&rev=f030e2639e22

Comment on attachment 685463 [details] [diff] [review] Part 2: Replace DownloadsCommon.indicatorData with a privacy aware DownloadsCommon.getIndicatorData(window) Review of attachment 685463 [details] [diff] [review]: ----------------------------------------------------------------- this looks good afaict, I'd probably merge this part with part 1, they are somehow related and the slightly larger patch shouldn't hurt anyone.

(In reply to Marco Bonardo [:mak] from comment #35) > The global approach looks what was previously discussed and looks good. > I'm still skimming through these many patches, so just feedbacking here and > there for now, want to take a second look later. > I'm flagging paolo in the meanwhile, more eyes are always welcome. Thanks for doing this! > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +196,2 @@ > > */ > > + get privateDownloadsData() PrivateDownloadsData, > > At first glance, I'd prefer if getData would be the only exposed way to get > these out of the module scope. Shouldn't be too hard, the only change in > this patch that looks problematic is DownloadsStartup, for which we may add > .initializeAllDataLinks, terminateAllDataLinks, > ensureAllPersistentDataLoaded to DownloadsCommon OK, I can do that. > ::: browser/components/downloads/src/DownloadsStartup.js > @@ +114,5 @@ > > // from this observer function. We can't defer the execution of this > > // step, to ensure that we don't lose events raised in the meantime. > > + DownloadsCommon.downloadsData.initializeDataLink( > > + aSubject.QueryInterface(Ci.nsIDownloadManager)); > > + DownloadsCommon.privateDownloadsData.initializeDataLink(aSubject); > > no QI for privateDownloadsData? Well, we QIed once, so now XPConnect knows about nsIDownloadManager on that object. QIing the second time will just waste time. :-) > ::: browser/components/downloads/test/browser/browser_first_download_panel.js > @@ +16,4 @@ > > > > // With this set to false, we should automatically open the panel > > // the first time a download is started. > > + DownloadsCommon.downloadsData.panelHasShownBefore = false; > > the test may just use .getData(window) Hmm, right. > ::: browser/components/downloads/test/browser/head.js > @@ +222,5 @@ > > }; > > > > // Start loading all the downloads from the database asynchronously. > > + DownloadsCommon.downloadsData.ensurePersistentDataLoaded(false); > > + DownloadsCommon.privateDownloadsData.ensurePersistentDataLoaded(false); > > ensurePersistentDataLoaded javadoc states "This method must only be called > when Private Browsing Mode is disabled.", that javadoc should probably > change. OK. > Btw, I don't think this generator may be used by a private window as is, but > I see gen_openPanel has an aData argument that has not been used, we may use > it to pass DownloadsCommon.getData(window) from where it's needed and use > aData inside it... > And the same could be done (adding aData) to gen_resetState Not sure if I can parse this. :-)

> > Btw, I don't think this generator may be used by a private window as is, but > > I see gen_openPanel has an aData argument that has not been used, we may use > > it to pass DownloadsCommon.getData(window) from where it's needed and use > > aData inside it... > > And the same could be done (adding aData) to gen_resetState > > Not sure if I can parse this. :-) gen_openPanel and gen_resetState may get the data as an argument directly from the test (the former is already but looks like it's a leftover).

(In reply to comment #39) > > > Btw, I don't think this generator may be used by a private window as is, but > > > I see gen_openPanel has an aData argument that has not been used, we may use > > > it to pass DownloadsCommon.getData(window) from where it's needed and use > > > aData inside it... > > > And the same could be done (adding aData) to gen_resetState > > > > Not sure if I can parse this. :-) > > gen_openPanel and gen_resetState may get the data as an argument directly from > the test (the former is already but looks like it's a leftover). ok, I'll look into this more.

(In reply to comment #37) > Comment on attachment 685463 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=685463 > Part 2: Replace DownloadsCommon.indicatorData with a privacy aware > DownloadsCommon.getIndicatorData(window) > > Review of attachment 685463 [details] [diff] [review]: > ----------------------------------------------------------------- > > this looks good afaict, I'd probably merge this part with part 1, they are > somehow related and the slightly larger patch shouldn't hurt anyone. Sure, I can do that when landing. I just wanted to make reviewing this stuff as easy as possible.

Comment on attachment 685464 [details] [diff] [review] Part 3: Use a privacy aware listener for DownloadsData and PrivateDownloadsData Review of attachment 685464 [details] [diff] [review]: ----------------------------------------------------------------- I see guids are handled in the next part, we likely want to switch to guids as far as possible. Privacy aware listeners should use guids for basically anything, so we may need part 4 doing a deeper id -> guid conversion. for example "download-manager-remove-download" should become "download-manager-remove-download-guid" The only problematic case looks the .sort() in _updateView, we may keep using id and fix that in a follow-up (we have .startTime on the download but doesn't look like what we need here)

Comment on attachment 685465 [details] [diff] [review] Part 4: Stop using the deprecated getDownload API Review of attachment 685465 [details] [diff] [review]: ----------------------------------------------------------------- As said, we should try to move more to guids, this part has some issues to be fixed regardless of that too ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +739,5 @@ > + .getDownloadByGUID(dataItem.guid, function(status, result) { > + if (status != Components.results.NS_OK) { > + this._removeDataItem(dataItem.downloadId); > + } > + }.bind(this)); yeah, as said this should instead use the new download-manager-remove-download-guid notification @@ +985,5 @@ > this.maxBytes = aStorageRow.getResultByName("maxBytes"); > > + Services.downloads.getDownloadByGUID(this.guid, function(status, result) { > + this.download = result; > + }.bind(this)); So, this needs to be re-thought well, the lazyGetter existed for performance reasons (don't access the DB unless it's really needed) and this is basically killing that performance tweak... plus it's breaking this getter checking code http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#766 Though I don't have an answer atm... it may involve deep changes to how we access the .download property... Paolo may have some insight on that from when he designed this.

Comment on attachment 685660 [details] [diff] [review] Part 5: Don't use the global PB service in DownloadsStartup.js Review of attachment 685660 [details] [diff] [review]: ----------------------------------------------------------------- looks good

Comment on attachment 685468 [details] [diff] [review] Part 6: Don't use the global PB service in DownloadsCommon.jsm Review of attachment 685468 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +746,4 @@ > break; > > case "download-manager-database-type-changed": > + if (this._isPrivate) { This notification can only be fired #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING so the code should just be ifndef

Comment on attachment 685469 [details] [diff] [review] Part 7: Use privacy aware cleanup methods Review of attachment 685469 [details] [diff] [review]: ----------------------------------------------------------------- looks good

(In reply to comment #43) > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +739,5 @@ > > + .getDownloadByGUID(dataItem.guid, function(status, result) { > > + if (status != Components.results.NS_OK) { > > + this._removeDataItem(dataItem.downloadId); > > + } > > + }.bind(this)); > > yeah, as said this should instead use the new > download-manager-remove-download-guid notification Why? > @@ +985,5 @@ > > this.maxBytes = aStorageRow.getResultByName("maxBytes"); > > > > + Services.downloads.getDownloadByGUID(this.guid, function(status, result) { > > + this.download = result; > > + }.bind(this)); > > So, this needs to be re-thought well, the lazyGetter existed for performance > reasons (don't access the DB unless it's really needed) and this is basically > killing that performance tweak... plus it's breaking this getter checking code > http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#766 > > Though I don't have an answer atm... it may involve deep changes to how we > access the .download property... Paolo may have some insight on that from when > he designed this. Yeah, the biggest problem is that getDownloadByGUID is async, and this code assumes that it has access to a sync API, and this is the best that I could come up with...

(In reply to comment #42) > The only problematic case looks the .sort() in _updateView, we may keep using > id and fix that in a follow-up (we have .startTime on the download but doesn't > look like what we need here) I'm not sure why that's a problem. We don't rely on id's being globally unique in sort, so I think we can just continue to use it.

(In reply to comment #45) > Comment on attachment 685468 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=685468 > Part 6: Don't use the global PB service in DownloadsCommon.jsm > > Review of attachment 685468 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +746,4 @@ > > break; > > > > case "download-manager-database-type-changed": > > + if (this._isPrivate) { > > This notification can only be fired > #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > > so the code should just be ifndef Good point! I'll do that.

(In reply to Ehsan Akhgari [:ehsan] from comment #48) > (In reply to comment #42) > > The only problematic case looks the .sort() in _updateView, we may keep using > > id and fix that in a follow-up (we have .startTime on the download but doesn't > > look like what we need here) > > I'm not sure why that's a problem. We don't rely on id's being globally > unique in sort, so I think we can just continue to use it. Id you look at the nsIDownload idl, .id is deprecated. But yes, here we may keep using it for sorting, then we'll need a follow-up to fix it properly. (In reply to Ehsan Akhgari [:ehsan] from comment #47) > (In reply to comment #43) > > ::: browser/components/downloads/src/DownloadsCommon.jsm > > @@ +739,5 @@ > > > + .getDownloadByGUID(dataItem.guid, function(status, result) { > > > + if (status != Components.results.NS_OK) { > > > + this._removeDataItem(dataItem.downloadId); > > > + } > > > + }.bind(this)); > > > > yeah, as said this should instead use the new > > download-manager-remove-download-guid notification > > Why? It gives you the guid, so you don't have to get it asynchronously... plus per idl we expect privacy aware listeners to work with guids... plus the old notification won't be fired in the new world: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1975

Comment on attachment 685461 [details] [diff] [review] Part 1: Break up the DownloadsCommon.data singleton into two objects, one for normal and one for private downloads Review of attachment 685461 [details] [diff] [review]: ----------------------------------------------------------------- Just a few nits - but I do think this is the right direction. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +175,5 @@ > }, > > /** > + * Get access to one of the DownloadsData or PrivateDownloadsData objects, > + * depending on the privacy status of the window in question. nit: missing @param (I know aWindow is pretty self-evident - that's why it's a nit. :p ) @@ +186,5 @@ > + } > + }, > + > + /** > + * Returns a reference to the PrivateDownloadsData object. This comment needs to be swapped with the comment above the property below. @@ +379,5 @@ > */ > +function DownloadsDataCtor(aPrivate) { > + this._isPrivate = aPrivate; > +} > +DownloadsDataCtor.prototype = { Nit - newline before prototype def'n, for consistency with the rest of the file. @@ +1189,2 @@ > } > }, Down after all of this is DownloadsSummaryData, which currently registers itself as an intermediary view between DownloadsCommon.data (which, in your patch, no longer exists) and DownloadsSummary in content/downloads.js. DownloadsSummary currently subscribes to DownloadsSummaryData via DownloadsCommon.getSummary(numItemsToSkip).addView You'll probably want to change getSummary to take a window parameter too, as you have with getData.

(In reply to Marco Bonardo [:mak] from comment #50) > (In reply to Ehsan Akhgari [:ehsan] from comment #48) > > (In reply to comment #42) > > > The only problematic case looks the .sort() in _updateView, we may keep using > > > id and fix that in a follow-up (we have .startTime on the download but doesn't > > > look like what we need here) > > > > I'm not sure why that's a problem. We don't rely on id's being globally > > unique in sort, so I think we can just continue to use it. > > Id you look at the nsIDownload idl, .id is deprecated. But yes, here we may > keep using it for sorting, then we'll need a follow-up to fix it properly. OK, filed bug 815941. > (In reply to Ehsan Akhgari [:ehsan] from comment #47) > > (In reply to comment #43) > > > ::: browser/components/downloads/src/DownloadsCommon.jsm > > > @@ +739,5 @@ > > > > + .getDownloadByGUID(dataItem.guid, function(status, result) { > > > > + if (status != Components.results.NS_OK) { > > > > + this._removeDataItem(dataItem.downloadId); > > > > + } > > > > + }.bind(this)); > > > > > > yeah, as said this should instead use the new > > > download-manager-remove-download-guid notification > > > > Why? > > It gives you the guid, so you don't have to get it asynchronously... plus > per idl we expect privacy aware listeners to work with guids... plus the old > notification won't be fired in the new world: > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/ > nsDownloadManager.cpp#1975 The last point is a very good one, but the first point it wrong, it gives you the string GUID, so you still need to get the download asynchronously in order to get its id so that we can remove it.

Sigh, we can't use download.id at all in per-window PB mode: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#3263

(In reply to Ehsan Akhgari [:ehsan] from comment #53) > Sigh, we can't use download.id at all in per-window PB mode: > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/ > nsDownloadManager.cpp#3263 Without lifting this restriction, a lot of things in this code needs to change, and I have no idea how to make those changes...

I was honestly expecting fixing this was like opening a can of worms. (In reply to Ehsan Akhgari [:ehsan] from comment #52) > The last point is a very good one, but the first point it wrong, it gives > you the string GUID, so you still need to get the download asynchronously in > order to get its id so that we can remove it. we can remove by GUID, can't we?

Comment on attachment 685965 [details] [diff] [review] Part 1: Break up the DownloadsCommon.data singleton into two objects, one for normal and one for private downloads Review of attachment 685965 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +253,3 @@ > */ > _summary: null, > + getSummary: function DC_getSummary(aNumToExclude, aWindow) So, I don't think this is going to work if we create a summary for a normal window, and then try creating a summary for a private window (since we'll just get the first copy again). We should probably store a _summary and a _privateSummary, and have getSummary return the one appropriate for the window. If one doesn't exist, it should instantiate it, passing a boolean to tell the DownloadsSummaryData what data it should subscribe to. Or, even better, instead of passing a boolean, it should just pass a reference to the data object to subscribe to. That would help us avoid isPrivate? conditionals within DownloadsSummaryData.

Clearing the needinfo for Mano - I think we're way past that now.

(In reply to Marco Bonardo [:mak] from comment #61) > I was honestly expecting fixing this was like opening a can of worms. > > (In reply to Ehsan Akhgari [:ehsan] from comment #52) > > The last point is a very good one, but the first point it wrong, it gives > > you the string GUID, so you still need to get the download asynchronously in > > order to get its id so that we can remove it. > > we can remove by GUID, can't we? There's no such public method exposed on nsIDownloadManager. We created the remove method on nsIDownload to avoid the need to care about the GUID/ID distinction.

What I meant is that the downloads panel code hashes downloads by id, but we can easily switch those hashes to be by guid (we should indeed) so here we'd get the guid and be immediately able to remove the download from the hash.

Note we can even store a guid=>id Map temporarily, for any problematic case.

There are a lot of good comments here already :-) I've also looked at the patches, and my take is that we should strike a balance between a quick and dirty approach on one side, and something that doesn't rely too much on hidden assumptions on the other side. The first step being done here is to go to a "transition" state where we can support builds with and without per-window private browsing mode. In the source code, it's worth noting that: * The new DownloadsData object still alternates between handling public and private downloads in builds without PWPBM. * PrivateDownloadsData is only used in builds with PWPBM. Given the first point, we still need the infrastructure that switches the list of data items when global private browsing states change, and waits until private browsing mode is exited before loading persistent data from disk, but this is going away when we switch to PWPBM. I don't think it's worth #ifdef'ing that code for now, but we should file a follow-up bug to get rid of it later. With regard to the .download getter on the dataItem, it should become an asynchronous method with a callback. Fortunately it's only used three times: * To set the resumable property. This only determines if the resume command is enabled, we can safely delay setting this property until the callback returns a few moments later. * To get the MIME info when opening the target file. We can easily wrap the code that opens the file in a callback function. * In the iterators for the summary view. Here, we already mirrored all the relevant properties for the summary to the dataItem. We can just rewrite the global summarizeDownloads function to accept an iterator of dataItems, and rewrite the _activeDownloads iterator so that it filters from the list of already available data items, instead of calling activeDownloads. More specific notes on the existing patches follow, though they're the direct consequence of the notes above.

Comment on attachment 685965 [details] [diff] [review] Part 1: Break up the DownloadsCommon.data singleton into two objects, one for normal and one for private downloads Review of attachment 685965 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +224,5 @@ > + */ > + ensureAllPersistentDataLoaded: > + function DC_ensureAllPersistentDataLoaded(aActiveOnly) { > + DownloadsData.ensurePersistentDataLoaded(); > + PrivateDownloadsData.ensurePersistentDataLoaded(); We don't store persistent data for private downloads, so no need to call that explicitly. Also, there's a typo, forgot to propagate aActiveOnly. @@ +256,5 @@ > { > if (this._summary) { > return this._summary; > } > + return this._summary = new DownloadsSummaryData(aNumToExclude, aWindow); nit: In addition to the previous comment about correct memoization, I suggest changing aWindow to aPrivate for consistency with the other constructors. nit: While you're here, you can move the linked variables (currently, "_summary") under the function so that the JavaDoc is correctly attributed to the function. @@ +413,5 @@ > * the main browser window until the autostart timeout elapses. > + * > + * Note that DownloadsData and PrivateDownloadsData are two equivalent singleton > + * objects, one accessing non-private downloads, and the other accessing private > + * ones. Fix comment to better describe the transition mode.

Comment on attachment 685966 [details] [diff] [review] Part 2: Use a privacy aware listener for DownloadsData and PrivateDownloadsData Review of attachment 685966 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +807,5 @@ > + if (aDownload.isPrivate != this._isPrivate) { > + // Ignore the downloads with a privacy status other than what we are > + // tracking. > + return; > + } Since _isPrivate is constant and doesn't change with global private browsing state, this and the other similar check should be #ifdef'd.

Comment on attachment 685967 [details] [diff] [review] Part 3: Stop using the deprecated getDownload API Review of attachment 685967 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +767,5 @@ > + for each (dataItem in this.dataItems) > + if (this.dataItems[dataItem].guid == guid)]; > + if (foundItems.length) { > + let id = foundItems[0]; > + this._removeDataItem(dataItem); Typo. @@ +1027,5 @@ > this.maxBytes = aStorageRow.getResultByName("maxBytes"); > > + Services.downloads.getDownloadByGUID(this.guid, function(status, result) { > + this.download = result; > + }.bind(this)); This will become an async getter function, we can't rely on delayed availability of the property.

Comment on attachment 685969 [details] [diff] [review] Part 5: Don't use the global PB service in DownloadsCommon.jsm Review of attachment 685969 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +795,2 @@ > case "download-manager-database-type-changed": > + if (this._isPrivate) { This should still check the current global private browsing state (though the instance for private downloads might as well avoid to react to this notification at all).

Comment on attachment 685970 [details] [diff] [review] Part 6: Use privacy aware cleanup methods Review of attachment 685970 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/content/downloads.js @@ +1162,5 @@ > downloadsCmd_clearList: function DVC_downloadsCmd_clearList() > { > + PrivateBrowsingUtils.isWindowPrivate(window) ? > + Services.downloads.cleanUpPrivate() : > + Services.downloads.cleanUp(); I'd rather see a plain "if" for statements...

(In reply to comment #64) > (In reply to Marco Bonardo [:mak] from comment #61) > > I was honestly expecting fixing this was like opening a can of worms. > > > > (In reply to Ehsan Akhgari [:ehsan] from comment #52) > > > The last point is a very good one, but the first point it wrong, it gives > > > you the string GUID, so you still need to get the download asynchronously in > > > order to get its id so that we can remove it. > > > > we can remove by GUID, can't we? > > There's no such public method exposed on nsIDownloadManager. We created the > remove method on nsIDownload to avoid the need to care about the GUID/ID > distinction. Sure, but that still doesn't help, since we *don't* have an nsIDownload object at hand. Please look at the patch to see how I get one. The only thing I need to do there is to call the remove function on the download object itself and not try to get its id.

(In reply to comment #65) > What I meant is that the downloads panel code hashes downloads by id, but we > can easily switch those hashes to be by guid (we should indeed) so here we'd > get the guid and be immediately able to remove the download from the hash. Hashing by guid may be something that we can do, but I don't know enough about all of the places where we use the numeric id to be able to tell how easy/sane that would be. (In reply to comment #66) > Note we can even store a guid=>id Map temporarily, for any problematic case. Problem is that we can't get the ids for the private downloads (except by maybe reading them directly from the privateDBConnection.

(In reply to comment #62) > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +253,3 @@ > > */ > > _summary: null, > > + getSummary: function DC_getSummary(aNumToExclude, aWindow) > > So, I don't think this is going to work if we create a summary for a normal > window, and then try creating a summary for a private window (since we'll just > get the first copy again). How so? The getSummary functions are called on two different objects, each holding its own _summary property, right?

(In reply to Ehsan Akhgari [:ehsan] from comment #75) > (In reply to comment #62) > > ::: browser/components/downloads/src/DownloadsCommon.jsm > > @@ +253,3 @@ > > > */ > > > _summary: null, > > > + getSummary: function DC_getSummary(aNumToExclude, aWindow) > > > > So, I don't think this is going to work if we create a summary for a normal > > window, and then try creating a summary for a private window (since we'll just > > get the first copy again). > > How so? The getSummary functions are called on two different objects, each > holding its own _summary property, right? getSummary and _summary both belong to the DownloadsCommon singleton (unless you've moved them in one of these patches and I somehow missed that).

(In reply to comment #67) > There are a lot of good comments here already :-) I've also looked at the > patches, and my take is that we should strike a balance between a quick and > dirty approach on one side, and something that doesn't rely too much on hidden > assumptions on the other side. > > The first step being done here is to go to a "transition" state where we can > support builds with and without per-window private browsing mode. In the source > code, it's worth noting that: > * The new DownloadsData object still alternates between handling public and > private downloads in builds without PWPBM. > * PrivateDownloadsData is only used in builds with PWPBM. To be honest, my understanding of this code is extremely limited, and I'm mostly following others' advice blindly. :-) But if I were going to design this myself from scratch, here's what I would do: I'd try to minimize the amount of differences between per-window PB and regular builds as much as possible. So, I would begin with two data store objects, and two processing objects and so on, one operating on the private download data and one operating on the non-private data. This way, the regular build would only register an observer for private-browsing and just switch whatever high-level object the UI code is holding on to, and everything would just work. A per-window PB build would not need to do anything special, and everything would work out of the box, and we were all happy. But I think the existing code is too complicated to allow someone like me who's not familiar with it to rewrite it from scratch with this model. :/ > Given the first point, we still need the infrastructure that switches the list > of data items when global private browsing states change, and waits until > private > browsing mode is exited before loading persistent data from disk, but this is > going away when we switch to PWPBM. I don't think it's worth #ifdef'ing that > code for now, but we should file a follow-up bug to get rid of it later. > > With regard to the .download getter on the dataItem, it should become an > asynchronous method with a callback. Fortunately it's only used three times: > * To set the resumable property. This only determines if the resume command is > enabled, we can safely delay setting this property until the callback > returns a few moments later. > * To get the MIME info when opening the target file. We can easily wrap the > code that opens the file in a callback function. > * In the iterators for the summary view. Here, we already mirrored all the > relevant properties for the summary to the dataItem. We can just rewrite > the global summarizeDownloads function to accept an iterator of dataItems, > and rewrite the _activeDownloads iterator so that it filters from the list > of already available data items, instead of calling activeDownloads. I read over this a couple of times, but it doesn't make any sense to me. :-) I'd appreciate if you can comment more on the code since it's more probable that I can grok those comments!

(In reply to comment #69) > Comment on attachment 685966 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=685966 > Part 2: Use a privacy aware listener for DownloadsData and PrivateDownloadsData > > Review of attachment 685966 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +807,5 @@ > > + if (aDownload.isPrivate != this._isPrivate) { > > + // Ignore the downloads with a privacy status other than what we are > > + // tracking. > > + return; > > + } > > Since _isPrivate is constant and doesn't change with global private browsing > state, this and the other similar check should be #ifdef'd. Is there an easy way to make regular builds also use two of these objects? I think patching things up this way to switch what's in the data store is a very bad approach and I'd like to avoid it if I can.

(In reply to comment #70) > Comment on attachment 685967 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=685967 > Part 3: Stop using the deprecated getDownload API > > Review of attachment 685967 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +767,5 @@ > > + for each (dataItem in this.dataItems) > > + if (this.dataItems[dataItem].guid == guid)]; > > + if (foundItems.length) { > > + let id = foundItems[0]; > > + this._removeDataItem(dataItem); > > Typo. I need to rewrite it to call remove on the download object anyway. :-) (Out of curiosity, why do we copy bits from the download object instead of just storing it in the hashmap?) > @@ +1027,5 @@ > > this.maxBytes = aStorageRow.getResultByName("maxBytes"); > > > > + Services.downloads.getDownloadByGUID(this.guid, function(status, result) { > > + this.download = result; > > + }.bind(this)); > > This will become an async getter function, we can't rely on delayed > availability of the property. Yes, I know! :-) But I can't think of a better solution. Can you please help out on that? Thanks!

(In reply to comment #71) > Comment on attachment 685969 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=685969 > Part 5: Don't use the global PB service in DownloadsCommon.jsm > > Review of attachment 685969 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/downloads/src/DownloadsCommon.jsm > @@ +795,2 @@ > > case "download-manager-database-type-changed": > > + if (this._isPrivate) { > > This should still check the current global private browsing state Actually, I should revert this whole hunk: - let pbs = Cc["@mozilla.org/privatebrowsing;1"] - .getService(Ci.nsIPrivateBrowsingService); - if (pbs.privateBrowsingEnabled) { + if (this._isPrivate) { > (though the > instance for private downloads might as well avoid to react to this > notification at all). Wouldn't that change the behavior of the existing code?

(In reply to comment #76) > (In reply to Ehsan Akhgari [:ehsan] from comment #75) > > (In reply to comment #62) > > > ::: browser/components/downloads/src/DownloadsCommon.jsm > > > @@ +253,3 @@ > > > > */ > > > > _summary: null, > > > > + getSummary: function DC_getSummary(aNumToExclude, aWindow) > > > > > > So, I don't think this is going to work if we create a summary for a normal > > > window, and then try creating a summary for a private window (since we'll just > > > get the first copy again). > > > > How so? The getSummary functions are called on two different objects, each > > holding its own _summary property, right? > > getSummary and _summary both belong to the DownloadsCommon singleton (unless > you've moved them in one of these patches and I somehow missed that). Ah shoot, right. I was confused.

Also, I still don't know how to solve the unavailability of the "id" property. Can someone please give me an idea on what the best thing to do is there? Thanks!

(In reply to Ehsan Akhgari [:ehsan] from comment #82) > Also, I still don't know how to solve the unavailability of the "id" > property. Can someone please give me an idea on what the best thing to do > is there? Thanks! I think you should switch dataItems to use guids, add a dateAdded property to nsIDownload, use it for sorting. Which other code snippets would still require the id at this point?

even if actually we can't have a dateAdded since the db doesn't have that info... we have a startTime, that is definitely not the same but maybe could be used regardless.

Just noticed that there's something missing here - we show a notification when new downloads begin here: http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsCommon.jsm#830 If a download gets triggered in a private window, and the user is using a non-private window, the non-private window is going to have the notification triggered on it, because it was the most recent browser. Is there a way to query for the most recent private window?

Comment on attachment 685970 [details] [diff] [review] Part 6: Use privacy aware cleanup methods Review of attachment 685970 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK to me.

(In reply to comment #83) > (In reply to Ehsan Akhgari [:ehsan] from comment #82) > > Also, I still don't know how to solve the unavailability of the "id" > > property. Can someone please give me an idea on what the best thing to do > > is there? Thanks! > > I think you should switch dataItems to use guids, add a dateAdded property to > nsIDownload, use it for sorting. Which other code snippets would still require > the id at this point? <http://mxr.mozilla.org/mozilla-central/search?find=%2Fbrowser%2Fcomponents%2Fdownloads%2F&string=downloadId> and <http://mxr.mozilla.org/mozilla-central/search?string=\.id&regexp=1&case=1&find=%2Fbrowser%2Fcomponents%2Fdownloads%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central>

(In reply to Mike Conley (:mconley) from comment #85) > Is there a way to query for the most recent private window? we have getMostRecentBrowserWindow in browserGlue we may add an optional aOnlyPrivate argument to it... Could be an interesting addition.

(In reply to comment #85) > Just noticed that there's something missing here - we show a notification when > new downloads begin here: > > http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsCommon.jsm#830 > > If a download gets triggered in a private window, and the user is using a > non-private window, the non-private window is going to have the notification > triggered on it, because it was the most recent browser. > > Is there a way to query for the most recent private window? No, but this code is broken already. Here's why: Click on a link which takes you to a download proxy page (the ones that tell you click on this link to start the download if it doesn't start automatically) and quickly switch to another window. The notification will incorrectly be attributed to the wrong window.

(In reply to comment #88) > (In reply to Mike Conley (:mconley) from comment #85) > > Is there a way to query for the most recent private window? > > we have getMostRecentBrowserWindow in browserGlue we may add an optional > aOnlyPrivate argument to it... Could be an interesting addition. Sure, we can do that, but the code would still be broken because of what I described earlier.

(In reply to Ehsan Akhgari [:ehsan] from comment #89) > (In reply to comment #85) > > Just noticed that there's something missing here - we show a notification when > > new downloads begin here: > > > > http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsCommon.jsm#830 > > > > If a download gets triggered in a private window, and the user is using a > > non-private window, the non-private window is going to have the notification > > triggered on it, because it was the most recent browser. > > > > Is there a way to query for the most recent private window? > > No, but this code is broken already. Here's why: Click on a link which > takes you to a download proxy page (the ones that tell you click on this > link to start the download if it doesn't start automatically) and quickly > switch to another window. The notification will incorrectly be attributed > to the wrong window. The notification is not necessarily supposed to indicate that a download *from a particular window* was started, just that one was started. With our current code, a download proxy page in window A could start while I'm focusing on window B, and window B notifies - this is OK / behaving as designed. We just want the user to know that a download has started, and show them where to click to find out more (the button). But we certainly don't want a proxy in a PB window to trigger a notification in a non-PB window.

(In reply to Ehsan Akhgari [:ehsan] from comment #87) > <http://mxr.mozilla.org/mozilla-central/ > search?find=%2Fbrowser%2Fcomponents%2Fdownloads%2F&string=downloadId> The downloadsCommon.jsm entries appear to be all fixable just by using guids per comment 83. http://mxr.mozilla.org/mozilla-central/search?string=downloadId&find=browser.*%2Fdownloads.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central so, viewItems could hash by guid the "id" attribute can use guid the "downloadId" attribute becomes downloadGuid (We just use it to find the download in viewItems) and (maybe) all the commands may use this.dataItem.download.method(), .download is asynchronous though in this new world, but I think it may be fine to getDownload(function(download) download.doSomething()) or something like that, depends on how we decide to handle it being asynchronous. Paolo said "it should become an asynchronous method with a callback" that would indeed work here.

(In reply to comment #91) > (In reply to Ehsan Akhgari [:ehsan] from comment #89) > > (In reply to comment #85) > > > Just noticed that there's something missing here - we show a notification when > > > new downloads begin here: > > > > > > http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsCommon.jsm#830 > > > > > > If a download gets triggered in a private window, and the user is using a > > > non-private window, the non-private window is going to have the notification > > > triggered on it, because it was the most recent browser. > > > > > > Is there a way to query for the most recent private window? > > > > No, but this code is broken already. Here's why: Click on a link which > > takes you to a download proxy page (the ones that tell you click on this > > link to start the download if it doesn't start automatically) and quickly > > switch to another window. The notification will incorrectly be attributed > > to the wrong window. > > The notification is not necessarily supposed to indicate that a download *from > a particular window* was started, just that one was started. With our current > code, a download proxy page in window A could start while I'm focusing on > window B, and window B notifies - this is OK / behaving as designed. We just > want the user to know that a download has started, and show them where to click > to find out more (the button). > > But we certainly don't want a proxy in a PB window to trigger a notification in > a non-PB window. What if the window that originally triggered the download is closed and there is no other private window to serve this purpose?

(In reply to comment #92) > (In reply to Ehsan Akhgari [:ehsan] from comment #87) > > <http://mxr.mozilla.org/mozilla-central/ > > search?find=%2Fbrowser%2Fcomponents%2Fdownloads%2F&string=downloadId> > > The downloadsCommon.jsm entries appear to be all fixable just by using guids > per comment 83. > > http://mxr.mozilla.org/mozilla-central/search?string=downloadId&find=browser.*%2Fdownloads.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central > > so, viewItems could hash by guid > the "id" attribute can use guid > the "downloadId" attribute becomes downloadGuid (We just use it to find the > download in viewItems) > and (maybe) all the commands may use this.dataItem.download.method(), .download > is asynchronous though in this new world, but I think it may be fine to > getDownload(function(download) download.doSomething()) or something like that, > depends on how we decide to handle it being asynchronous. Paolo said "it should > become an asynchronous method with a callback" that would indeed work here. OK, I'll take a stab at this.... I may actually do that in another bug, since this bug is already too large for my little brain. I just hope that this won't mean rewriting half of this stuff (/me shivers).

OK, I have an initial patch for comment 94 in bug 815941.

(In reply to Ehsan Akhgari [:ehsan] from comment #95) > OK, I have an initial patch for comment 94 in bug 815941. And of course these patches need to be rebased on top of it.. I did that on a pristine branch.

(In reply to Ehsan Akhgari [:ehsan] from comment #77) > But if I were going to design this myself from scratch, here's what I would > do: I'd try to minimize the amount of differences between per-window PB and > regular builds as much as possible. So, I would begin with two data store > objects, and two processing objects and so on, one operating on the private > download data and one operating on the non-private data. This way, the > regular build would only register an observer for private-browsing and just > switch whatever high-level object the UI code is holding on to, and > everything would just work. A per-window PB build would not need to do > anything special, and everything would work out of the box, and we were all > happy. Yeah, that's the complete approach. For now, we cannot just use two data stores at the same time, in builds without PWPBM, because we don't have access to the database connection for public downloads while we are in global private browsing mode (DBConnection becomes the same as privateDBConnection). And this switching in the back-end does not only affect the connection, but also all the related download notifications. > But I think the existing code is too complicated to allow someone like me > who's not familiar with it to rewrite it from scratch with this model. :/ Even for people already familiar with the code, this approach has a higher potential for unforeseen regressions (in addition to requiring additions to the underlying nsIDownloadManager interface). This is what I meant with striking a balance between a quick approach and the ideal solution. > > Given the first point, we still need the infrastructure that switches the list > > of data items [...] > > With regard to the .download getter on the dataItem, [...] > > I read over this a couple of times, but it doesn't make any sense to me. > :-) I'd appreciate if you can comment more on the code since it's more > probable that I can grok those comments! Sorry for having taken too many things for granted there, hope you've not lost too much time trying to link those indications to actual code. I've seen you've eventually addressed those in bug 815941, thanks for letting me know it was unclear in any case, it helps a lot in improving my review skills :-) (In reply to Ehsan Akhgari [:ehsan] from comment #78) > > Since _isPrivate is constant and doesn't change with global private browsing > > state, this and the other similar check should be #ifdef'd. > > Is there an easy way to make regular builds also use two of these objects? > I think patching things up this way to switch what's in the data store is a > very bad approach and I'd like to avoid it if I can. Not an easy one, no, as indicated above. The good news are that, in builds with PWPBM enabled instead, we do have access to both data stores at the same time. So, the code that handles the switching will go away: http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#714 (In reply to Ehsan Akhgari [:ehsan] from comment #79) > (Out of curiosity, why do we copy bits from the download object instead of > just storing it in the hashmap?) Firstly, because for completed downloads, we build the dataItem directly from the database, and we don't have a corresponding nsIDownload object. We'll get an nsIDownload object later if needed. Accessing the nsIDownload object for a completed download causes synchronous main-thread database access, so we want to minimize that if necessary. An secondly, because in the medium term we're getting rid of nsIDownload once and for all, replacing it with a pure JavaScript object that will effectively take the place of the dataItem. This will remove *lots* of compatibility code and complexity from the panel front-end. https://wiki.mozilla.org/User:P.A./Download_architecture_improvements (In reply to Ehsan Akhgari [:ehsan] from comment #80) > Actually, I should revert this whole hunk: > > - let pbs = Cc["@mozilla.org/privatebrowsing;1"] > - .getService(Ci.nsIPrivateBrowsingService); > - if (pbs.privateBrowsingEnabled) { > + if (this._isPrivate) { True. > > (though the > > instance for private downloads might as well avoid to react to this > > notification at all). > > Wouldn't that change the behavior of the existing code? Ad interim, we'll have two DownloadsDataCtor instances, each with one dataItems and one _persistentDataItems property. When (only in builds without PWPBM) "download-manager-database-type-changed" is notified, we exchange the two items in DownloadsData. The PrivateDownloadsData object is never used. In builds with PWPBM, we use the dataItems collection in DownloadsData and PrivateDownloadsData, thus we don't need _persistentDataItems anymore. Since "download-manager-database-type-changed" is never fired, we don't need to do anything special to avoid that we switch the collections inadvertently. I hope we'll stop supporting builds without PWPBM soon after enabling it :-)

(In reply to Mike Conley (:mconley) from comment #91) > But we certainly don't want a proxy in a PB window to trigger a notification > in a non-PB window. Well, that would not be too bad, the private window is already open in the background and the worst thing that can happen is that a notification will briefly move towards a button in a public window, hinting towards a panel where the started download is not really present. Plus, I'm not sure we allow web pages to auto-start downloads without prompting in private windows at all. We can file an enhancement follow-up bug for that edge case. (To clarify, we're talking about the transient moving arrow, not the permanent attention highlight for completed downloads, that is already different for private and public windows.)

(In reply to Ehsan Akhgari [:ehsan] from comment #96) > And of course these patches need to be rebased on top of it.. I did that on > a pristine branch. May I ask you (if the other reviewers don't disagree) to fold to a single patch in the next revision? There are not so many changes remaining on top of bug 815941, and I find it easier both for getting a global sense of the changes while reviewing, and for applying the patch to a local working copy (that is the best way for me to review when, like in this case, I need more context).

(In reply to comment #97) > Yeah, that's the complete approach. For now, we cannot just use two data stores > at the same time, in builds without PWPBM, because we don't have access to the > database connection for public downloads while we are in global private > browsing > mode (DBConnection becomes the same as privateDBConnection). And this switching > in the back-end does not only affect the connection, but also all the related > download notifications. Hmm, good point. In that case, maybe it makes sense to #ifdef a bunch of stuff (but not hopefully too much.) This is what we've been doing elsewhere in the code where it was not possible to come up with a clean implementation handling both modes. > Sorry for having taken too many things for granted there, hope you've not lost > too much time trying to link those indications to actual code. I've seen you've > eventually addressed those in bug 815941, thanks for letting me know it was > unclear in any case, it helps a lot in improving my review skills :-) No worries. :-) > (In reply to Ehsan Akhgari [:ehsan] from comment #78) > > > Since _isPrivate is constant and doesn't change with global private browsing > > > state, this and the other similar check should be #ifdef'd. > > > > Is there an easy way to make regular builds also use two of these objects? > > I think patching things up this way to switch what's in the data store is a > > very bad approach and I'd like to avoid it if I can. > > Not an easy one, no, as indicated above. OK. > The good news are that, in builds with PWPBM enabled instead, we do have access > to both data stores at the same time. So, the code that handles the switching > will go away: > > http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/src/DownloadsCommon.jsm#714 Right. I have this #ifdef'ed out in one of these patches. > (In reply to Ehsan Akhgari [:ehsan] from comment #79) > > (Out of curiosity, why do we copy bits from the download object instead of > > just storing it in the hashmap?) > > Firstly, because for completed downloads, we build the dataItem directly from > the database, and we don't have a corresponding nsIDownload object. We'll get > an nsIDownload object later if needed. Accessing the nsIDownload object for > a completed download causes synchronous main-thread database access, so we > want to minimize that if necessary. I was going to say what if we used GetDownloadByGUID, since it's asynchronous, but I was looking at that code last night and it seems to be main-thread as well, and just calls the callback through a runnable. ;-) > An secondly, because in the medium term we're getting rid of nsIDownload once > and for all, replacing it with a pure JavaScript object that will effectively > take the place of the dataItem. This will remove *lots* of compatibility code > and complexity from the panel front-end. > > https://wiki.mozilla.org/User:P.A./Download_architecture_improvements Scary stuff! ;-) > > > (though the > > > instance for private downloads might as well avoid to react to this > > > notification at all). > > > > Wouldn't that change the behavior of the existing code? > > Ad interim, we'll have two DownloadsDataCtor instances, each with one dataItems > and one _persistentDataItems property. When (only in builds without PWPBM) > "download-manager-database-type-changed" is notified, we exchange the two > items in DownloadsData. The PrivateDownloadsData object is never used. I'm not sure if I understand what you're trying to do here. > In builds with PWPBM, we use the dataItems collection in DownloadsData and > PrivateDownloadsData, thus we don't need _persistentDataItems anymore. Since > "download-manager-database-type-changed" is never fired, we don't need to do > anything special to avoid that we switch the collections inadvertently. Oh I see. OK. I guess I actually need to read this whole code and understand what it does the next time that I try to rebase these patches. Which is sad news since I'm working on this in my "free time", but I'll try to deal with it. :-) > I hope we'll stop supporting builds without PWPBM soon after enabling it :-) My goal is to remove all of that code and anything protected by these #ifdefs one release after we ship this. The build time flag is basically being used as a disaster recovery in case something goes wrong late inthe beta cycle for example. This is unfortunate, but it was the only way to pull this large of a project off. :/

(In reply to comment #98) > (In reply to Mike Conley (:mconley) from comment #91) > > But we certainly don't want a proxy in a PB window to trigger a notification > > in a non-PB window. > > Well, that would not be too bad, the private window is already open in the > background and the worst thing that can happen is that a notification will > briefly move towards a button in a public window, hinting towards a panel where > the started download is not really present. Plus, I'm not sure we allow web > pages to auto-start downloads without prompting in private windows at all. I'm not aware of any code which would prevent that. > (To clarify, we're talking about the transient moving arrow, not the permanent > attention highlight for completed downloads, that is already different for > private and public windows.) Oh ok. In that case I guess I don't care very much about this problem, so I can do whatever you guys want me to. :-)

(In reply to comment #99) > (In reply to Ehsan Akhgari [:ehsan] from comment #96) > > And of course these patches need to be rebased on top of it.. I did that on > > a pristine branch. > > May I ask you (if the other reviewers don't disagree) to fold to a single patch > in the next revision? There are not so many changes remaining on top of bug > 815941, and I find it easier both for getting a global sense of the changes > while > reviewing, and for applying the patch to a local working copy (that is the best > way for me to review when, like in this case, I need more context). I'll do you one better. I'll submit individual patches for review, and will _also_ submit a folded patch that you can easily apply!

(In reply to Ehsan Akhgari [:ehsan] from comment #100) > I was going to say what if we used GetDownloadByGUID, since it's > asynchronous, but I was looking at that code last night and it seems to be > main-thread as well, and just calls the callback through a runnable. ;-) yes, it's fake async. Unfortunately mixing up synchronous and asynchronous queries on the same connection is not going to work (thread contention). So this is ready for future but not yet totally async.

I would also like a merged patch here, we got an idea of the various pieces, at this point we need the complete image of the changes. The parts 2-6 are so small that are not helping much the review.

This patch should address all of the comments so far. This passes the tests on both global and per-window PB builds (although I have not written a per-window test for it yet.) I don't see any JS errors when running either of the two builds, which is good news. Testing in global PB builds, I think that this patch does not regress anything (I'd really appreciate if you can test it as well.) Testing in per-window PB builds, the patch doesn't seem to work correctly, in that both private and non-private downloads show up in the downloads panel for both normal and private windows. I can't quite figure out why, and the fact that I cannot seem to get the new chrome debugger devtools to work doesn't let me debug it either. Do you guys have any idea what I'm doing wrong? Thanks!

Looks like a few properties on the prototype for the DownloadsDataCtor needed to be moved into instances. I think I got them all. This fixes the behaviour of the downloads panel in Private Browsing for me, and seems to correctly separate the two groups of downloads from one another. \o/

Thanks a lot, Mike! Out of curiosity, is this an accurate interdiff for your changes? <https://bugzilla.mozilla.org/attachment.cgi?oldid=687469&action=interdiff&newid=688559&headers=1>

(In reply to Ehsan Akhgari [:ehsan] from comment #107) > Thanks a lot, Mike! Out of curiosity, is this an accurate interdiff for > your changes? > <https://bugzilla.mozilla.org/attachment. > cgi?oldid=687469&action=interdiff&newid=688559&headers=1> Yes, it is. Lines 1407 onwards were Josh's changes.

Comment on attachment 688559 [details] [diff] [review] Patch v4 Review of attachment 688559 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty OK with what's going on in here. Just a few minor notes. ::: browser/components/downloads/content/downloads.js @@ +1162,5 @@ > */ > commands: { > downloadsCmd_clearList: function DVC_downloadsCmd_clearList() > { > + PrivateBrowsingUtils.isWindowPrivate(window) ? I think I recall Paolo saying he'd prefer if/else's for this kinda thing. @@ +1463,5 @@ > { > if (aVisible == this._visible || !this._summaryNode) { > return this._visible; > } > + let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window); It's strange that we have a getData(window) pattern established elsewhere, but for getSummary, we pass isPrivate. How about for getSummary, we pass (window, DownloadsView.kItemCountLimit) instead, for symmetry? ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +215,5 @@ > + > + /** > + * Reloads the specified kind of downloads from the private and non-private > + * databases. > + * This Nit: weird newline. @@ +268,1 @@ > _summary: null, Nit - should define _privateSummary: null down here too. @@ +430,5 @@ > + // associated objects are replaced with the value "null". This is required to > + // prevent race conditions when populating the list asynchronously. > + this.dataItems = {}; > + > + // While operating in Private Browsing Mode, persistent data items are parked With what we're doing now with private browsing, I'm not sure _persistentDataItems is needed anymore... @@ +1214,5 @@ > /** > + * Determines whether this view object is over the private or non-private > + * downloads. > + */ > + _isPrivate: false, We're kinda dodging a bullet here by having the "child classes" that use this prototype override this._isPrivate in their constructors. I wonder if we might help enforce this (that the children *must* override _isPrivate), but replacing this _isPrivate here with a getter that throws NS_ERROR_NOT_IMPLEMENTED. Dunno if that's how we roll with this kind of thing, just an idea. If so, we should probably do the same thing with _views up above.

(In reply to Mike Conley (:mconley) from comment #109) > ::: browser/components/downloads/content/downloads.js > @@ +1162,5 @@ > > */ > > commands: { > > downloadsCmd_clearList: function DVC_downloadsCmd_clearList() > > { > > + PrivateBrowsingUtils.isWindowPrivate(window) ? > > I think I recall Paolo saying he'd prefer if/else's for this kinda thing. Right, I'll fix it in the next iteration. > @@ +1463,5 @@ > > { > > if (aVisible == this._visible || !this._summaryNode) { > > return this._visible; > > } > > + let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window); > > It's strange that we have a getData(window) pattern established elsewhere, > but for getSummary, we pass isPrivate. > > How about for getSummary, we pass (window, DownloadsView.kItemCountLimit) > instead, for symmetry? Hmm, that's what I used to do and then I think someone asked me to pass in a boolean. I can change it again but I'd prefer us to come to a conclusion first, because otherwise this can go on for a while. :-) > @@ +430,5 @@ > > + // associated objects are replaced with the value "null". This is required to > > + // prevent race conditions when populating the list asynchronously. > > + this.dataItems = {}; > > + > > + // While operating in Private Browsing Mode, persistent data items are parked > > With what we're doing now with private browsing, I'm not sure > _persistentDataItems is needed anymore... Hmm, is it not used for global PB? > @@ +1214,5 @@ > > /** > > + * Determines whether this view object is over the private or non-private > > + * downloads. > > + */ > > + _isPrivate: false, > > We're kinda dodging a bullet here by having the "child classes" that use > this prototype override this._isPrivate in their constructors. > > I wonder if we might help enforce this (that the children *must* override > _isPrivate), but replacing this _isPrivate here with a getter that throws > NS_ERROR_NOT_IMPLEMENTED. Dunno if that's how we roll with this kind of > thing, just an idea. FWIW, that still won't prevent people from forgetting to override _isPrivate. :-) And this is not the only place in this code where we have this problem, so I'd rather do what all other such places do (i.e., nothing!)

Comment on attachment 688559 [details] [diff] [review] Patch v4 Review of attachment 688559 [details] [diff] [review]: ----------------------------------------------------------------- I'm leaving the final review to paolo who has the best knowledge here, though I'm quite positive about these changes. I think would be simpler for Paolo to wait an updated patch with Mike's and my comments addressed. ::: browser/components/downloads/content/downloads.js @@ +1463,5 @@ > { > if (aVisible == this._visible || !this._summaryNode) { > return this._visible; > } > + let isPrivate = PrivateBrowsingUtils.isWindowPrivate(window); I honestly don't have preferences between window or a bool, it's basically the same we always end up invoking isWindowPrivate... I'd say to just keep using window consistently as Mike suggested: (window, DownloadsView.kItemCountLimit) . ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +214,5 @@ > + }, > + > + /** > + * Reloads the specified kind of downloads from the private and non-private > + * databases. The comment should be updated to reflect reality, we actually need to do this only for non-private store @@ +253,2 @@ > */ > + getSummary: function DC_getSummary(aNumToExclude, aIsPrivate) invert, pass window, what was said, thx, bye @@ +262,5 @@ > + if (this._summary) { > + return this._summary; > + } > + return this._summary = new DownloadsSummaryData(aNumToExclude, aIsPrivate); > + } Is there a reason this is not using the same lazyGetter style we used for all of the other singletons? while here we should probably fix it @@ +432,5 @@ > + this.dataItems = {}; > + > + // While operating in Private Browsing Mode, persistent data items are parked > + // here until we return to the normal mode. > + this._persistentDataItems = {}; can be #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING @@ +444,5 @@ > + > + // Indicates which kind of items from the persistent downloads database have > + // been fully loaded in memory and are available to the views. This can > + // assume the value of one of the kLoad constants. > + this._loadState = 0; You need to move only objects and arrays to the constructor, since they are by ref, simple types are by value. So you should be able to move back _pendingStatement and _loadState @@ +916,5 @@ > } > > // Show the panel in the most recent browser window, if present. > + let browserWin = > + gBrowserGlue.getMostRecentBrowserWindowWithPrivacyStatus(this._isPrivate); ok, we clearly need to wait for the dependency here! @@ +1214,5 @@ > /** > + * Determines whether this view object is over the private or non-private > + * downloads. > + */ > + _isPrivate: false, Add an uppercase comment above views and _isPrivate that they MUST be overridden... _views should likely be inited to null, since that array hold by ref would be a subtle bug to find. @@ +1231,5 @@ > if (this._views.length == 0) { > + let data = this._isPrivate ? > + PrivateDownloadsData : > + DownloadsData; > + data.addView(this); you don't need the data temp var here, just if (this._isPrivate) privatedd.addview else dd.addview @@ +1270,5 @@ > if (this._views.length == 0) { > + let data = this._isPrivate ? > + PrivateDownloadsData : > + DownloadsData; > + data.removeView(this); ditto

Comment on attachment 688559 [details] [diff] [review] Patch v4 Review of attachment 688559 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +262,5 @@ > + if (this._summary) { > + return this._summary; > + } > + return this._summary = new DownloadsSummaryData(aNumToExclude, aIsPrivate); > + } Yes. You cannot use lambda functions which take arguments with XPCOMUtils.defineLazyGetter, so there would be no way to pass in aNumToExclude. Speaking of which, this function doesn't take aNumToExclude into account for memoizaion. In the interest of keeping the scope of this bug limited and actually getting to land this patch at some point, I filed bug 818776 for that. :-) @@ +444,5 @@ > + > + // Indicates which kind of items from the persistent downloads database have > + // been fully loaded in memory and are available to the views. This can > + // assume the value of one of the kLoad constants. > + this._loadState = 0; _pendingStatement _is_ an object. I'll move _loadState back. @@ +1214,5 @@ > /** > + * Determines whether this view object is over the private or non-private > + * downloads. > + */ > + _isPrivate: false, Hmm, that requires me to do a null check for this._views everywhere. I'll do that in the next version of the patch.

Hopefully I've addressed all of the comments on this patch! /me holds his breath...

(In reply to Ehsan Akhgari [:ehsan] from comment #112) > @@ +444,5 @@ > > + > > + // Indicates which kind of items from the persistent downloads database have > > + // been fully loaded in memory and are available to the views. This can > > + // assume the value of one of the kLoad constants. > > + this._loadState = 0; > > _pendingStatement _is_ an object. I'll move _loadState back. no, _pendingStatement is null, and later an object is assigned to it in an instance, that means it's assigned to the instance, not to the prototype. You can test that in scratchpad in case of doubts. > > @@ +1214,5 @@ > > /** > > + * Determines whether this view object is over the private or non-private > > + * downloads. > > + */ > > + _isPrivate: false, > > Hmm, that requires me to do a null check for this._views everywhere. I'll > do that in the next version of the patch. Why, it would fail only if someone doesn't override it in the constructor, it should indeed fail and throw. If you null check it, you remove this "protection".

Addressed Marco's comments.

Comment on attachment 689183 [details] [diff] [review] Patch (v6) Review of attachment 689183 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +1215,5 @@ > * data changes. > + * > + * SUBCLASSES MUST OVERRIDE THIS PROPERTY. > + */ > + _views: null, I think you misunderstood me, the only instance of _views I wanted inited to null was this one, since defining an array here was defining it for the prototype, by ref. so could have created subtle bugs. (note: this is indeed right) @@ +1407,5 @@ > * the main browser window until the autostart timeout elapses. > */ > +function DownloadsIndicatorDataCtor(aPrivate) { > + this._isPrivate = aPrivate; > + this._views = null; I didn't mean here... @@ +1683,5 @@ > this._numToExclude = aNumToExclude; > // Since we can have multiple instances of DownloadsSummaryData, we > // override these values from the prototype so that each instance can be > // completely separated from one another. > + this._views = null; nor here

(Rebased on trunk)

Comment on attachment 689192 [details] [diff] [review] Patch (v7.1) Review of attachment 689192 [details] [diff] [review]: ----------------------------------------------------------------- Per discussion on IRC, we would like to have this in-tree before the next nightly since tomorrow there is a testday, so I'm marking this since I feel like it's ready. I'd still appreciate if Paolo could take a look at it and we'll address his comments post-facto. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +255,5 @@ > + if (PrivateBrowsingUtils.isWindowPrivate(aWindow)) { > + if (this._privateSummary) { > + return this._privateSummary; > + } > + return this._privateSummary = new DownloadsSummaryData(aNumToExclude, true); I find confusing that arguments of DownloadsSummaryData are inverted compared to those of getSummary, may you please swap them so we pass (true, aNumToExclude)? @@ +260,5 @@ > + } else { > + if (this._summary) { > + return this._summary; > + } > + return this._summary = new DownloadsSummaryData(aNumToExclude, false); here as well @@ +922,4 @@ > } > > // Show the panel in the most recent browser window, if present. > + let browserWin = RecentWindow.getMostRecentBrowserWindow({private: this._isPrivate}); add whitespaces in the object to help readability: ({ private: this._isPrivate }); @@ +1491,4 @@ > */ > getViewItem: function DID_getViewItem(aDataItem) > { > + let data = this._isPrivate ? PrivateDownloadsIndicatorData : DownloadsIndicatorData; this is going over 80 chars, let data = this._isPrivate ? PrivateDownloadsIndicatorData : DownloadsIndicatorData; @@ +1609,5 @@ > _activeDataItems: function DID_activeDataItems() > { > + let dataItems = this._isPrivate ? > + PrivateDownloadsData.dataItems : > + DownloadsData.dataItems; nit: indent as let dataItems = this._isPrivate ? PrivateDownloadsData.dataItems : DownloadsData.dataItems; @@ +1680,2 @@ > */ > +function DownloadsSummaryData(aNumToExclude, aIsPrivate) { invert arguments, as said before (also in the javadoc)

Backed out because of leaks: https://hg.mozilla.org/mozilla-central/rev/cf0da0cd7723

Note to self: this._views is not initialized in DownloadsIndicatorData

Let's try again boys! https://hg.mozilla.org/mozilla-central/rev/ea20935fa19d

Backed out again because of leaks: https://hg.mozilla.org/mozilla-central/rev/c8e785e18be8

ehsan so, in global PB builds, we hand out one or the other, depending on whether isWindowPrivate returns true *at that specific time* ehsan when you switch to PB mode, isWindowPrivate will start to return true ehsan and then we'll start using the wrong object mak yes, I see what you mean mak but I thought we did finalization of both ehsan which in this case leads to a leak, because we add views on one object and remove them on others ehsan no we don't ehsan look at DownloadsPanel.terminate for evidence ehsan but it's actually worse than just leaking! ehsan if you go to PB mode, and then open a new window, the panel for the original window is still using the non-private object ehsan and the panel for the new window will use a private one ehsan so your two panels will watch over different downloads!

Comment on attachment 689476 [details] [diff] [review] Fix Review of attachment 689476 [details] [diff] [review]: ----------------------------------------------------------------- I admit 2am is not the best time of the day to figure out a review, though we'll discover easily if everything breaks :)

Sorry guys, I've been concentrated on getting bug 789932 out of the door and couldn't follow this bug with the due attention this week. I can only do a drive-by review at this time in the night, will be able to look again into this tomorrow or the day after. So, does the check in onDownloadStateChange and onDownloadProgressChange work correctly? If we use DownloadsData for both private and non-private downloads, in global PBM builds, I'd expect it not to match when PB mode is entered (because the private attribute of downloads does change depending on whether the download was started in PBM). (In reply to Ehsan Akhgari [:ehsan] from comment #126) > ehsan > so, in global PB builds, we hand out one or the other, depending on whether > isWindowPrivate returns true *at that specific time* > ehsan > when you switch to PB mode, isWindowPrivate will start to return true Interesting data point, didn't know that.

(In reply to comment #129) > Sorry guys, I've been concentrated on getting bug 789932 out of the door and > couldn't follow this bug with the due attention this week. I can only do a > drive-by review at this time in the night, will be able to look again into this > tomorrow or the day after. > > So, does the check in onDownloadStateChange and onDownloadProgressChange work > correctly? If we use DownloadsData for both private and non-private downloads, > in global PBM builds, I'd expect it not to match when PB mode is entered > (because the private attribute of downloads does change depending on whether > the download was started in PBM). That's a very good point. I'll take it out. Actually, I'll do a bit more manual testing on this before I land it...

Third time's a... https://hg.mozilla.org/mozilla-central/rev/9135ccf6e408

I reviewed the latest version in the tree, and it looks good to me! The source selection in ensurePersistentDataLoaded could be replaced by an assertion that the function is only called on the non-private DownloadsData object, but it doesn't hurt to keep it as is: https://hg.mozilla.org/mozilla-central/rev/9135ccf6e408#l3.289 Thanks Ehsan for working on this rapidly, and sorry again for the delay!

(In reply to comment #132) > I reviewed the latest version in the tree, and it looks good to me! The source > selection in ensurePersistentDataLoaded could be replaced by an assertion that > the function is only called on the non-private DownloadsData object Right.. If you feel that we should do this anyway, I'd be more than happy to do that. Please file a follow-up bug if you want that fixed. Thanks!

Comment on attachment 689221 [details] [diff] [review] Patch (v8) >--- a/browser/components/downloads/src/DownloadsCommon.jsm >+++ b/browser/components/downloads/src/DownloadsCommon.jsm >- let browserWin = gBrowserGlue.getMostRecentBrowserWindow(); >+ let browserWin = RecentWindow.getMostRecentBrowserWindow({ private: this._isPrivate }); This makes gBrowserGlue unused.

(In reply to Dão Gottwald [:dao] from comment #134) > Comment on attachment 689221 [details] [diff] [review] > Patch (v8) > > >--- a/browser/components/downloads/src/DownloadsCommon.jsm > >+++ b/browser/components/downloads/src/DownloadsCommon.jsm > > >- let browserWin = gBrowserGlue.getMostRecentBrowserWindow(); > >+ let browserWin = RecentWindow.getMostRecentBrowserWindow({ private: this._isPrivate }); > > This makes gBrowserGlue unused. Filed bug 819956 for that.

(In reply to Ehsan Akhgari [:ehsan] from comment #133) > Right.. If you feel that we should do this anyway, I'd be more than happy > to do that. Please file a follow-up bug if you want that fixed. Thanks! Filed bug 820301, but there's no hurry to work on that.

Hi All, I am facing an issue and it seems it might be related to what has been discussed above, I have kind of skimmed through it as its too much in detail lol. I had upgraded the ff18 to Ver 19 a few days ago and came across this issue/ bug. I have again upgraded again to the beta ver20 as well and it still persists. Whenever I try to download something in the private browsing mode, none of the downloads (download progress) are shown in the download window I.e. things like 'the % downloaded' and the 'speed' etc even though I know the download is taking place. This was very useful in the old versions of FF. Any suggestions other than going back to ver18? I have also tried disabling all the addons as well.

lolzz270, would you mind filing a new bug for the issue you described in commment 137? We try to keep our issues distinct here, and the one described originally is fixed for the purposes of release management. I'd like to discuss it further with you without making all the people on the CC list here have to read our conversation as well.