Last Comment Bug 1329109 - Remove unused codes for download progressbar and remaining time display on download toolbar icon
: Remove unused codes for download progressbar and remaining time display on do...
Status: RESOLVED FIXED
[CHE-MVP] [photon-animation]
:
Product: Firefox
Classification: Client Software
Component: Downloads Panel (show other bugs)
: unspecified
: Unspecified Unspecified
P1 normal (vote)
: Firefox 55
Assigned To: Sam Foster [:sfoster]
:
: :Paolo Amadini
Mentors:
Depends on: 1270006
Blocks: 1269956 1352065
  Show dependency treegraph
 
Reported: 2017-01-06 01:28 PST by KM Lee [:rexboy]
Modified: 2017-05-16 10:42 PDT (History)
18 users (show)
mmucci: qe‑verify-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 55.5 - May 15
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
WIP (11.13 KB, patch)
2017-01-06 01:28 PST, KM Lee [:rexboy]
no flags Details | Diff | Splinter Review
Bug 1329109 - remove unused time and progressbar code from downloads toolbar icon. (59 bytes, text/x-review-board-request)
2017-05-04 10:09 PDT, Sam Foster [:sfoster]
paolo.mozmail: review+
Details | Review

Description User image KM Lee [:rexboy] 2017-01-06 01:28:50 PST
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.
Comment 1 User image KM Lee [:rexboy] 2017-01-06 01:30:26 PST
s/some odes/some old codes/


Sorry for the typo :-/
Comment 2 User image :Paolo Amadini 2017-04-25 11:36:00 PDT
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 User image Sam Foster [:sfoster] 2017-04-25 18:43:24 PDT
(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 User image Sam Foster [:sfoster] 2017-05-02 10:43:24 PDT
(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 User image :Paolo Amadini 2017-05-02 11:05:14 PDT
(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 7 User image Sam Foster [:sfoster] 2017-05-04 09:57:14 PDT
(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 User image Sam Foster [:sfoster] 2017-05-04 10:09:38 PDT Comment hidden (mozreview-request)
Comment 9 User image :Paolo Amadini 2017-05-04 11:14:24 PDT
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 User image Sam Foster [:sfoster] 2017-05-04 11:32:08 PDT
(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 User image Sam Foster [:sfoster] 2017-05-05 17:34:25 PDT Comment hidden (mozreview-request)
Comment 12 User image Sam Foster [:sfoster] 2017-05-05 17:40:11 PDT
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 User image :Paolo Amadini 2017-05-08 04:34:26 PDT
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 User image Sam Foster [:sfoster] 2017-05-08 07:00:03 PDT
> 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 User image Sam Foster [:sfoster] 2017-05-08 12:42:17 PDT Comment hidden (mozreview-request)
Comment 16 User image :Paolo Amadini 2017-05-08 13:49:00 PDT
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 User image Sam Foster [:sfoster] 2017-05-08 14:41:29 PDT Comment hidden (mozreview-request)
Comment 18 User image Pulsebot 2017-05-08 16:09:05 PDT
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 User image Carsten Book [:Tomcat] 2017-05-09 05:49:54 PDT
https://hg.mozilla.org/mozilla-central/rev/51f1e5035fe3

Note You need to log in before you can comment on or make changes to this bug.