Remove unused codes for download progressbar and remaining time display on download toolbar icon

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Downloads Panel
P1
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: rexboy, Assigned: sfoster)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [CHE-MVP] [photon-animation])

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
Created attachment 8824349 [details] [diff] [review]
WIP

+++ This bug was initially created as a clone of Bug #1270006 +++

Bug 1270006 introduces a new design to download progress which removed the time display and progressbar by "filling" the arrow icon as icon.
When we finally landed and tested the new design, some odes for previous design becomes unused and need to be removed.
See to the WIP attachment for detail.
(Reporter)

Comment 1

7 months ago
s/some odes/some old codes/


Sorry for the typo :-/
Whiteboard: [CHE-MVP]

Updated

7 months ago
Priority: -- → P3
Blocks: 1269956

Updated

4 months ago
Assignee: nobody → sfoster
Blocks: 1349423
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify?
Priority: P3 → P1
Whiteboard: [CHE-MVP] → [CHE-MVP] [photon-animation]

Comment 2

4 months ago
For context, this was originally scheduled to land only after we released the new Downloads Indicator, in order to have a killswitch available for the feature. Given the Photon redesign though, which made this area of the code much more active than planned, it makes sense to remove the old code sooner.

Sam, I've seen you're assigned to the bug. Are you looking into removing the custom XUL binding of the Downloads Indicator as well? That would probably be a performance and maintainability improvement, but is definitely a separate changeset than just the removal of the unused code paths. If you're planning on investigating that, just be aware that the panel should be anchored to an element that doesn't animate, so I'm not sure we can do that with a standard toolbar button.
Flags: needinfo?(sfoster)
(Assignee)

Comment 3

4 months ago
(In reply to :Paolo Amadini from comment #2)
> For context, this was originally scheduled to land only after we released
> the new Downloads Indicator, in order to have a killswitch available for the
> feature. Given the Photon redesign though, which made this area of the code
> much more active than planned, it makes sense to remove the old code sooner.
> 
> Sam, I've seen you're assigned to the bug. Are you looking into removing the
> custom XUL binding of the Downloads Indicator as well? That would probably
> be a performance and maintainability improvement, but is definitely a
> separate changeset than just the removal of the unused code paths. If you're
> planning on investigating that, just be aware that the panel should be
> anchored to an element that doesn't animate, so I'm not sure we can do that
> with a standard toolbar button.

Yeah I figure now is a good time to cull dead code before we start layering on new stuff. If its a quick win, I can slip this in. If its jumping the gun, it's fine to leave it be for now. I've not started looking at the binding yet, I'm currently testing the WIP patch rebased against central. I'm working without any historical context on this, so any information you can give me is a big help.
Flags: needinfo?(sfoster)

Updated

4 months ago
Iteration: 55.4 - May 1 → 55.5 - May 15
(Assignee)

Comment 4

4 months ago
(In reply to :Paolo Amadini from comment #2)
> For context, this was originally scheduled to land only after we released
> the new Downloads Indicator, in order to have a killswitch available for the
> feature. Given the Photon redesign though, which made this area of the code
> much more active than planned, it makes sense to remove the old code sooner.\

Paolo, can you confirm that this is good to remove at this point? Is there something else we are waiting for - my understanding is that the Downloads Indicator is already released. So, provided tests pass, this should be as good a time as any to land this patch. If I'm reading comment #2 correctly, you are suggesting this is sooner than you had planned. I'm not trying to rush things if things are still mid-flight or something.
Flags: needinfo?(paolo.mozmail)

Comment 5

4 months ago
(In reply to Sam Foster [:sfoster] from comment #4)
> Paolo, can you confirm that this is good to remove at this point?

Yes, I think so.

> my understanding is that the Downloads Indicator is already released.

The redesigned indicator is currently in Beta, we were originally going to wait for Release but I think we can proceed now.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 6

4 months ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58261c43b4bdb0f21bd5c0305ab9b95464fd2b62&selectedJob=96367276
(Assignee)

Comment 7

3 months ago
(In reply to Sam Foster [:sfoster] from comment #6)
> Try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=58261c43b4bdb0f21bd5c0305ab9b95464fd2b62&selectedJob=9
> 6367276

AFAICT that looks good - there's a few oranges but none look related so I'll request review.
Comment hidden (mozreview-request)

Comment 9

3 months ago
mozreview-review
Comment on attachment 8864578 [details]
Bug 1329109 - remove unused time and progressbar code from downloads toolbar icon.

https://reviewboard.mozilla.org/r/136252/#review139326

Thanks! The original patch is only a starting point though, because we can remove the arrowStyledIndicator killswitch assuming it will always be true:

https://dxr.mozilla.org/mozilla-central/rev/4a6a71f4aa22e4dc3961884ce505ce34bdd799a2/browser/components/downloads/DownloadsCommon.jsm#147-152

In addition to the JavaScript code, there are various CSS styles that will go away as a result of classes or attributes never being set. Strings like "shortTimeLeftSeconds" have to be removed as well from the localization files.
Attachment #8864578 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 10

3 months ago
(In reply to :Paolo Amadini from comment #9)
> Comment on attachment 8864578 [details]
> Bug 1329109 - remove unused time and progressbar code from downloads toolbar
> icon.
> 
> https://reviewboard.mozilla.org/r/136252/#review139326
> 
> Thanks! The original patch is only a starting point though

That's fair. I'll dig a bit deeper to see what else can go.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

3 months ago
Attachment 8864578 [details] now has a much more aggressive clean-up of the #downloads-indicator-progress-area and all its associated CSS, the strings and code for the time remaining display - including the unit test, as well as the arrowStyleIndicator and any logic using it. Hopefully not too aggressive :/. I've tested the patch against obvious scenarios but I'm not sure where all the corners are for this functionality so apologies in advance if I missed something.

Comment 13

3 months ago
mozreview-review
Comment on attachment 8864578 [details]
Bug 1329109 - remove unused time and progressbar code from downloads toolbar icon.

https://reviewboard.mozilla.org/r/136252/#review140010

::: commit-message-0b255:3
(Diff revision 2)
> +* Adopted from patch https://bug1329109.bmoattachments.org/attachment.cgi?id=8824349 by :rexboy
> +* Remove unit test for formatTimeLeft which is gone
> +* Remove shortTime* strings which are only used with the old progress indicator
> +* Remove arrowStyledIndicator, indicatorType and withProgress CSS class
> +* Remove #downloads-indicator-progress-area and its style rules
> +* Remove indicatorCount and indicatorProgress

At least for me, the extended commit message shouldn't just list the changes that can already be seen by reading the code. Here is a better commit message for this change:

This removes the arrowStyledIndicator killswitch and the associated ability for system add-ons to restore the previous design of the Downloads Indicator in the toolbar. The removal includes all the related code, styles, and strings. Based on the original patch by :rexboy.

::: browser/components/downloads/DownloadsCommon.jsm:1282
(Diff revision 2)
>      // Display the estimated time left, if present.
>      if (summary.rawTimeLeft == -1) {
>        // There are no downloads with a known time left.
>        this._lastRawTimeLeft = -1;
>        this._lastTimeLeft = -1;
> -      this._counter = "";
> +    } else if (this._lastRawTimeLeft != summary.rawTimeLeft) {

The entire time computation including the member variables and the _activeDownloads generator can actually go away.

::: browser/components/downloads/content/indicator.js:438
(Diff revision 2)
>      let widgetGroup = CustomizableUI.getWidget("downloads-button");
>      let inMenu = widgetGroup.areaType == CustomizableUI.TYPE_MENU_PANEL;
>  
>      // For arrow-Styled indicator, suppress success attention if we have
>      // progress in toolbar
> -    let suppressAttention = DownloadsCommon.arrowStyledIndicator && !inMenu &&
> +    let suppressAttention = inMenu &&

!inMenu
Attachment #8864578 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 14

3 months ago
> At least for me, the extended commit message shouldn't just list the changes
> that can already be seen by reading the code. Here is a better commit
> message for this change:

Yep, seems reasonable. 

> > -    let suppressAttention = DownloadsCommon.arrowStyledIndicator && !inMenu &&
> > +    let suppressAttention = inMenu &&
> 
> !inMenu

ugh, good catch!
(Assignee)

Updated

3 months ago
Blocks: 1352065
No longer blocks: 1349423
Comment hidden (mozreview-request)

Comment 16

3 months ago
mozreview-review
Comment on attachment 8864578 [details]
Bug 1329109 - remove unused time and progressbar code from downloads toolbar icon.

https://reviewboard.mozilla.org/r/136252/#review140294

Thanks!

::: commit-message-0b255:5
(Diff revision 3)
> +MozReview-Commit-ID: 6ceXwqDGxmu
> +MozReview-Commit-ID: 3MlDDSVJl6u

For some reason there are two MozReview Commit IDs...
Attachment #8864578 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)

Comment 18

3 months ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51f1e5035fe3
remove unused time and progressbar code from downloads toolbar icon. r=Paolo

Comment 19

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/51f1e5035fe3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

3 months ago
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.