812894 - Shorten time strings, reduce padding and margin in the panel to reintroduce per item download speed

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Bug 759397 removed the per-download rate indicator to reduce the clutter and noise in the downloads panel.

We decided instead to try displaying a global download rate. We need to sort out how this looks.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Wireframe

How do we feel about showing a global rate at the top of the list?

Not sure what we'd do if no downloads were in progress. Perhaps just display nothing. Or collapse the row.

Comment 2

[Marco Bonardo [:mak]]

I honestly don't see any other positioning possible...

Last night, instead of sleeping, I was thinking to a graphical vertical representation of speed in the right part of the Summary element. Basically the top limit is kept up-to-date to the maximum speed reached in the current session, while an animated bar fills up to the current speed. Something like a network icon with bars, similar to the mobile phones bars to be clear, but auto-adapting to the session.
There are cons though. This would give information on how much we are filling the available bandwidth, though wouldn't give any info on the global bandwidth (e.g. some other software in background may be slowing us down). Plus may go mad if the user doesn't restart the session when changing connection, like when traveling at the airport.

Comment 3

[Marco Bonardo [:mak]]

The first cons may be fixed by a tooltip with the effective speed, fwiw. Not sure how I'd fix the latter, unless the OS gives us the top limit info or we invalidate it on idle.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Stephen - do you have a position on this?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: A second wireframe

This second wireframe adds a visualization of the global download rate. It's got a sliding maximum, and invalidates when all downloads go idle.

I'm still not sure if I'm on the right track here...

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 684435 [details]
A second wireframe

I detect a lot of progress bars here :)

Honestly, if we are going to add a graphical indicator, I'd like it t not be a bar, it's too easy to confuse with a global progress bar.  My take on graphical representation (I repeat, if we really want it) is signal-strength bars

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: A third wireframe

Another idea:

This puts a vertical "strength" indicator along the right side of the panel. The current global rate could then be displayed in a tooltip on that indicator.

Comment 8

[:Paolo Amadini]

I think we should wait for Stephen's feedback before proceeding any further. I
suspect that, since the panel is designed only to surface status and provide
quickly actionable items, a graphical indication might be too much, and just be
confusing. In many cases, there isn't a specific action that can be taken based
on global download rate.

Comment 9

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: A fourth wireframe

Another idea can be simply by putting global download rate speed in the bottom info, of course if this info (time&size) represent state of all downloaded files, like it's currently in Downloads panel.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Virtual_ManPL [:Virtual] from comment #9)
> Another idea can be simply by putting global download rate speed in the
> bottom info, of course if this info (time&size) represent state of all
> downloaded files, like it's currently in Downloads panel.

that's not feasible due to space constraints, the same reason it was removed from the single items.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

So the idea of a global rate has been mulled about, and now tossed.

We're going to switch back to showing individual rates per download item.

However, in order to reduce the possibility of truncated strings in the "details" description of a download item, we're going to shorten some of the components of the strings considerably.

59 minutes, 59 seconds remaining - 1022 of 1023 bytes (999.4 bytes/sec)

Becomes

59 min 59 secs left - 1022/1023 B (999.4 B/s)

We could also try replacing "hr", "min", and "secs" with "h", "m", and "s". I'll start at the long one and see where we get.

Comment 12

[Marco Bonardo [:mak]]

I liked the readability of the current text, was very good for non-advanced users :(

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Are we sure we're OK copying all of this from mozapps/downloads/DownloadUtils.jsm?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Note that this patch relies on Mano's patch in bug 675902.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 688868 [details] [diff] [review]
WIP Patch 1

Review of attachment 688868 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +89,5 @@
> +
> +const kToolkitStrMap = {
> +  statusFormat: "statusFormat3",
> +  timePair: "timePair2",
> +  // Update timeSize in convertTimeUnits if changing the length of this array

which array? this is fake Map with 2 properties :)

@@ +92,5 @@
> +  timePair: "timePair2",
> +  // Update timeSize in convertTimeUnits if changing the length of this array
> +}
> +
> +const kCachedLastMaxSize = 10;

needs doc

@@ +157,5 @@
>    /**
>     * Returns the status-text and status-tip to be displayed for the given
>     * data item.
>     */
> +  getStatusTextAndTipForDataItem: function(aDataItem, aMediumLength) {

aUseAbbrFormat or something like that, this is not a length

@@ +174,5 @@
>      } else if (aDataItem.state == nsIDM.DOWNLOAD_DOWNLOADING) {
>        let newEstimatedSecondsLeft;
> +      if (aMediumLength) {
> +        [status, newEstimatedSecondsLeft] =
> +          DownloadsCommon.getMediumDownloadStatus(aDataItem.currBytes,

s/medium/Abbr/ basically everywhere in the patch

@@ +344,5 @@
> +
> +    return [timeLeft, aSeconds];
> +  },
> +
> +  convertTimeUnits: function DC_convertTimeUnits(aSecs)

modify toolkit's DownloadUtils.convertTimeUnits, add an optional argument to it that is a convertTimeUnitsUnits implementation, if it's not passed will use its own convertTimeUnitsUnits.

may we do the same for getTimeLeft?

::: browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ +56,5 @@
>  
> +mediumTimeLeftSeconds=sec;secs
> +mediumTimeLeftMinutes=min;mins
> +mediumTimeLeftHours=hr;hrs
> +mediumTimeLeftDays=day;days

I'd suggest s/medium/abbr/

@@ +65,5 @@
> +# LOCALIZATION NOTE (timeLeftDouble): %1$S time left; %2$S time left sub units
> +# example: 11 hours, 2 minutes remaining; 1 day, 22 hours remaining
> +timeLeftDouble=%1$S, %2$S left
> +timeFewSeconds=A few seconds left
> +timeUnknown=Unknown time left

As discussed on irc, this change from "remaining" to "left" is just done to short string a bit, but it opens a can of worms for localizers since "left" is even worse than "remaining" plural-wise, and they'll have to figure out the reason we changed from one to the other one, and eventually stick to the existing "remaining" translation. Lots of noise for nothing. Since the problem has already been solved on their side, let's avoid reopening it until we decide to properly support plurals in toolkit.

Please stick with toolkit strings.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 2

Oh yes - passing functions to DownloadUtils.jsm is *much* better. Far less repetition and copying. Thanks for that idea!

Getting pretty close - just a little polish here and there.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 3

Adding a few more hooks into DownloadUtils to allow us to swap some strings in and out. Getting pretty close to something reviewable.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Screenshots coming.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Clearing the needinfo - we have our spec from shorlander.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on OSX

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on Ubuntu

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on Windows 7 (through RDP)

Sorry for the low quality screenshot - I'm at home, and I'm remote-ing into my Windows box. That's why some of the panel shading might look off.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on Windows 7 (through RDP)

Now with more context!

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Mike Conley (:mconley) from comment #22)
> Created attachment 689782 [details]
> Patch on Windows 7 (through RDP)
> 
> Sorry for the low quality screenshot - I'm at home, and I'm remote-ing into
> my Windows box. That's why some of the panel shading might look off.

I have been wondering for some days, why doesn't the windows one has the focus ring. I used to get focus ring earlier, but from a couple of days, it is gone :(

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on Windows XP (through RDP)

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on OSX

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch on Ubuntu

Comment 28

[Mike Conley (:mconley) (:⚙️)]

(In reply to Girish Sharma [:Optimizer] from comment #24)
> (In reply to Mike Conley (:mconley) from comment #22)
> > Created attachment 689782 [details]
> > Patch on Windows 7 (through RDP)
> > 
> > Sorry for the low quality screenshot - I'm at home, and I'm remote-ing into
> > my Windows box. That's why some of the panel shading might look off.
> 
> I have been wondering for some days, why doesn't the windows one has the
> focus ring. I used to get focus ring earlier, but from a couple of days, it
> is gone :(

Hrm. Good question. I've filed bug 819428 - I'll have a peek on Monday when I'm back near my Windows machine.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 689768 [details] [diff] [review]
Patch v1

Ok, I think I'm ready for some review.

Comment 30

[Marco Bonardo [:mak]]

Comment on attachment 689768 [details] [diff] [review]
Patch v1

Review of attachment 689768 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/src/DownloadsCommon.jsm
@@ +306,5 @@
> +   * @param aIndex
> +   *        The index value for the unit. The index is mapped to a value in
> +   *        kAbbrTimeUnits.
> +   */
> +  convertTimeUnitsUnits: function DC_convertTimeUnitsUnits(aTime, aIndex)

I don't understand this naming (yes I know it's used in toolkit, that doesn't make it suck less), convertTimeIndexToUnit maybe?

@@ +324,5 @@
> +   * @param aIndex
> +   *        The index value for the unit. The index is mapped to a value
> +   *        in kToolkitStrMap.units.
> +   */
> +  byteIndexConverter: function DC_byteIndexConverter(aIndex)

and "convertByteIndexToUnit"

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +23,2 @@
>  
> +     That's 55 characters, so we set the width at 55ch.

hum, I count 56, plus B/s is not the longest form, that would be MB/s or KB/s, so that ends up being 57, that is even larger :(

I don't have further ideas, the only compromise would be something like
"59 mins, 59 secs remaining. 1022 / 1023 KB (999.7 KB/s)" that wins back 2ch, probably not even worth the added complication.

Or properly replace "remaining" with a plural form "left"... regarding this, I found that in the past we changed "left" to "remaining", based on UX request: https://bugzilla.mozilla.org/show_bug.cgi?id=394516#c26
Surely if we decide to go back we should not use one in the panel and another one in the manager, but the UX pointers in the above comment state "left" is just wrong.

Or just wontfix this and keep the panel without speed ratio, this was the original ux idea and at this point I'm not sure benefits are worth all of this.

Comment 31

[Marco Bonardo [:mak]]

and for sake of completeness, left replaced remaining, around 2003, cause it was shorter... So looks like we keep changing from one to the other every N years.

Comment 32

[Marco Bonardo [:mak]]

Unblocking the feature, while we have clear advantages and disadvantages of both approaches, the time this is taking away could be better spent on other blockers.

Comment 38

[:Paolo Amadini]

To clarify, we want to redesign the items in the Downloads Panel to show the
download speed also, in an unobtrusive way.

Comment 40

[:Paolo Amadini]

(In reply to Bryant Mao [:bryantmao] from bug 1269958 comment #9)
> One more thing which different from current ETA format is that we'll remove
> "minutes" when the remaining time is above 1 hr, as we think under this time
> scale "minutes" becomes less necessary to show.

Well, if I have to leave in one hour and a half, I may be interested in knowing if my download takes only one hour or it takes one hour and 25 minutes...

Comment 41

[Wesly Huang (EPM)]

Hi Mike, 

This is EPM Wesly from Taipei, nice to meet you!

I learned[1] this bug is suitable to be part of our project scope[2], so wondering do you plan to work on it soon? Or do you think we are able to take it, together with our latest UX spec[3] for moving on?

Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1269958#c7
[2] https://mana.mozilla.org/wiki/display/PM/Content+Handling+Enhancement
[3] attachment in https://bugzilla.mozilla.org/show_bug.cgi?id=1269956

Comment 42

[Mike Conley (:mconley) (:⚙️)]

I don't plan on working on it anytime soon - in fact, I'd forgotten it was assigned to me! It's all yours. :)

Comment 43

[Wesly Huang (EPM)]

Thanks Mike, we'll have someone from project team to take it later!

Comment 45

[Ben Tian (inactive)]

Assign to Sean to help. Some related discussion on implementation order is since bug 1281662 comment 4.

Comment 46

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 812894 - Use the short time format for downloading remaining time.;

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

Comment 47

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

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

Comment 48

[:Paolo Amadini]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review98604

This is a first pass, I'll review the function code more thoroughly in the next version.

::: browser/components/downloads/DownloadsViewUI.jsm:206
(Diff revision 2)
>        [tip, newEstimatedSecondsLeft] = DownloadUtils.getDownloadStatus(
>                                            this.download.currentBytes,
>                                            totalBytes,
>                                            this.download.speed,
>                                            this.lastEstimatedSecondsLeft);
> +      text = tip;

The tip is already set to be equal to the text below, so you can just set the text in the destructuring assignment above.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:26
(Diff revision 2)
>  
> -     59 minutes, 59 seconds remaining - 1022 of 1023 KB
> +     59m 59s left - 1022 of 1023 KB (120.5 KB/sec)
>  
> -     That's 50 characters, so we set the width at 50ch.
> +     That's 45 characters, so we set the width at 45ch.
>       -->
> -<!ENTITY downloadDetails.width            "50ch">
> +<!ENTITY downloadDetails.width            "45ch">

We should keep the current value even if the string for in-progress downloads is shorter, because the strings for blocked downloads and the new labels we are adding for hover cases may be longer.

In fact, the way the rule is explained in the comment is not that useful. We can update the comment for now to explain, but we may actually consider redefining these widths in a separate bug at some point.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:542
(Diff revision 2)
>        scale *= timeSize[unitIndex];
>        unitIndex++;
>      }
>  
>      let value = convertTimeUnitsValue(time);
> -    let units = convertTimeUnitsUnits(value, unitIndex);
> +    let units = gBundle.GetStringFromName(gStr.shortTimeUnits[unitIndex]);

I suspect we still need the PluralForm call here.

Some languages might not have useful short forms, or the short forms might be different for plurals.

Comment 49

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

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

Comment 50

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review98604

> We should keep the current value even if the string for in-progress downloads is shorter, because the strings for blocked downloads and the new labels we are adding for hover cases may be longer.
> 
> In fact, the way the rule is explained in the comment is not that useful. We can update the comment for now to explain, but we may actually consider redefining these widths in a separate bug at some point.

I will close the issue once the relative bug is opened.

> I suspect we still need the PluralForm call here.
> 
> Some languages might not have useful short forms, or the short forms might be different for plurals.

I restore the changes, so the PluralForm in downloads.properties looks like this `shortSeconds=s;s`.

Comment 51

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

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

Comment 52

[Ben Tian (inactive)]

As a reference, this bug will change following UX items:
- Shorten time format to “Xm Xs left — X of X MB (x kB/s)”
- Add the download speed
- “Remaining” > “Left”

Comment 53

[:Paolo Amadini]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review102044

::: browser/components/downloads/DownloadsViewUI.jsm
(Diff revision 4)
> -      // By default, extended status information including the individual
> -      // download rate is displayed in the tooltip. The history view overrides
> -      // the getter and displays the datails in the main area instead.
> -      [text] = DownloadUtils.getDownloadStatusNoRate(
> -                                          this.download.currentBytes,
> -                                          totalBytes,
> -                                          this.download.speed,
> -                                          this.lastEstimatedSecondsLeft);

The getDownloadStatusNoRate function has only one other caller, which is for the Downloads Summary.

You should update the other caller too to include the rate, and remove the implementation of getDownloadStatusNoRate and the related strings.

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd:22
(Diff revision 4)
> -     59 minutes, 59 seconds remaining - 1022 of 1023 KB
> +     59m 59s left - 1022 of 1023 KB (120.5 KB/sec)
>  
> -     That's 50 characters, so we set the width at 50ch.
> +     That's 45 characters, so we set the width at 45ch.
>       -->
> -<!ENTITY downloadDetails.width            "50ch">
> +<!ENTITY downloadDetails.width            "45ch">

There is a previous review comment for this. Did you just forget to update this version of the patch?

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:5
(Diff revision 4)
>  # LOCALIZATION NOTE (seconds, minutes, hours, days): Semi-colon list of plural
>  # forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  seconds=second;seconds
>  minutes=minute;minutes
>  hours=hour;hours
>  days=day;days

You shouldn't leave unused strings behind.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:50
(Diff revision 4)
>  dontQuitButtonWin=Don’t Exit
>  dontQuitButtonMac=Don’t Quit
>  dontGoOfflineButton=Stay Online
>  dontLeavePrivateBrowsingButton2=Stay in Private Browsing
>  downloadsCompleteTitle=Downloads Complete
> -downloadsCompleteMsg=All files have finished downloading. 
> +downloadsCompleteMsg=All files have finished downloading.

On another bug we were asked to keep the trailing space here, to avoid confusing localization tools.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:89
(Diff revision 4)
>  # LOCALIZATION NOTE (timePair2): %1$S time number; %2$S time unit
>  # example: 1 minute; 11 hours
>  timePair2=%1$S %2$S
> +# LOCALIZATION NOTE (shortTimePair): %1$S time number; %2$S time unit
> +# example: 1m; 11h
> +shortTimePair=%1$S%2$S

You've used a mix of "shortStringName" and "stringName2" conventions, it should be one or the other. I recommend using the "stringName2" convention.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:126
(Diff revision 4)
>  stateFailed=Failed
>  stateCanceled=Canceled
>  # LOCALIZATION NOTE (stateBlocked): 'Parental Controls' should be capitalized
>  stateBlocked=Blocked by Parental Controls
>  stateDirty=Blocked: Download may contain a virus or spyware
> -# LOCALIZATION NOTE (stateBlockedPolicy): 'Security Zone Policy' should be capitalized 
> +# LOCALIZATION NOTE (stateBlockedPolicy): 'Security Zone Policy' should be capitalized

This whitespace fix might be harmless, since it's in a localization note, but again we might want to revert it to be sure.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:79
(Diff revision 4)
> -  timePair: "timePair2",
> -  timeLeftSingle: "timeLeftSingle2",
> -  timeLeftDouble: "timeLeftDouble2",
> -  timeFewSeconds: "timeFewSeconds",
> -  timeUnknown: "timeUnknown",
> +  shortTimePair: "shortTimePair",
> +  timeLeftSingle: "timeLeftSingle3",
> +  shortTimeLeftDouble: "shortTimeLeftDouble",
> +  timeFewSeconds: "timeFewSeconds2",
> +  timeUnknown: "timeUnknown2",

To simplify, follow consistently the same approach that was used before in this file, so don't modify the keys of the object, just add one to the number in the string value.

::: toolkit/mozapps/downloads/DownloadUtils.jsm:90
(Diff revision 4)
>    yesterday: "yesterday",
>    doneScheme: "doneScheme2",
>    doneFileScheme: "doneFileScheme",
>    units: ["bytes", "kilobyte", "megabyte", "gigabyte"],
>    // Update timeSize in convertTimeUnits if changing the length of this array
> -  timeUnits: ["seconds", "minutes", "hours", "days"],
> +  shortTimeUnits:["shortSeconds", "shortMinutes", "shortHours", "shortDays"],

Same here, keep "timeUnits" intact in the code. In this case, though, "shortSeconds" might be clearer than "seconds2" as a string identifier for localizers.

Comment 54

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

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

Comment 55

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review102044

I go through all issues and resolved them in the latest patch. Please review it again. Thanks.

> Same here, keep "timeUnits" intact in the code. In this case, though, "shortSeconds" might be clearer than "seconds2" as a string identifier for localizers.

I am not sure if I understand this comment correctly. "short" prefix is remained here for L10N team to recognize the purpose of these chagnes.

Comment 56

[Sean Lee [:seanlee][:weilonge]]

Hi Bryant,

In the latest patch, the detail information in Summary section is with download rate now. The calculation is like this:

Total Downloading Rate = Sum the downloaded rate for all items in Summary section.
Total Remaining Bytes = Sum the remaining bytes for all items in Summary section.

Estimation Time = Total Remaining Bytes / Total Downloading Rate

Could you help to confirm these items?
1. Display Total Downloading Rate in Summary section or now.
2. What's Estimation Time formula if we want to show 1.

Thanks.

Comment 57

[Bryant Mao [:bryantmao]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #56)
> Hi Bryant,
> 
> In the latest patch, the detail information in Summary section is with
> download rate now. The calculation is like this:
> 
> Total Downloading Rate = Sum the downloaded rate for all items in Summary
> section.
> Total Remaining Bytes = Sum the remaining bytes for all items in Summary
> section.
> 
> Estimation Time = Total Remaining Bytes / Total Downloading Rate
> 
> Could you help to confirm these items?
> 1. Display Total Downloading Rate in Summary section or now.

My answer is no, at least for now. 

Originally, there are two reasons we want to introduce download rate to summary section: 1) The information is consistent since we are now planning to add download rate on each downloading item; 2) User might feel faster if they see the download rate (Our assumption). 

However, considering user may want to see the overall download progress and remaining time rather than the download rate which is not even very precise, as well as the average rate might conflict with our current estimation time formula (longest time), I don't think it worth the efforts.

Comment 58

[:Paolo Amadini]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #55)
> I am not sure if I understand this comment correctly. "short" prefix is
> remained here for L10N team to recognize the purpose of these chagnes.

Yes, this is what I meant. Thanks!

Comment 59

[:Paolo Amadini]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review103358

I've filed bug 1329167 about the removal of getDownloadStatusNoRate so that it can be addressed separately in the future. For the moment you can revert the related changes in this patch, which is how I interpreted Bryant's comment.

The rest looks good, but please make sure that all the tests pass before asking for the final review.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:82
(Diff revision 5)
>  
> -# LOCALIZATION NOTE (timePair2): %1$S time number; %2$S time unit
> -# example: 1 minute; 11 hours
> -timePair2=%1$S %2$S
> -# LOCALIZATION NOTE (timeLeftSingle2): %1$S time left
> -# example: 1 minute remaining; 11 hours remaining
> +# LOCALIZATION NOTE (timePair3): %1$S time number; %2$S time unit
> +# example: 1m; 11h
> +timePair3=%1$S%2$S
> +# LOCALIZATION NOTE (timeLeftSingle3): %1$S time left
> +# example: 1 minute left; 11 hours left

nit: update the example to say "1m left; 11h left".

Comment 60

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97820/diff/5-6/

Comment 61

[Sean Lee [:seanlee][:weilonge]]

Hi Paolo,

This [1] is the treeherder result of the latest patch. I've checked that these failed cases are not relative to the changes of Downloads Panel. All issues you raised are resolved as well.
Please review it again. Thank you.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d990ae0e3d67292f1e443326bcfccf5f42671b88

Comment 62

[:Paolo Amadini]

Comment on attachment 8817589 [details]
Bug 812894 - Use the short time format for downloading remaining time.;

https://reviewboard.mozilla.org/r/97820/#review103806

Thanks!

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/053b67802207
Use the short time format for downloading remaining time.; r=Paolo

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/053b67802207

Comment 65

[Francesco Lodolo [:flod]]

There's a problem with the plural strings (it was already there)
https://hg.mozilla.org/mozilla-central/diff/053b67802207/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties

Each string needs to have its own localization comment for plural forms, one before the group won't work.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

Not sure if you want to address it here, since it's just comments in the .properties file, or in a new bug.

Side question: is this code going to be used in Android too? I wonder if we can't simplify this relying on Intl APIs (ccing Zibi too).

Comment 66

[Zibi Braniecki [:zbraniecki][:gandalf]]

not yet. We are planning to get Intl.RelativeTimeFormat ("in 1 minute") and Intl.UnitFormat ("1 minute", "2 minutes"), but we that's probably closer to H2 2017.

Comment 67

[Sean Lee [:seanlee][:weilonge]]

Hey Francesco,

Do you mean this line[2] should be changed to this[2]:

[1] # LOCALIZATION NOTE (seconds, minutes, hours, days): Semi-colon list of plural

[2] # LOCALIZATION NOTE (shortSeconds, shortMinutes, shortHours, shortDays): Semi-colon list of plural

Comment 68

[Francesco Lodolo [:flod]]

No, it should become

# LOCALIZATION NOTE (shortSeconds): Semi-colon list of plural
# forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# s is the short form for seconds
shortSeconds=s;s

# LOCALIZATION NOTE (shortMinutes): Semi-colon list of plural
# forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# m is the short form for minutes
shortMinutes=m;m

etc.

Comment 69

[Vuyisile Ndlovu [:terrameijar]]

User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

Time strings shortened, download speed visible, bug verified fixed.

Comment 70

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using 53.0a1, build ID:20161218030213, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx53.0a1, build ID: 20170113030227, on Windows 10 x64, Mac OS X 10.12.3 and Ubuntu 14.04 LTS.

Cheers!

Comment 71

[Roman Müller]

I'm not sure if this belongs here, but download panel tip pointing on its button is not consistent with few other panels (History, Pocket, Text Encoding, Developer Tools and Horlander's mockup). It is however consistent with few other panels (App menu and Bookmarks). See screenshots below:

Ovlerlapping tip of panel:
Downloads panel http://fii.cz/pqbmwcae
App menu http://fii.cz/ykcbss
Bookmarks http://fii.cz/udfahjuv

Not overlapping tip:
History http://fii.cz/hegskd
Pocket http://fii.cz/aerxh
Developer Tools http://fii.cz/nysuyac
Text Encoding http://fii.cz/wraaxsm
Horlander's mockup http://fii.cz/btqnvsd

personally I do not mind which style of these two is supposed to be the right one but you should pick one and stick with it.

Comment 72

[:Paolo Amadini]

Roman, that's good point related to polishing our user interface. The screenshots are useful reference. Can you file a new bug under "Firefox :: Toolbars and Customization" if one isn't on file already, so this doesn't get lost?

Comment 73

[Roman Müller]

Paolo, yes I can.(In reply to :Paolo Amadini from comment #72)
> Roman, that's good point related to polishing our user interface. The
> screenshots are useful reference. Can you file a new bug under "Firefox ::
> Toolbars and Customization" if one isn't on file already, so this doesn't
> get lost?
Yes I can. Done under https://bugzilla.mozilla.org/show_bug.cgi?id=1331927

Comment 74

[:Paolo Amadini]

Thanks!