1117145 - Unify the "download.xml" binding between the Panel and the Library

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[:Paolo Amadini]

The XBL binding used to display download items is different for the Downloads Panel and the Downloads View, but actually quite similar in structure.

Unfortunately, unifying the two bindings is not that easy because the exact structure is used in theme CSS, which requires lots of testing.

But now we need to modify the XUL structure of the element anyways, in order to implement the new state for downloads that can be unblocked. This new state should work the same way from all views, so it may be worth looking into using the same base CSS first.

This work can probably start on top of bug 1117141.

At the same time or maybe just after the unification, it may be worth looking into removing the "state" attribute completely, and simplify the controlling CSS, that is currently difficult to understand.

Comment 1

[:Paolo Amadini]

Note that having a build on more than one platform (Windows, Linux, or Mac) is recommended to test the theme changes.

Comment 2

[Abhishek Bhattacharya [:babhishek21]]

Hey Paolo, I am interested.(Actually came from bug 936973) I was wondering if this is on high priority/tight schedule? Moreover where should one start?

Comment 3

[:Paolo Amadini]

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #2)
> Hey Paolo, I am interested.(Actually came from bug 936973) I was wondering
> if this is on high priority/tight schedule?

Thanks for your interest!

With regard to priority, we think this bug is important and can lead to visible improvements to the Downloads Panel and Library window, but the visual design here hasn't been prioritized as one of the immediate responsibilities of the team. This means it is a good opportunity, as it doesn't have a tight schedule, but keep in mind that turnaround time might be slower than usual as I devote attention to higher priorities.

> Moreover where should one start?

Take a look at "browser/components/downloads/content/download.xml". There are two bindings there, and in the end we want to get rid of the "full-ui" one.

We can work in steps. The two bindings have a slightly different structure from each other, for example the full-ui one doesn't have a <xul:stack> around the buttons. The first step could be to update the full-ui binding to use a stack as well, and update the relevant CSS so that the rules still match the elements.

Comment 4

[:Paolo Amadini]

Actually, I'm working on this right now, so clearing the whiteboard tags.

Comment 5

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws

Review commit: https://reviewboard.mozilla.org/r/33779/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33779/

Comment 6

[:Paolo Amadini]

MozReview currently doesn't show the contents of the removed files, but the changeset above removed duplication by having one CSS file instead of three with repeated rules to control the visibility of the elements inside download items.

This first pass also unifies how the buttons are handled, using "visibility: hidden" inside a stack element instead of "display: none" in the Library.

Comment 7

[:Paolo Amadini]

Comment on attachment 8716296 [details]
MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33779/diff/1-2/

Comment 8

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1117145 - Part 2 - Unify DownloadsViewItem and DownloadsViewItemController. r=jaws

Review commit: https://reviewboard.mozilla.org/r/34921/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34921/

Comment 9

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1117145 - Part 3 - Share the download item binding between the Panel and the Library. r=jaws

Review commit: https://reviewboard.mozilla.org/r/34923/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34923/

Comment 10

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1117145 - Part 4 - Unify how commands are handled in the Downloads Panel and the Library. r=jaws

Review commit: https://reviewboard.mozilla.org/r/34925/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34925/

Comment 11

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1117145 - Part 5 - Share the implementation of some of the download item commands. r=jaws

Review commit: https://reviewboard.mozilla.org/r/34927/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34927/

Comment 12

[:Paolo Amadini]

Marco, let me know if you want to provide feedback on the patches. I've requested review from Jared.

Comment 13

[Marco Bonardo [:mak]]

I'll take a look when I can, don't block on me since I have far too many requests to handle already.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8719293 [details]
MozReview Request: Bug 1117145 - Part 5 - Share the implementation of some of the download item commands. r=jaws

https://reviewboard.mozilla.org/r/34927/#review31861

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8719292 [details]
MozReview Request: Bug 1117145 - Part 4 - Unify how commands are handled in the Downloads Panel and the Library. r=jaws

https://reviewboard.mozilla.org/r/34925/#review31583

::: browser/components/downloads/content/allDownloadsViewOverlay.js:354
(Diff revision 1)
> -      case "downloadsCmd_open": {
> +      this[aCommand](this);

I don't see any commands that use the `this` argument, so it looks like this can be simplified to just `this[aCommand]()`.

Did you intend to write `this[aCommand].bind(this)();` ?

::: browser/components/downloads/content/allDownloadsViewOverlay.js:1467
(Diff revision 1)
> +      if (name.startsWith("cmd_") || name.startsWith("downloadsCmd_")) {

Lines 1185 and 1186 do this similar check, along with a couple places in downloads.js. Can you factor this out to a `isValidCommand()` function?

::: browser/components/downloads/content/downloads.js:1095
(Diff revision 1)
> -      this.commands[aCommand].apply(this);
> +      this[aCommand](this);

What is the point of passing `this` as the first argument here?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8719291 [details]
MozReview Request: Bug 1117145 - Part 3 - Share the download item binding between the Panel and the Library. r=jaws

https://reviewboard.mozilla.org/r/34923/#review31863

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8719290 [details]
MozReview Request: Bug 1117145 - Part 2 - Unify DownloadsViewItem and DownloadsViewItemController. r=jaws

https://reviewboard.mozilla.org/r/34921/#review31865

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8716296 [details]
MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws

https://reviewboard.mozilla.org/r/33779/#review31563

::: browser/components/downloads/content/downloads.css:17
(Diff revision 2)
> +{
> +  display: none;
> +}
> +
> +#downloadsSummary:not([inprogress]) > vbox > #downloadsSummaryProgress,
> +#downloadsSummary:not([inprogress]) > vbox > #downloadsSummaryDetails,
> +#downloadsFooter[showingsummary] > #downloadsHistory,
> +#downloadsFooter:not([showingsummary]) > #downloadsSummary
> +{
> +  display: none;
> +}

nit, place the opening bracket on the same line as the last selector

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9e22d8f88099
https://hg.mozilla.org/integration/fx-team/rev/5cec228f15f1
https://hg.mozilla.org/integration/fx-team/rev/8cac6a90ec0c
https://hg.mozilla.org/integration/fx-team/rev/231e7092dac2
https://hg.mozilla.org/integration/fx-team/rev/45f9877d7c4a

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3b3428c04743

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e22d8f88099
https://hg.mozilla.org/mozilla-central/rev/5cec228f15f1
https://hg.mozilla.org/mozilla-central/rev/8cac6a90ec0c
https://hg.mozilla.org/mozilla-central/rev/231e7092dac2
https://hg.mozilla.org/mozilla-central/rev/45f9877d7c4a
https://hg.mozilla.org/mozilla-central/rev/3b3428c04743