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

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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

Comment 1

[KM Lee [:rexboy] (inactivate after Sep. 22)]

s/some odes/some old codes/


Sorry for the typo :-/

Comment 2

[:Paolo Amadini]

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

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

Comment 4

[Sam Foster [:sfoster] (he/him)]

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

Comment 5

[:Paolo Amadini]

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

Comment 6

[Sam Foster [:sfoster] (he/him)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58261c43b4bdb0f21bd5c0305ab9b95464fd2b62&selectedJob=96367276

Comment 7

[Sam Foster [:sfoster] (he/him)]

(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 8

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1329109 - remove unused time and progressbar code from downloads toolbar icon.

* Adopted from patch https://bug1329109.bmoattachments.org/attachment.cgi?id=8824349 by :rexboy
* remove unit test for formatTimeLeft which is gone

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

Comment 9

[:Paolo Amadini]

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.

Comment 10

[Sam Foster [:sfoster] (he/him)]

(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 11

[Sam Foster [:sfoster] (he/him)]

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

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

Comment 12

[Sam Foster [:sfoster] (he/him)]

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

[:Paolo Amadini]

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

Comment 14

[Sam Foster [:sfoster] (he/him)]

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

Comment 15

[Sam Foster [:sfoster] (he/him)]

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

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

Comment 16

[:Paolo Amadini]

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

Comment 17

[Sam Foster [:sfoster] (he/him)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136252/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51f1e5035fe3
remove unused time and progressbar code from downloads toolbar icon. r=Paolo

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51f1e5035fe3