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

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Downloads Panel
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Paolo, Assigned: Paolo, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 47
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

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

Attachments

(5 attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Note that having a build on more than one platform (Windows, Linux, or Mac) is recommended to test the theme changes.
Mentor: paolo.mozmail@amadzone.org
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [diamond][lang=js]
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?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 3

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

Updated

2 years ago
Flags: qe-verify?
Whiteboard: [diamond][lang=js] → [diamond][lang=js] [fxprivacy]
(Assignee)

Updated

2 years ago
Flags: qe-verify? → qe-verify-

Updated

2 years ago
Priority: -- → P3

Updated

2 years ago
Priority: P3 → P2

Updated

2 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 47.1 - Feb 8
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Depends on: 1244473

Updated

2 years ago
Iteration: 47.1 - Feb 8 → ---
Priority: P1 → P2
Whiteboard: [diamond][lang=js] [fxprivacy] → [diamond][lang=js] [fxprivacy] [blocked]
(Assignee)

Updated

2 years ago
Whiteboard: [diamond][lang=js] [fxprivacy] [blocked] → [diamond][lang=js] [fxprivacy]
(Assignee)

Comment 4

2 years ago
Actually, I'm working on this right now, so clearing the whiteboard tags.
Priority: P2 → P1
Whiteboard: [diamond][lang=js] [fxprivacy] → [fxprivacy]
(Assignee)

Comment 5

2 years ago
Created attachment 8716296 [details]
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/
(Assignee)

Comment 6

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

Updated

2 years ago
Iteration: --- → 47.1 - Feb 8

Updated

2 years ago
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
(Assignee)

Comment 7

2 years ago
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/
Attachment #8716296 - Attachment description: MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. → MozReview Request: Bug 1117145 - Part 1 - Unify the functional CSS files for the Downloads Panel and the Library. r=jaws
Attachment #8716296 - Flags: review?(jaws)
(Assignee)

Comment 8

2 years ago
Created attachment 8719290 [details]
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/
Attachment #8719290 - Flags: review?(jaws)
(Assignee)

Comment 9

2 years ago
Created attachment 8719291 [details]
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/
Attachment #8719291 - Flags: review?(jaws)
(Assignee)

Comment 10

2 years ago
Created attachment 8719292 [details]
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/
Attachment #8719292 - Flags: review?(jaws)
(Assignee)

Comment 11

2 years ago
Created attachment 8719293 [details]
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/
Attachment #8719293 - Flags: review?(jaws)
(Assignee)

Comment 12

2 years ago
Marco, let me know if you want to provide feedback on the patches. I've requested review from Jared.
Flags: needinfo?(mak77)
I'll take a look when I can, don't block on me since I have far too many requests to handle already.
(Assignee)

Updated

2 years ago
Flags: needinfo?(mak77)
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
Attachment #8719293 - Flags: review?(jaws) → review+
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?
Attachment #8719292 - Flags: review?(jaws) → review+
Attachment #8719291 - Flags: review?(jaws) → review+
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
Attachment #8719290 - Flags: review?(jaws) → review+
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 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
Attachment #8716296 - Flags: review?(jaws) → review+

Updated

2 years ago
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7

Comment 19

2 years ago
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

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

Comment 21

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

2 years ago
Depends on: 1279329
Depends on: 1280799

Updated

2 years ago
Depends on: 1276973
You need to log in before you can comment on or make changes to this bug.