This button opens a subview when clicked, which contains: * A button called 'Open Downloads Folder' * A button called 'Clear Downloads' * A separator * A list of recently downloaded items. It is recommended to implement the complete subview in a separate bug, per-spec. The title of the subview reads 'Downloads', the same as the button label.

Do pending downloads show up in this view?

No, this should be a record of things already in the library. The specialized Download Menu can handle other download states.

(In reply to Aaron Benson from comment #2) > No, this should be a record of things already in the library. The > specialized Download Menu can handle other download states. So, just so we're 100% on the same page, both pending and canceled (and, I guess, blocked) downloads show up in the library (ie the downloads section of what shows up if you press accel-shift-b). It sounds like you want only successfully completed items to show up in this menu, is that right?

Oh then I probably jumped to conclusions there .. if we're already showing canceled and blocked then that's okay. But we definitely don't need to show pending.

Comment on attachment 8883528 [details] Bug 1354532 - Part 1 - Refactor all relevant parts in allDownloadsViewOverlay.js into reusable components in DownloadsViewUI.jsm. I'm looking for feedback now that I've got the whole thing feature complete. What would you think is a good separation of these patches for review? Thanks!!

For a start, bugfixes like the one in panelUI.js can land now in their own bug. Small, but it helps!

Also, when moving a lot of code between files, like DownloadsViewUI.BaseDownloadsPlacesView, it helps to have a changeset that does only the refactoring, without adding the code that implements the new functionality yet. For the <commandset id="downloadCommands">, can we include it from the components/downloads folder? This can be its own changeset too. Anyways, I'll take a look at the situation with this entire patch applied. It seems to me that everything has been moved to the right place, but I'll check to avoid having to move things again later.

I'm not sure if this is an issue, but when running Firefox in permanent private browsing mode, the downloads are not visible in this view, while they are visible in the Library window. Shouldn't these views work on the same data, except that in-progress downloads are filtered?

(I first noticed this when saving an "about:" page in normal mode, since it's not added to history. Maybe other schemes like "data:" have a similar issue in normal mode as well.)

(In reply to :Paolo Amadini from comment #13) > I'm not sure if this is an issue, but when running Firefox in permanent > private browsing mode, the downloads are not visible in this view, while > they are visible in the Library window. Shouldn't these views work on the > same data, except that in-progress downloads are filtered? Yeah, this view takes a History-first approach as in it only displays things that are also in the places DB. Which is what we want, but I haven't yet implemented the session download filtering yet. So at the moment it shows those as well. In the final patch(es), this will be different. WRT private browsing mode, I haven't checked that codepath yet and I believe you right away that this is not yet working as expected. This needs to change as well, I think, because ppl will expect this to be shown in a Library-type view. I will take your advise in how to sub-divide the patches. Thanks for the feedback!

Comment on attachment 8883528 [details] Bug 1354532 - Part 1 - Refactor all relevant parts in allDownloadsViewOverlay.js into reusable components in DownloadsViewUI.jsm. I think Paolo's review here will be enough. If you think it's necessary, I'm happy to review specific bits that you want me to look at once the patches are split up a bit more. At the moment I would just spend a lot of time trying to grok copied-over downloads bits...

Comment on attachment 8883528 [details] Bug 1354532 - Part 1 - Refactor all relevant parts in allDownloadsViewOverlay.js into reusable components in DownloadsViewUI.jsm. https://reviewboard.mozilla.org/r/154458/#review159880 Still a preliminary review... ::: browser/components/customizableui/content/panelUI.css:64 (Diff revision 2) > margin: 0; > padding: 0; > } > > +.subviewbutton.download { > + -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-download"); This should definitely live somewhere in "browser/", possibly together with the current download bindings. ::: browser/components/customizableui/content/panelUI.css:67 (Diff revision 2) > +.subviewbutton.download[openLabel]:hover > .toolbarbutton-text > .status-full, > +.subviewbutton.download:-moz-any(:not(:hover),[buttonover]) > .toolbarbutton-text > .status-open, > +.subviewbutton.download:-moz-any(:not(:hover),:hover:not([buttonover])) > .toolbarbutton-text > .status-show, > +.subviewbutton.download:not(:hover) > .show-button, > +.subviewbutton.download > .show-button > .toolbarbutton-text { > + display: none; > +} We should try to have a state like the "file moved or missing" we have in the Downloads Panel, although we can probably work on it in a follow-up. ::: browser/components/customizableui/content/panelUI.inc.xul:759 (Diff revision 2) > + oncommand="DownloadsPanelView.show(this);"/> > + </vbox> > + </panelview> > + <panelview id="PanelUI-downloads" class="PanelUI-subView"> > + <vbox class="panel-subview-body"> > + <commandset id="downloadCommands"/> I think this is generally in its own section inside a <sets> element. ::: browser/components/customizableui/content/panelUI.inc.xul:760 (Diff revision 2) > + </vbox> > + </panelview> > + <panelview id="PanelUI-downloads" class="PanelUI-subView"> > + <vbox class="panel-subview-body"> > + <commandset id="downloadCommands"/> > + <toolbarbutton id="appMenu-library-downloads-open-button" I think -show-folder instead of -open is more consistent with the current naming. ::: browser/components/downloads/content/allDownloadsViewOverlay.xul:61 (Diff revision 2) > <commandset id="downloadCommands" > commandupdater="true" > events="focus,select,contextmenu" > - oncommandupdate="goUpdateDownloadCommands();"> > + oncommandupdate="DownloadsView.goUpdateCommands();"> > <command id="downloadsCmd_pauseResume" > - oncommand="goDoCommand('downloadsCmd_pauseResume')"/> > + oncommand="DownloadsView.goDoCommand('downloadsCmd_pauseResume')"/> I'll have to take a closer look later at command handling. Routing commands through the global overlay is a common pattern, and if we don't need it we may be able to use a different approach than commands. ::: browser/components/downloads/content/downloads.js:580 (Diff revision 2) > > +// DownloadsPanelView > + > +let gPanelViewInstances = new WeakMap(); > + > +class DownloadsPanelView extends DownloadsViewUI.BaseDownloadsPlacesView { We should definitely call this something different than DownloadsPanel. We could call it the DownloadsSubview, which would make this class the DownloadsSubviewView, awkward but correct... any better ideas welcome. Also, I would prefer if as much as possible of the code that is specific to the subview is moved to its own file. We could consider renaming downloads.js to downloadsPanel.js.

Comment on attachment 8883528 [details] Bug 1354532 - Part 1 - Refactor all relevant parts in allDownloadsViewOverlay.js into reusable components in DownloadsViewUI.jsm. https://reviewboard.mozilla.org/r/154458/#review160664 ::: browser/components/downloads/DownloadsViewUI.jsm:51 (Diff revision 2) > +this.DownloadsViewUI.BaseDownloadsPlacesView = class { > + constructor(window) { > + this.init(window); > + } This is going in the right direction, separating the data component that merges session downloads and history downloads, using the HistoryDownload back-end type and keeping the annotations cache. However, this is also serving a view role, meaning for example that for every window we end up duplicating the cache and the mixing logic. We should better separate the two, possibly placing the data handling parts in DownloadsCommon or a separate module, and we definitely need to move the code around in smaller steps to make review easier.

I wanted to mention that I discussed a couple of alternatives for the architecture with Mike on Tuesday, and we left with the intention of us taking some more time to look into this. I've reviewed the alternatives, and my conclusion is that we should work on an architecture that has a better separation of the data and the view behavior. The capability to use history downloads should be added to Toolkit at the Downloads API level, exposing the data from the Places back-end through a Downloads-API-like interface. This model would also be much easier to write regression tests for. We already have most of the pieces in place in allDownloadsViewOverlay.js. In fact, WebExtensions are going to need this interface for bug 1255507. If we went the way of the front-end code using the Places interfaces directly, the WebExtensions team would have to reimplement all the historical and session mixup logic again, with obvious maintainability issues. We don't need full isolation at first, and we can expose some Places operations to the consumers of the Downloads API if it ends up simplifying the code. We'll probably talk some more on how to proceed with the implementation. A good thing to note is that, thanks to the eventual interface being similar, it should be quite possible to work on the front-end and back-end pieces in parallel. In other words, it should be possible to work on a subview connected to current downloads, that can later be switched to historical downloads by swapping the back-end. I'll note some of the issues we discussed with the current code in review comments, mostly to have them on record.

Just noting how many of the Places interfaces are not used by Downloads: https://dxr.mozilla.org/mozilla-central/rev/03bcd6d65af62c5e60a0ab9247ccce43885e707b/browser/components/downloads/content/allDownloadsViewOverlay.js#1034-1045 The remaining ones can be abstracted through the existing HistoryDownload object and a DownloadList.

For the front-end, a lot of the code that handles the Places interfaces is also not used in practice for Downloads: https://dxr.mozilla.org/mozilla-central/rev/03bcd6d65af62c5e60a0ab9247ccce43885e707b/browser/components/places/content/browserPlacesViews.js#185-227,268-273,329-337,343-367,386-391,400,414-575,596-614,636-724,784-907 And we're also using a different XML binding to display the results. We may be able to refactor the code that is actually shared into a base class, if it ends up making sense.

I think I agree - even though it results in quite a bit of additional work for me. Sorry I couldn't be reached yesterday, I had to attend a funeral. However, I'm not sure what the exact architecture would be for this proper separation and you seem to have one. I'm not an immediate fan of MVC and that concept has been muddled in gray-ness for many years now. I'm not a fan of names, but proper architecture. I'm not really interested in having an interim solution working off the session downloads backend during the interim, because that'll only be confusing to Nightly users. I think the frontend patches in here are already quite good, so the emphasis and blocking factor really is the refactor. Now, enough of these non-motivating comments! What it really comes down to is this: Would it be possible for you to pencil down a plan that I can execute? Or is this something you'd rather take on yourself and this work should depend on? Like a bit of a diagram that says and explains 'Status Quo' --> 'Where we wanna be'. I'd like to get to a point where I can give an estimation of how much work/ effort this is going to be and perhaps even how much time this might take.

(In reply to Mike de Boer [:mikedeboer] from comment #28) > I'm not really interested in having an interim solution working off the > session downloads backend during the interim, because that'll only be > confusing to Nightly users. Yeah, to be clear the front-end code wasn't meant to land before the back-end is ready, but I wanted to note that prototyping without having to wait for the back-end code is a possibility. > Would it be possible for you to pencil down a plan that I can execute? Or is > this something you'd rather take on yourself and this work should depend on? I'm now working on a new DownloadHistoryList object in the Downloads API, with the usual addView / onDownloadAdded interface, but possibly a different kind of Download object that contains the data portion of the current history download element shell. This isn't a lot of work, so I'll file a separate bug that I'll take. We should probably talk about how we can architect the front-end pieces linked to this interface.

Not working on this atm, until bug 1381411 is implemented.

Comment on attachment 8885212 [details] Bug 1354532 - Part 6 - Make sure we're using the public session downloads data in the Library panel subview. https://reviewboard.mozilla.org/r/156094/#review162922 ::: browser/components/downloads/DownloadsViewUI.jsm:72 (Diff revision 1) > > this._active = active; > > // Register as a downloads view. The places data will be initialized by > // the places setter. > - this._downloadsData = DownloadsCommon.getData(window.opener || window); > + this._downloadsData = DownloadsCommon.getData(window.opener || window, forcePublicData); We'll simplify this logic that uses argument passing by accessing the right view on past downloads, that is already linked to public session downloads.

Comment on attachment 8885211 [details] Bug 1354532 - Part 5 - Make sure to only show completed downloads in the Library panel subview. https://reviewboard.mozilla.org/r/156092/#review162924 Hiding elements in the menu is a simple and effective approach that we will use in the final version.

Comment on attachment 8885210 [details] Bug 1354532 - Part 4 - Move the Places metadata cache used by the Downloads library views to the DownloadsCommon singleton. https://reviewboard.mozilla.org/r/156090/#review162928 The metadata cache will be shared globally in the new DownloadHistory.jsm module in bug 1381411.

Comment on attachment 8885209 [details] Bug 1354532 - Part 3 - Add a new Library panel subview that lists all downloads that are not in progress. https://reviewboard.mozilla.org/r/156088/#review162930 ::: browser/components/downloads/DownloadsViewUI.jsm:1305 (Diff revision 1) > - return this.view._richlistbox.selectedItems.length == 1; > + return this.view._richlistbox ? this.view._richlistbox.selectedItems.length == 1 : > + this.view.selectedNodes.length == 1; This should definitely be implemented by two different view classes, rather than having "if" statements in each method. If there is code that can be shared, it should be moved to a base class. ::: browser/components/places/content/browserPlacesViews.js:1985 (Diff revision 1) > this._insertNewItem(this._resultNode.getChild(i), null); > } > } > }; > > -class PlacesPanelview extends PlacesViewBase { > +this.PlacesPanelview = class extends PlacesViewBase { As noted earlier, the base class for this view does not need to be linked to the Places interfaces.

Comment on attachment 8885208 [details] Bug 1354532 - Part 2 - Generalize the Downloads commands handling across panel and library window. https://reviewboard.mozilla.org/r/156086/#review162936 I haven't looked at command handling in detail yet, but we've noticed how most of the "downloadsCmd_" code paths don't necessarily have to be commands at all. One feature of command elements is that they can control the enabled or disabled state of items they are linked to automatically, but we have custom code that does this already, because the combination of the visibility and enabled state is different for each place of the user interface that can trigger the action. We still need to use commands for Copy and Delete because they are linked to global keyboard shortcuts and menu items, but we don't have to add new XUL elements for them.

Comment on attachment 8883528 [details] Bug 1354532 - Part 1 - Refactor all relevant parts in allDownloadsViewOverlay.js into reusable components in DownloadsViewUI.jsm. https://reviewboard.mozilla.org/r/154458/#review162940 This moves the data and view portions wholesale to the DownloadsViewUI module, resulting in some of the issues already mentioned in the previous comments. I'm working on separating these two aspects in bug 1381411.

Mike, there is a preliminary patch in bug 1381411. It's waiting for feedback, but I think you can probably take a look to see how it simplified the code in "allDownloadsViewOverlay.js". The majority of the remaining code is for the command handling.

Just a heads up that bug 1381411 is landing.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review173500 I'll try to have this reviewed by the end of the week. I'll post comments as I work on the review. ::: browser/base/content/browser.js:116 (Diff revision 1) > XPCOMUtils.defineLazyScriptGetter(this, ["setContextMenuContentData", > "openContextMenu", "nsContextMenu"], > "chrome://browser/content/nsContextMenu.js"); > XPCOMUtils.defineLazyScriptGetter(this, ["DownloadsPanel", > "DownloadsOverlayLoader", > + "DownloadsPanelSubView", We should call this DownloadsSubview or maybe even DownloadsHistorySubview. As both SubView and Subview are in use, we can use either variation here. This will avoid confusion with the completely different DownloadsPanel (although it also has its own unrelated DownloadsBlockedSubview).

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review173952 There's a glitch when going back to the main view. ::: browser/components/customizableui/content/panelUI.inc.xul:749 (Diff revision 1) > + <panelview id="PanelUI-downloads" class="PanelUI-subView"> > + <vbox class="panel-subview-body"> > + <commandset id="downloadCommands"/> > + <toolbarbutton id="appMenu-library-downloads-show-button" > + class="subviewbutton subviewbutton-iconic" > + label="Show Downloads Folder" Localization, also below. ::: browser/components/downloads/DownloadsPanelSubView.jsm:1 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ nit: use the latest version of the header ::: browser/components/downloads/DownloadsPanelSubView.jsm:11 (Diff revision 1) > + > +this.EXPORTED_SYMBOLS = [ > + "DownloadsPanelSubView", > +]; > + > +const { classes: Cc, interfaces: Ci, utils: Cu } = Components; nit: also Cr to use a standard line. ::: browser/components/downloads/DownloadsPanelSubView.jsm:31 (Diff revision 1) > +let gPanelViewInstances = new WeakMap(); > +const kEvents = ["ViewShowing", "ViewHiding", "click", "command"]; > +XPCOMUtils.defineLazyGetter(this, "kButtonLabels", () => { > + return { > + show: DownloadsCommon.strings[AppConstants.platform == "macosx" ? "showMacLabel" : "showLabel"], > + open: DownloadsCommon.strings.openFileLabel nit: comma at end of line for multi-line object literals ::: browser/components/downloads/DownloadsPanelSubView.jsm:37 (Diff revision 1) > + }; > +}); > + > +class DownloadsPanelSubView { > + constructor(event) { > + const document = this.document = event.target.ownerDocument missing semicolon ::: browser/components/downloads/DownloadsPanelSubView.jsm:37 (Diff revision 1) > + const document = this.document = event.target.ownerDocument > + const window = this.window = document.defaultView; nit: I think let is preferred over const ::: browser/components/downloads/DownloadsPanelSubView.jsm:71 (Diff revision 1) > + // Get the Download button out of the attention state since we're about to > + // view all downloads. > + DownloadsCommon.getIndicatorData(window).attention = DownloadsCommon.ATTENTION_NONE; We probably shouldn't do this here, because this view won't allow the user to handle temporarily blocked downloads. ::: browser/components/downloads/DownloadsPanelSubView.jsm:142 (Diff revision 1) > + if (this.searchTerm) { > + shell.element.hidden = !shell.matchesSearchTerm(this.searchTerm); > + } leftover ::: browser/components/downloads/DownloadsViewUI.jsm:41 (Diff revision 1) > + canClearDownloads(nodeContainer) { > + // Downloads can be cleared if there's at least one removable download in > + // the list (either a history download or a completed session download). > + // Because history downloads are always removable and are listed after the > + // session downloads, check from bottom to top. > + for (let elt = nodeContainer.lastChild; elt; elt = elt.previousSibling) { > + // Stopped, paused, and failed downloads with partial data are removed. > + let download = elt._shell.download; > + if (download.stopped && !(download.canceled && download.hasPartialData)) { > + return true; > + } > + } > + return false; > + } Looks like a method for a base class shared by the two history download views, since it has the knowledge of "_shell" and the list structure. There is also opportunity for sharing some of the code related to the batch fragment in a base class. I guess this may be complicated to do when mixing classes and traditional objects, so file a bug to have this sorted out, and reference it in a comment on this method. We may want to convert everything to use classes at some point, now that they support async methods. ::: browser/components/downloads/DownloadsViewUI.jsm:414 (Diff revision 1) > downloadsCmd_cancel() { > // This is the correct way to avoid race conditions when cancelling. > this.download.cancel().catch(() => {}); > this.download.removePartialData().catch(Cu.reportError); > }, nit: I'd keep all the downloadsCmd_ handlers together. Or is there any reason for the reordering?

I have a concern about the current design: it makes it very easy to clear all your download history accidentally with no way to undo the action.

(In reply to :Paolo Amadini from comment #43) > I have a concern about the current design: it makes it very easy to clear > all your download history accidentally with no way to undo the action. Well, I think that's a valid concern. Let's ask the one who invented it! ;)

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review173952 > Looks like a method for a base class shared by the two history download views, since it has the knowledge of "_shell" and the list structure. There is also opportunity for sharing some of the code related to the batch fragment in a base class. > > I guess this may be complicated to do when mixing classes and traditional objects, so file a bug to have this sorted out, and reference it in a comment on this method. We may want to convert everything to use classes at some point, now that they support async methods. I'll work on this in this bug as well - just wanted to show you the progress so far. > nit: I'd keep all the downloadsCmd_ handlers together. Or is there any reason for the reordering? Ah, yeah... there was supposed to be a alphabetical re-ordering of these items, but well, failed. :P Fixed now.

I agree that users can clear their downloads easily, and it deserves an undo option (ctrl+z / command+z), and possibly an visible undo button when initially cleared. However, clearing the downloads list with one button push has been the norm in the Library for maybe a decade. I think it's fine if we ship it like this, and fix it in a follow up bug.

(In reply to :Paolo Amadini from comment #41) > Comment on attachment 8896952 [details] > Bug 1354532 - Implement a new Downloads view as part of the Library widget. > > https://reviewboard.mozilla.org/r/168252/#review173952 > > There's a glitch when going back to the main view. I know about this issue and it's not caused by this patch - it also show the glitch for the Bookmarks panel, for example. I think it's a regression from bug 1382243, but I'm in the middle of investigating that.

Now I know for sure that bug 1382243 introduced the glitch.

(In reply to bbell from comment #47) > However, clearing the downloads list with one button push has been the norm > in the Library for maybe a decade. The difference is that the button in the Library window is normally shown far away from the position of the mouse pointer. Since the subview moves to overlap the main view, just clicking accidentally a second time with the cursor still in the vicinity has a chance to trigger the command. This happened to me for other submenu commands on a few occasions already, in a harmless way though.

Useful script for creating some completed and failed downloads using Unix file paths: for (let i = 0; i < 10; i++) { var s = Services.io.newURI("http://example.com/entry" + i); var t = NetUtil.newURI(new FileUtils.File("/tmp/entry" + i)); var startPRTime = Date.now() * 1000; Cc["@mozilla.org/browser/download-history;1"] .getService(Ci.nsIDownloadHistory) .addDownload(s, null, startPRTime, t); } (async () => { let list = await Downloads.getList(Downloads.PUBLIC); for (let i = 0; i < 10; i++) { let download = await Downloads.createDownload({ source: "http://example.com/download" + i, target: "/tmp/download" + i, }); download.start(); list.add(download); } })().catch(Cu.reportError);

A small issue is that Clear Downloads may be enabled even if you're not seeing any downloads in the list, for example because they all failed. Using the command will clear all downloads, including failed ones and temporarily blocked ones, then the menu item will be disabled. I don't think this is a blocking issue.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. Clearing review for now since there are a few revisions upcoming.

(In reply to :Paolo Amadini from comment #53) > Clearing review for now since there are a few revisions upcoming. Which suggestions? Anyways, I think I finished implementing a baseclass for the Library views, but I feel I can only share the clear-downloads logic, because the batch handling is too different between both. I applied the script you mentioned... do you mean to say that I should also list the failed downloads in this case? In case you meant that, I implemented it - including the retry-download logic to mirror the Library functionality. I hope you like it!

(In reply to Mike de Boer [:mikedeboer] from comment #55) > I applied the script you mentioned... do you mean to say that I should also > list the failed downloads in this case? In case you meant that, I > implemented it - including the retry-download logic to mirror the Library > functionality. Ah, I wasn't sure it was included in the design, probably was unspecified, but sounds like a useful feature :-)

I still see panel glitches, but I've rebased the patch on the latest mozilla-central, so it may be either an effect of doing that or a merge artifact. I'll retry tomorrow. In general, the patch looks good so I hope to have it reviewed soon.

Ough, I forgot to fix the panel-opening glitch that you see... the downloads data view is async, so I need to add a ViewShowing event blocker. Working on that atm, will not change the code much.

One thing I find confusing is that only public downloads are listed in private browsing windows, and all downloads started from private browsing mode are not shown. I think we should either show private downloads only, show both public and private downloads, or hide the button completely. Also, I haven't tested permanent private browsing mode, but looking at the code I guess this won't work as expected. For history we still show only public pages, but it makes more sense as you may want to load one of them again in private browsing mode. A similar case to what we could do here is the "Recently Closed Tabs" menu, that shows only private tabs in private windows.

Weirdly, retrying a failed download causes two new downloads to be created.

(In reply to :Paolo Amadini from comment #59) > One thing I find confusing is that only public downloads are listed in > private browsing windows, and all downloads started from private browsing > mode are not shown. I think we should either show private downloads only, > show both public and private downloads, or hide the button completely. Also, > I haven't tested permanent private browsing mode, but looking at the code I > guess this won't work as expected. OK - so in a private browsing window we'd want the Downloads.ALL list and in a public browsing window we'd want the Downloads.PUBLIC list? I mean, that'd make sense to make to have this view make sense in Private browsing mode. In other words; I don't think it's a good idea after all to show private downloads in a non-private window...

(In reply to :Paolo Amadini from comment #61) > Weirdly, retrying a failed download causes two new downloads to be created. Is there a directory watcher on the user's Downloads dir?

(In reply to Mike de Boer [:mikedeboer] from comment #62) > OK - so in a private browsing window we'd want the Downloads.ALL list and in > a public browsing window we'd want the Downloads.PUBLIC list? > I mean, that'd make sense to make to have this view make sense in Private > browsing mode. In other words; I don't think it's a good idea after all to > show private downloads in a non-private window... Wait, I'm wrong here: we don't show private downloads in a non-private window atm. OK. So I just need to get the Downloads.ALL list in private windows. On it!

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review175270 ::: browser/components/downloads/DownloadsSubview.jsm:405 (Diff revision 4) > + _updateState() { > + super._updateState(); > + this.element.setAttribute("label", this.element.getAttribute("displayName")); > + this.element.setAttribute("tooltiptext", this.element.getAttribute("fullStatus")); > + > + if (this.isCommandEnabled("downloadsCmd_show")) { This can still do main-thread I/O, so it should be avoided. I believe you should be able to use the download properties here, which is also simpler. ::: browser/components/downloads/DownloadsViewUI.jsm:114 (Diff revision 4) > + get browserWindow() { > + return RecentWindow.getMostRecentBrowserWindow(); > + }, > + Just call RecentWindow.getMostRecentBrowserWindow() directly. This makes it clearer that the result may vary or be null. ::: browser/components/downloads/content/downloadsOverlay.xul:198 (Diff revision 4) > + <toolbarbutton id="appMenu-library-downloads-clear-button" > + class="subviewbutton subviewbutton-iconic" > + disabled="true" > + label="&cmd.clearDownloads.label;" > + closemenu="none" > + oncommand="DownloadsSubview.onClearDownloads(this)"/> nit: semicolon ::: browser/themes/shared/customizableui/panelUI.inc.css:2082 (Diff revision 4) > + fill: #0a84ff; > + outline: 1px solid #0a84ff; Looks like these colors shouldn't be hardcoded, but I'm not sure what is the right solution here. ::: toolkit/content/widgets/toolbarbutton.xml:118 (Diff revision 4) > class="toolbarbutton-menu-dropmarker" xbl:inherits="disabled,label"/> > </content> > </binding> > + > + <binding id="toolbarbutton-download" > + extends="chrome://global/content/bindings/button.xml#menu-button-base"> Why is this extending the menu-button base binding?

I think the clickable area of the action buttons at a minimum should be expanded a bit. For what it's worth, this structure with a small button was the one used by the Downloads Panel before we redesigned it last year, so it seems a design regression to reintroduce this structure instead of using a separator and the entire area on the left for the action.

(In reply to Mike de Boer [:mikedeboer] from comment #63) > (In reply to :Paolo Amadini from comment #61) > > Weirdly, retrying a failed download causes two new downloads to be created. > > Is there a directory watcher on the user's Downloads dir? Not sure why you're asking, but I believe the issue might be with the onClick / onCommand events I'm looking at now.

(In reply to :Paolo Amadini from comment #67) > Not sure why you're asking, but I believe the issue might be with the > onClick / onCommand events I'm looking at now. Ah yes, I'm pretty sure it is. Will fix that.

(In reply to :Paolo Amadini from comment #66) > I think the clickable area of the action buttons at a minimum should be > expanded a bit. > > For what it's worth, this structure with a small button was the one used by > the Downloads Panel before we redesigned it last year, so it seems a design > regression to reintroduce this structure instead of using a separator and > the entire area on the left for the action. Aaron, what do you think? Should we reconsider the design here?

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review175284 DownloadsSubview.jsm is the remaining part I'll have to review, the rest looks good. ::: browser/components/downloads/DownloadsSubview.jsm:39 (Diff revision 4) > + let document = this.document = panelview.ownerDocument; > + let window = this.window = document.defaultView; Just use this.document and this.window, no need for the double assignment.

It seems to me we should have the "Clear Downloads" command either in the menu or in the context menu, but not both. If we keep it only in the context menu, we can avoid accidental deletions until we have the "undo" button. Also, the "Clear Recent History" command is able to delete downloads already, after displaying a confirmation dialog.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review175308 ::: browser/components/downloads/DownloadsSubview.jsm:374 (Diff revision 4) > + // Since the state changed, we may need to check the target file again. > + this._targetFileChecked = false; > + > + this._updateState(); > + > + DownloadsSubview.updateClearDownloads(this.element); If you need it, you can just initialize the Button objects with a property that links back to the associated view, so you can avoid the static methods on DownloadsSubview. However, in this case it seems to me you're calling the expensive updateClearDownloads function for every download that is added to the list, even during batch loading, so this should be fixed as well.

We should probably limit the number of history downloads we show by adjusting the query just like we do with page history. You can probably create a variant of DownloadHistory.getList to that effect. This will also let you use the Downloads.ALL list as the backing DownloadList for the DownloadHistoryList in private windows.

Also, the design includes a "Show All Downloads" button that is currently missing from the patch.

There's a clear hover state for the item and the "open in folder" option so I don't think it will cause problems. The downloads menu design would be too large to fit into this sub-panel so it's good to ship as-is.

(In reply to :Paolo Amadini from comment #71) > It seems to me we should have the "Clear Downloads" command either in the > menu or in the context menu, but not both. > > If we keep it only in the context menu, we can avoid accidental deletions > until we have the "undo" button. Also, the "Clear Recent History" command is > able to delete downloads already, after displaying a confirmation dialog. Aaron, I actually strongly agree with Paolo here.

I strongly agree as well.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review177882 Looks good! Nice simplification and performance improvement with the clear downloads change as well. I'll test the front-end bits soon. ::: browser/components/customizableui/content/panelUI.css:67 (Diff revision 5) > +.subviewbutton.download[openLabel]:hover > .toolbarbutton-text > .status-full, > +.subviewbutton.download[retryLabel][buttonover]:hover > .toolbarbutton-text > .status-full, > +.subviewbutton.download:-moz-any(:not(:hover),[buttonover]) > .toolbarbutton-text > .status-open, > +.subviewbutton.download:-moz-any(:not(:hover),:hover:not([buttonover]),:not([retryLabel])) > .toolbarbutton-text > .status-retry, > +.subviewbutton.download:-moz-any(:not(:hover),:hover:not([buttonover]),:not([exists])) > .toolbarbutton-text > .status-show, > +.subviewbutton.download:not(:hover) > .action-button, > +.subviewbutton.download > .action-button > .toolbarbutton-text { > + display: none; > +} > + This is very magical. Can you add comments and keep a structure similar to the one we use for the Downloads Panel items? https://dxr.mozilla.org/mozilla-central/rev/892c8916ba32b7733e06bfbfdd4083ffae3ca028/browser/themes/shared/downloads/downloads.inc.css#202-232 ::: browser/components/downloads/DownloadsSubview.jsm:69 (Diff revision 5) > + clearButton.previousSibling.hidden = true; > + } > + this.panelview.appendChild(contextMenu); > + this.container.setAttribute("context", this.context); > + > + this._downloadsData = DownloadsCommon.getData(this.window, true, true, true); It's so true! ;-) ::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:68 (Diff revision 5) > - getList() { > + getList(isPrivate = false, limit) { > if (!this._promiseList) { > - this._promiseList = Downloads.getList(Downloads.PUBLIC).then(list => { > - return new DownloadHistoryList(list, HISTORY_PLACES_QUERY); > + this._promiseList = Downloads.getList(isPrivate ? Downloads.ALL : Downloads.PUBLIC) > + .then(list => { > + let query = HISTORY_PLACES_QUERY + (limit ? "&maxResults=" + limit : ""); > + return new DownloadHistoryList(list, query); > - }); > + }); > } > > return this._promiseList; > }, > _promiseList: null, You actually need to keep a map between the options and the returned lists, similarly to what Downloads.getList does. Also, a better API would be: DownloadHistory.getList({ type: Downloads.PUBLIC, maxHistoryResults: 42, }); Both options would be optional. This needs regression tests, so it's worth filing a separate bug.

(In reply to :Paolo Amadini from comment #80) > This is very magical. Can you add comments and keep a structure similar to > the one we use for the Downloads Panel items? > > https://dxr.mozilla.org/mozilla-central/rev/ > 892c8916ba32b7733e06bfbfdd4083ffae3ca028/browser/themes/shared/downloads/ > downloads.inc.css#202-232 Sure! I compacted the selectors to be like this, which seemed to perhaps be more performant. But I can just as well write them out (even though we'll most likely never touch this code again, because it's so tightly coupled with the design & nodes structure. > You actually need to keep a map between the options and the returned lists, > similarly to what Downloads.getList does. Also, a better API would be: > > DownloadHistory.getList({ > type: Downloads.PUBLIC, > maxHistoryResults: 42, > }); > > Both options would be optional. This needs regression tests, so it's worth > filing a separate bug. Ah, of course! On it right away. I was planning on a follow-up to write regression tests for the whole darn thing, not only this part.

(In reply to Mike de Boer [:mikedeboer] from comment #81) > Ah, of course! On it right away. I was planning on a follow-up to write > regression tests for the whole darn thing, not only this part. Yeah, let's add the easy tests in the back-end now, and leave the massive new front-end tests for the follow-up.

(In reply to :Paolo Amadini from comment #82) > Yeah, let's add the easy tests in the back-end now, and leave the massive > new front-end tests for the follow-up. I'll be adding these in a separate commit.

(In reply to Mike de Boer [:mikedeboer] from comment #84) > I'll be adding these in a separate commit. Cool, please move the jsdownloads changes to the same commit.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review177990 Hm, it doesn't seem the context menu is working? ::: browser/components/downloads/DownloadsSubview.jsm:347 (Diff revision 7) > + /** > + * Handle state changes of a download. > + */ > + onStateChanged() { > + // Since the state changed, we may need to check the target file again. > + this._targetFileChecked = false; > + > + this._updateState(); > + } > + > + /** > + * Handler method; invoked when any state attribute of a download changed. > + */ > + onChanged() { > + let newState = DownloadsCommon.stateOfDownload(this.download); > + if (this._downloadState !== newState) { > + this._downloadState = newState; > + this.onStateChanged(); > + } else { > + this._updateState(); > + } > + > + // This cannot be placed within onStateChanged because > + // when a download goes from hasBlockedData to !hasBlockedData > + // it will still remain in the same state. > + this.element.classList.toggle("temporary-block", > + !!this.download.hasBlockedData); > + } There might be some leftovers related to the "file moved or missing" check, we should likely have a bug to implement that check at some point. Also, aren't we calling _updateState too frequently?

Comment on attachment 8901205 [details] Bug 1354532 - Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. https://reviewboard.mozilla.org/r/172674/#review177994 This actually looks like a potential footgun, I'd like a second opinion on whether we should have an about:config preference for this. If this is set to a high value like 100000, the performance of the browser would be severely affected, undoing our general performance efforts. I wouldn't do this unless we have a real need.

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. https://reviewboard.mozilla.org/r/172676/#review178000 Looks good, just waiting for the back-end tests.

(In reply to :Paolo Amadini from comment #89) > Hm, it doesn't seem the context menu is working? Clicking a Download item also doesn't work! The command event is never fired for these elements, including the context menu! Is this a recent regression? What's going on? > There might be some leftovers related to the "file moved or missing" check, > we should likely have a bug to implement that check at some point. I'm not sure about this; it sounds to me like you know more, so can you file that bug perhaps? > Also, aren't we calling _updateState too frequently? I don't know, this is mirroring the Library DownloadsView and quite necessary to follow the progress of an in progress download. But since we don't show that at all in this view, it's basically a no-op (even though the DOM node is updated, there's no visual change).

Comment on attachment 8901205 [details] Bug 1354532 - Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. Since Marco suggested this to be a pref before, I think it might be good to ask him for review.

Marco, see comment 90.

(In reply to Mike de Boer [:mikedeboer] from comment #92) > (In reply to :Paolo Amadini from comment #89) > > Hm, it doesn't seem the context menu is working? > > Clicking a Download item also doesn't work! The command event is never fired > for these elements, including the context menu! Is this a recent regression? > What's going on? In fact, when I click the secondary action button it _DOES_ dispatch a 'command' event and works like it should. I've been looking at this damn problem all day already and I can't figure it out. What on earth is going on here?!?!

Forget it, I'm not going to use the 'command' event anymore. Clicks work much more reliably.

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. https://reviewboard.mozilla.org/r/172676/#review179008 ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:239 (Diff revision 2) > + // Add a new Download to a private list. > + let privateList = await promiseNewList(true); > + sessionDownloadToAdd = { offset: NEXT_OFFSET + 5, inSession: true, succeeded: true }; > + await addTestDownload(sessionDownloadToAdd, privateList, true); > + await view.waitForExpected(); > + await privateView.waitForExpected(); This shows no difference between the public view and the private one. Is that a bug? Is the test code wrong? Do you have any tips/ advice I can follow?

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. https://reviewboard.mozilla.org/r/172676/#review179018 First pass review, if the parameter is not the issue we can look into what's happening later. ::: toolkit/components/jsdownloads/src/DownloadHistory.jsm:68 (Diff revisions 1 - 2) > * may return. > * @return {Promise} > * @resolves The requested DownloadHistoryList object. > * @rejects JavaScript exception. > */ > - getList({type = Downloads.PUBLIC, maxHistoryResults}) { > + getList({type = Downloads.PUBLIC, maxHistoryResults} = {}) { Hm, doesn't look like "type" is initialized by the default argument? ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:92 (Diff revision 2) > { offset: 35, canceled: true, hasPartialData: true, inSession: true }, > { offset: 55, succeeded: true, inSession: true }, > ]; > const NEXT_OFFSET = 60; > > - async function addTestDownload(properties) { > + async function addTestDownload(properties, list = publicList, isPrivate = false) { Just pass isPrivate as part of the properties, any extra ones are ignored by Downloads.create. ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:120 (Diff revision 2) > // Add all the test downloads to history. > let publicList = await promiseNewList(); You can move the list initialization earlier since you're using it as a default in the other function. Actually, once you called promiseNewList, you can use the Downloads.ALL list to have the download added to the right list based on whether it's private. ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:228 (Diff revision 2) > await publicList.add(await Downloads.createDownload(sessionDownloadToAdd)); > await view.waitForExpected(); > > + // Check that creating a second list that also shows private downloads works > + // as expected. > + let privateView = Object.create(view); Hm, you can just turn the object into a normal constructor function with prototype, or a class. ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:235 (Diff revision 2) > + let privateHistoryList = await DownloadHistory.getList({ type: Downloads.ALL, maxHistoryResults: 42 }); > + await privateHistoryList.addView(privateView); > + await privateView.waitForExpected(); > + > + // Add a new Download to a private list. > + let privateList = await promiseNewList(true); One thing to note is that promiseNewList resets the internal state of both the private and public lists, this may be unneeded here so you can await Downloads.getList(Downloads.PRIVATE).

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. https://reviewboard.mozilla.org/r/172676/#review179456 ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:92 (Diff revision 4) > - async function addTestDownload(properties) { > + async function addTestDownload(properties, list = publicList) { > properties.source = { url: httpUrl("source" + properties.offset) }; Just pass { isPrivate: properties.isPrivate } in the source object, then place... let publicList = await promiseNewList(); let allList = await Downloads.getList(Downloads.ALL); ...at the beginning of the test, and use allList.add in the function. ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:233 (Diff revision 4) > await view.waitForExpected(); > > + // Check that creating a second list that also shows private downloads works > + // as expected. > + let privateView = new TestView(); > + let privateHistoryList = await DownloadHistory.getList({ type: Downloads.ALL }); This should probably be named allHistoryList. ::: toolkit/components/jsdownloads/test/unit/test_DownloadHistory.js:252 (Diff revision 4) > + let privateView2 = new TestView(); > + let privateHistoryList2 = await DownloadHistory.getList({ type: Downloads.ALL, maxHistoryResults: 3 }); > + await privateHistoryList2.addView(privateView2); > + // Prepare the set of downloads to contain fewer history downloads. Yes, there's > + // an off-by-one error for the maxResults Places query parameter. > + testDownloads.splice(2, testDownloads.filter(d => !d.inSession).length - 2); Wait, shouldn't be the _oldest_ downloads the ones that disappear?

(In reply to :Paolo Amadini from comment #106) > Wait, shouldn't be the _oldest_ downloads the ones that disappear? Oeh, I didn't een realize that one... That, together with the off-by-one error, means that there are serious bugs in the `maxResults` query parameter handling on the Places side.

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. I did a few minor changes and I moved the patch to bug 1396581. I removed the maxHistoryResults option, leaving it for bug 1395663, because if we want to land a partial implementation, it's better to show all downloads than not showing the most recent ones. The DownloadsCommon changes contained here can be folded into the main patch.

Comment on attachment 8901205 [details] Bug 1354532 - Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. Marco can review this per comment 90: This actually looks like a potential footgun, I'd like a second opinion on whether we should have an about:config preference for this. If this is set to a high value like 100000, the performance of the browser would be severely affected, undoing our general performance efforts. I wouldn't do this unless we have a real need.

By the way, if Marco still thinks it should be done, you could move part 2 to a separate bug and just use a hardcoded number for the Downloads view if needed. Anyways, until bug 1395663 lands, this will not be relevant for this bug.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. This does look good, but I'm going to wait for the folded DownloadsCommon changes based on bug 1396581 to test the final implementation.

(In reply to :Paolo Amadini from comment #112) > Comment on attachment 8901205 [details] > Bug 1354532 - Part 2 - Introduce 'browser.menu.maxDynamicItems' pref that > holds the maximum number of items to be shown in a (panel)menu. > > Marco can review this per comment 90: > > This actually looks like a potential footgun, I'd like a second opinion on > whether we should have an about:config preference for this. If this is set > to a high value like 100000, the performance of the browser would be > severely affected, undoing our general performance efforts. I wouldn't do > this unless we have a real need. I concur. I would really like this to be a pref, but I think we should do that in a separate but and at a better time, when we're not overloaded by the release. I'm not sure why this is necessary for this specific bug? There is also the case of the user setting it to 0, or even worse to a negative number, and then the risk is we'll have no limit at all. All in all, this needs more attention and thus should not just be rushed here.

Comment on attachment 8901205 [details] Bug 1354532 - Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. https://reviewboard.mozilla.org/r/172674/#review181066 Let's do this apart in a separate lower priority bug. The pref should have a better name and the getter should check input and impose good min and max limits.

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. https://reviewboard.mozilla.org/r/172676/#review181752 ::: browser/components/customizableui/PanelMultiView.jsm:738 (Diff revision 6) > } > > let oldSibling = viewNode.nextSibling || null; > this._offscreenViewStack.appendChild(viewNode); > > - this.window.addEventListener("MozAfterPaint", () => { > + BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => { You should wait on the returned Promise: BrowserUtils.promiseLayoutFlushed(this.document, "layout").then(() => {

Comment on attachment 8901205 [details] Bug 1354532 - Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. https://reviewboard.mozilla.org/r/172674/#review181756 ::: browser/components/downloads/DownloadsCommon.jsm:81 (Diff revision 5) > otherDownloads3: true > }; > > const kPartialDownloadSuffix = ".part"; > > +const kMaxDynamicItems = 42; Hm, maybe kMaxHistoryResultsForLimitedView or something? ::: browser/components/downloads/DownloadsCommon.jsm:202 (Diff revision 5) > * True to include history downloads when the window is public. > - */ > - getData(window, history = false) { > - if (PrivateBrowsingUtils.isContentWindowPrivate(window)) { > + * @param [optional] privateAll > + * Whether to force the public downloads data to be returned together > + * with the private downloads data for a private window. > + */ > + getData(window, history = false, privateAll = false, limit = false) { limit => limited ::: browser/components/downloads/DownloadsCommon.jsm:647 (Diff revision 5) > * the main browser window until the autostart timeout elapses. > * > * This powers the DownloadsData, PrivateDownloadsData, and HistoryDownloadsData > * singleton objects. > */ > -function DownloadsDataCtor({ isPrivate, isHistory } = {}) { > +function DownloadsDataCtor({ isPrivate, isHistory, limit } = {}) { limit => maxHistoryResults

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review181754 Mainly we need to simplify or document the click event handling, for the rest this looks good to land! If minor issues with code or behavior come up later, we can fix them in follow-ups. This is a complex change so I'd expect some minor regressions anyways. ::: browser/components/customizableui/content/panelUI.inc.xul:723 (Diff revision 11) > label="&appMenuRemoteTabs.label;" > closemenu="none" > oncommand="PanelUI.showSubView('PanelUI-remotetabs', this)"/> > + <toolbarbutton id="appMenu-library-downloads-button" > + class="subviewbutton subviewbutton-iconic subviewbutton-nav" > + label="&downloads.label;" It occurred to me that me may want to use a different string, because in other languages the label for the menu might require changes. ::: browser/components/downloads/DownloadsSubview.jsm:146 (Diff revision 11) > + let element = this._viewItemsForDownloads.get(download).element; > + element.remove(); nit: no need for the intermediate variable ::: browser/components/downloads/DownloadsSubview.jsm:227 (Diff revision 11) > + if (button.hasAttribute("exists")) > + menu.setAttribute("exists", button.getAttribute("exists")); Should remove the attribute if the original one isn't set. ::: browser/components/downloads/DownloadsSubview.jsm:234 (Diff revision 11) > + menu.classList.toggle("temporary-block", button.classList.contains("temporary-block")); > + menu.querySelector("menuitem[command='downloadsCmd_clearDownloads'").disabled = > + !DownloadsSubview.canClearDownloads(button); > + // The menu anchorNode property is not available long enough to be used elsewhere, > + // so tack it another property name. > + menu.__anchorNode = button; nit: one underscore is sufficient Do we need to clear the value when the menu is closed so we don't keep a reference to an element that may be removed? ::: browser/components/downloads/DownloadsSubview.jsm:283 (Diff revision 11) > + command = button.getAttribute("command"); > + button = button.parentNode.__anchorNode; Hm, don't menuitems with a "command" attribute already get the "oncommand" handler from the <command> element? Is there anything to do here? ::: browser/components/downloads/DownloadsSubview.jsm:293 (Diff revision 11) > + if (!button || button == this.panelview || button.hasAttribute("oncommand")) > + return; In which cases doesn't this return? Looks like we could simplify something... maybe start by adding a comment to explain the various cases we may encounter, it might shed light on what is happening here.

Comment on attachment 8896952 [details] Bug 1354532 - Part 1 - Implement a new Downloads view as part of the Library widget. https://reviewboard.mozilla.org/r/168252/#review181808 You should just add a note to the event handling to say that we don't have a command dispatcher registered for this view, so we don't go through the goDoCommand path like we do for the other views.

Comment on attachment 8901205 [details] Bug 1354532 - Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. https://reviewboard.mozilla.org/r/172674/#review181810

Comment on attachment 8901206 [details] Bug 1354532 - Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. https://reviewboard.mozilla.org/r/172676/#review181812

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f1d18c741b2c Part 1 - Implement a new Downloads view as part of the Library widget. r=Paolo https://hg.mozilla.org/integration/autoland/rev/63909748b3ce Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. r=Paolo https://hg.mozilla.org/integration/autoland/rev/545712909b9b Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. r=Paolo

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/238f1dbcb600 Backed out changeset 545712909b9b https://hg.mozilla.org/integration/autoland/rev/48fa888e2007 Backed out changeset 63909748b3ce https://hg.mozilla.org/integration/autoland/rev/7b74ef52385d Backed out changeset f1d18c741b2c for eslint failure at browser/components/downloads/DownloadsCommon.jsm:671: Expected property shorthand. r=backout

Backed out for eslint failure at browser/components/downloads/DownloadsCommon.jsm:671: Expected property shorthand: https://hg.mozilla.org/integration/autoland/rev/7b74ef52385d68a1af6cf54463bc049ed4ba8308 https://hg.mozilla.org/integration/autoland/rev/48fa888e2007823bd91b8aca788cdb14844cd082 https://hg.mozilla.org/integration/autoland/rev/238f1dbcb6003dd97f0b10096f8f66591320c2f6 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=545712909b9be7cf10713169c1eac44636589769&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=128924315&repo=autoland > TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/downloads/DownloadsCommon.jsm:671:9 | Expected property shorthand. (object-shorthand)

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d59008966d79 Part 1 - Implement a new Downloads view as part of the Library widget. r=Paolo https://hg.mozilla.org/integration/autoland/rev/ca6349d6a8ae Part 2 - To facilitate the DownloadsSubview, add a 'LimitedHistoryDownloadsData' and 'LimitedPrivateHistoryDownloadData' dataview. r=Paolo https://hg.mozilla.org/integration/autoland/rev/3c2d759ce467 Part 3 - Ensure that the offscreen bounds check for panelviews does not cause temporary empty panels. r=Paolo

https://hg.mozilla.org/mozilla-central/rev/d59008966d79 https://hg.mozilla.org/mozilla-central/rev/ca6349d6a8ae https://hg.mozilla.org/mozilla-central/rev/3c2d759ce467

I can see 'Downloads' button added to the Library panel on latest nightly 57.0a1 in Ubuntu 16.04, 64 bit Build ID 20170912100139 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

I can see this feature implemented on latest nightly 57.0a1 (2017-09-12) in windows 10 (32bit) Build ID 20170912013600 Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0 [bugday-20170913]

As per Comment 139 and Comment 140, I am marking this bug as verified fixed.