Closed Bug 1329109 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Downloads Panel, defect, P1)

Product:
Firefox
Component:
Downloads Panel
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.5 - May 15
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rexboy, Assigned: sfoster)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

WIP
11.13 KB, patch
Details | Diff | Splinter Review
Bug 1329109 - remove unused time and progressbar code from downloads toolbar icon.
59 bytes, text/x-review-board-request
Paolo
: review+
Details
Attached patch WIP — — Splinter Review 
+++ 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.
s/some odes/some old codes/


Sorry for the typo :-/
Whiteboard: [CHE-MVP]
Priority: -- → P3
Blocks: 1269956
Assignee: nobody → sfoster
Blocks: photon-animation
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify?
Priority: P3 → P1
Whiteboard: [CHE-MVP] → [CHE-MVP] [photon-animation]
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)
(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)
Iteration: 55.4 - May 1 → 55.5 - May 15
(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)
(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)
(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 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)
(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.
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 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)
> 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!
Blocks: 1352065
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+
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
https://hg.mozilla.org/mozilla-central/rev/51f1e5035fe3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: