Add a 'Downloads' button to the Library panel

ASSIGNED
Assigned to

Status

()

Firefox
Toolbars and Customization
P1
normal
ASSIGNED
5 months ago
18 hours ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 2 bugs)

52 Branch
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 months ago
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.
(Assignee)

Updated

5 months ago
Blocks: 1354533

Updated

5 months ago
No longer blocks: 1354533
No longer depends on: 1354159

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]

Comment 1

2 months ago
Do pending downloads show up in this view?
Flags: needinfo?(abenson)

Comment 2

2 months ago
No, this should be a record of things already in the library. The specialized Download Menu can handle other download states.
Flags: needinfo?(abenson)

Comment 3

2 months ago
(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?
Flags: needinfo?(abenson)

Comment 4

2 months ago
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.
Flags: needinfo?(abenson)
(Assignee)

Updated

2 months ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
(Assignee)

Updated

2 months ago
Priority: P2 → P1

Updated

2 months ago
Iteration: 56.3 - Jul 24 → 56.1 - Jun 26

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
(Assignee)

Comment 5

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=041516cd87a48c5e87ffe3bdb2a0982f3aecebcb
(Assignee)

Comment 6

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f4317e299d97caf094816330af3e6b6c6662993
(Assignee)

Comment 7

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82cb9ff4da49f750e0c9ed26c2f146d3587e1884
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 months ago
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!!
Attachment #8883528 - Flags: review?(paolo.mozmail)
Attachment #8883528 - Flags: review?(gijskruitbosch+bugs)
Attachment #8883528 - Flags: feedback?(paolo.mozmail)
Attachment #8883528 - Flags: feedback?(gijskruitbosch+bugs)

Comment 10

2 months ago
For a start, bugfixes like the one in panelUI.js can land now in their own bug. Small, but it helps!
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8883528 - Flags: review?(paolo.mozmail)
Attachment #8883528 - Flags: review?(gijskruitbosch+bugs)
Attachment #8883528 - Flags: feedback?(paolo.mozmail)
Attachment #8883528 - Flags: feedback?(gijskruitbosch+bugs)

Comment 12

2 months ago
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.

Comment 13

2 months ago
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?
Flags: needinfo?(mdeboer)

Comment 14

2 months ago
(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.)
(Assignee)

Comment 15

2 months ago
(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!
Flags: needinfo?(mdeboer)

Comment 16

2 months ago
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...
Attachment #8883528 - Flags: feedback?(gijskruitbosch+bugs)

Comment 17

2 months ago
mozreview-review
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.
(Assignee)

Updated

2 months ago
Depends on: 1378790
(Assignee)

Updated

2 months ago
Depends on: 1378794

Comment 18

2 months ago
mozreview-review
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.

Updated

2 months ago
Attachment #8883528 - Flags: feedback?(paolo.mozmail)

Updated

a month ago
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

a month ago
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.

Comment 26

a month ago
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.

Comment 27

a month ago
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.
(Assignee)

Comment 28

a month ago
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.
Flags: needinfo?(paolo.mozmail)

Comment 29

a month ago
(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.
Flags: needinfo?(paolo.mozmail)

Updated

a month ago
Depends on: 1381411
(Assignee)

Comment 30

a month ago
Not working on this atm, until bug 1381411 is implemented.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 56.3 - Jul 24 → ---

Updated

a month ago
Priority: P1 → P2

Comment 31

a month ago
mozreview-review
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.
Attachment #8885212 - Flags: review?(paolo.mozmail)

Comment 32

a month ago
mozreview-review
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.
Attachment #8885211 - Flags: review?(paolo.mozmail)

Comment 33

a month ago
mozreview-review
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.
Attachment #8885210 - Flags: review?(paolo.mozmail)

Comment 34

a month ago
mozreview-review
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.
Attachment #8885209 - Flags: review?(paolo.mozmail)

Comment 35

a month ago
mozreview-review
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.
Attachment #8885208 - Flags: review?(paolo.mozmail)

Comment 36

a month ago
mozreview-review
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.
Attachment #8883528 - Flags: review?(paolo.mozmail)

Comment 37

a month ago
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.

Comment 38

20 days ago
Just a heads up that bug 1381411 is landing.
Flags: needinfo?(mdeboer)
Blocks: 1387512
(Assignee)

Updated

15 days ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)

Updated

15 days ago
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
(Assignee)

Updated

10 days ago
Attachment #8883528 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8885208 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8885209 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8885210 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8885211 - Attachment is obsolete: true
(Assignee)

Updated

10 days ago
Attachment #8885212 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 40

10 days ago
mozreview-review
Comment on attachment 8896952 [details]
Bug 1354532 - 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).
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29

Comment 41

9 days ago
mozreview-review
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.

::: 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?

Updated

9 days ago
Duplicate of this bug: 1390201

Comment 43

9 days ago
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.
Flags: needinfo?(mdeboer)
(Assignee)

Comment 44

9 days ago
(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! ;)
Flags: needinfo?(mdeboer) → needinfo?(bbell)
Comment hidden (mozreview-request)
(Assignee)

Comment 46

9 days ago
mozreview-review-reply
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

> 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.

Comment 47

9 days ago
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.
Flags: needinfo?(bbell)
(Assignee)

Comment 48

9 days ago
(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.
(Assignee)

Comment 49

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

Comment 50

8 days ago
(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.

Comment 51

8 days ago
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);

Comment 52

8 days ago
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 53

8 days ago
Comment on attachment 8896952 [details]
Bug 1354532 - Implement a new Downloads view as part of the Library widget.

Clearing review for now since there are a few revisions upcoming.
Attachment #8896952 - Flags: review?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 55

8 days ago
(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!

Comment 56

8 days ago
(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 :-)

Comment 57

7 days ago
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.
(Assignee)

Comment 58

6 days ago
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.

Comment 59

6 days ago
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.
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request)

Comment 61

6 days ago
Weirdly, retrying a failed download causes two new downloads to be created.
(Assignee)

Comment 62

6 days ago
(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...
(Assignee)

Updated

6 days ago
Flags: needinfo?(mdeboer)
(Assignee)

Comment 63

6 days ago
(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?
(Assignee)

Updated

6 days ago
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 64

6 days ago
(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 65

6 days ago
mozreview-review
Comment on attachment 8896952 [details]
Bug 1354532 - 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?

Comment 66

6 days ago
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.

Comment 67

6 days ago
(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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 68

6 days ago
(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.
(Assignee)

Comment 69

6 days ago
(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 70

6 days ago
mozreview-review
Comment on attachment 8896952 [details]
Bug 1354532 - 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.
Attachment #8896952 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

6 days ago
Flags: needinfo?(abenson)

Comment 71

6 days ago
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 72

6 days ago
mozreview-review
Comment on attachment 8896952 [details]
Bug 1354532 - 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.

Comment 73

6 days ago
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.

Comment 74

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

Comment 75

6 days ago
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.
Flags: needinfo?(abenson)

Updated

18 hours ago
Duplicate of this bug: 1393049
You need to log in before you can comment on or make changes to this bug.