The "Show All Downloads", and "+ X other downloads" items in the Download Panel currently open the toolkit downloads window. If the Places Downloads view is preffed on, we should open it instead.

This changes BrowserDownloadsUI to try opening the Places view if browser.library.useNewDownloadsView is set to true. If false, or if all else fails, reverts to the old toolkit Downloads window.

Comment on attachment 693002 [details] [diff] [review] Patch v1 r=mano for the browser.js changes only (I think the gnomestipe bits belong to bug 822257)

(In reply to Mano from comment #2) > Comment on attachment 693002 [details] [diff] [review] > Patch v1 > > r=mano for the browser.js changes only (I think the gnomestipe bits belong > to bug 822257) Wow, yep, that stuff really shouldn't be in there. Thanks, I'll remove before landing.

Hm - I seem to have missed a spot: http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsUI.js#62 I'll come up with a new patch.

(In reply to Mike Conley (:mconley) from comment #4) > Hm - I seem to have missed a spot: > > http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/ > downloads/src/DownloadsUI.js#62 > > I'll come up with a new patch. Whoops - linked to the wrong lines: http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsUI.js#75 and http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/downloads/src/DownloadsUI.js#87

Instead of patching BrowserDownloadsUI like before, we put the switching logic inside DownloadsUI.js. Marco - for the Places UI, do you know if I should handle getAttention and visible for nsIDownloadManagerUI as well?

(In reply to Mike Conley (:mconley) from comment #6) > Marco - for the Places UI, do you know if I should handle getAttention and > visible for nsIDownloadManagerUI as well? I'm not sure if we documented somewhere how and if the Library should get attention compared to the main window that contains the panel. The current code just makes me think we didn't want to get attention with the new ui. This is likely something for a separate bug to evaluate that. I'd be fine with the lack attention on first impl. visible is used to show the downloads UI when a new download starts: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadProxy.h#63 we don't want this, so I think for now it's ok to just always return true (if useToolkitUI is false), with a nice comment. Then we'll see in future. The problem is whether we want to open the Library or bring on a browser window with the panel.

Ok, added the comment. I hope I understood the problem correctly - let me know if I should alter it any.

Comment on attachment 693426 [details] [diff] [review] Patch v3 Review of attachment 693426 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsUI.js @@ +108,5 @@ > + * Helper function that tries to open the Downloads view in the Places > + * Organizer. If the Downloads View in Places is preffed off, we fallback > + * to showing the toolkit downloads manager. > + */ > + _showPlacesUI: function(aWindowContext, aID, aReason) So, if useToolkitUI is true, we should always trust that, regardless the downloads view pref. Thus I think this should be renamed to _showDownloadManagerUI() and will indeed open the right manager. And the javadoc should be updated according. @@ +115,5 @@ > + if (Services.prefs.getBoolPref("browser.library.useNewDownloadsView")) { > + let organizer = Services.wm.getMostRecentWindow("Places:Organizer"); > + if (!organizer) { > + Services.ww > + .activeWindow So, I wonder if we should open the Library window if there is no Browser window around... I'm prone to say no, so maybe here we should just use RecentWindow.jsm @@ +128,5 @@ > + return; > + } > + } catch(e) {} > + > + this._toolkitUI.show(aWindowContext, aID, aReason); As stated, we should show the toolkitUI if: - useToolkitUI is true - useToolkitUI is false but we are not using the new view

(In reply to Marco Bonardo [:mak] from comment #9) > Comment on attachment 693426 [details] [diff] [review] > Patch v3 > > Review of attachment 693426 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/downloads/src/DownloadsUI.js > @@ +108,5 @@ > > + * Helper function that tries to open the Downloads view in the Places > > + * Organizer. If the Downloads View in Places is preffed off, we fallback > > + * to showing the toolkit downloads manager. > > + */ > > + _showPlacesUI: function(aWindowContext, aID, aReason) > > So, if useToolkitUI is true, we should always trust that, regardless the > downloads view pref. Ok, fixed. > > Thus I think this should be renamed to _showDownloadManagerUI() and will > indeed open the right manager. And the javadoc should be updated according. > Done. > @@ +115,5 @@ > > + if (Services.prefs.getBoolPref("browser.library.useNewDownloadsView")) { > > + let organizer = Services.wm.getMostRecentWindow("Places:Organizer"); > > + if (!organizer) { > > + Services.ww > > + .activeWindow > > So, I wonder if we should open the Library window if there is no Browser > window around... I'm prone to say no, so maybe here we should just use > RecentWindow.jsm > Done - though I'm a little worried about downloads that were initiated from private browsing windows... though I guess we can sort that out once we have the downloads view for private browsing windows displaying in tabs.

Comment on attachment 693879 [details] [diff] [review] Patch v4 Review of attachment 693879 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/src/DownloadsUI.js @@ +63,3 @@ > if (!aReason) { > aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED; > } I think I confused you with my previous comments, and now I see you were right. Sorry! The global check: if (DownloadsCommon.useToolkitUI) { this._toolkitUI.show(aWindowContext, aID, aReason); return; } should stay where it was, at the beginning of show() and not be moved to _showDownloadManagerUI. This way we reach _showDownloadManagerUI only if DownloadsCommon.useToolkitUI is false, that is exactly what we want. Doing differently may change the default behavior of the old UI. @@ +87,5 @@ > > get visible() > { > + // If we're still using the toolkit downloads manager, delegate the call > + // to it. Otherwise, return true for now until we decide on how we want comma after "now" @@ +106,5 @@ > + * > + * 1) useToolkitUI is true > + * 2) useToolkitUI is false, but the Places Downloads View is disabled. > + * > + * Otherwise, it attempts to open the Places Downloads View. this becomes: "Helper function that opens the right download manager UI: the new Downloads View if it's enabled, the old toolkit UI otherwise." (feel free to fix my broken English) @@ +114,5 @@ > + { > + if (DownloadsCommon.useToolkitUI) { > + this._toolkitUI.show(aWindowContext, aID, aReason); > + return; > + } This should be moved back, as I said @@ +152,5 @@ > + // If we got here, then the browser.library.useNewDownloadsView pref > + // either didn't exist or was false, so just show the toolkit downloads > + // manager. > + this._toolkitUI.show(aWindowContext, aID, aReason); > + } MAybe we can reduce indentation a bit here, using some early return (pseudocode): if (!usePlacesView) { show_toolkit return } let organizer... if (organizer) { show organizer return } all-the-remaining

Thanks Marco! Comments addressed - here's the final patch.

Landed on mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/6bcdc46945a6