448344 - Download manager says "1 minute remaining" when it's really between 1 and 2 minutes

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ariel]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1

The download manager is one minute off. The download timer says how many minutes are left, and it says "1 minute remaining" for about a minute, and only after that does the minute-long seconds countdown begin.

It should say 2 minutes remaining, not 1 minute remaining, and then after the first of those two minutes are done it should get into the seconds countdown.

Reproducible: Always

Steps to Reproduce:
1. Choose to download a file that'd take a couple minutes.
2. Notice when it changes to '1 minute remaining'.
3. One minute later, only then does the 1 minute countdown begin.

Comment 1

[Sylvain Pasche]

For the remaining time under one hour, only the minutes are shown:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/DownloadUtils.jsm?mark=299-300#299

This means than where there is 1.5 minutes remaining, "1 minute remaining" is displayed, because the the value is floor rounded.

I see the following options:
1) wontfix
2) Show minutes and seconds when under like 5 minutes
3) Use Math.ceil instead of Math.floor for rounding

Comment 2

[Shawn Wilsher :sdwilsh]

I think Math.ceil would be the best option here.

Comment 3

[João Neves]

Attachment: Patch applying proposed change

Following the change proposed by Shawn Wilsher, here's a patch changing the rounding method from Math.floor to Math.ceil.

Testing seems to indicate it's working as expected.

Comment 4

[Sylvain Pasche]

You will also need to update the unit tests.  For instance: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js

See https://developer.mozilla.org/en/Mozilla_automated_testing for more information.

Comment 5

[João Neves]

Argh, my bad. There are a few quirks that still need to be worked around, so that patch isn't valid.

Hadn't noticed the change breaks at least one of the test cases.

Simply using Math.ceil instead of Math.floor also causes "1 hour and 34 minutes left" go "2 hours left", for instance.

Comment 6

[João Neves]

Attachment: Fixed DownloadUtils patch

This patch makes it such that minutes are rounded up when on their own, thus fixing the described bug.

For instance, 110 seconds are now rounded up to 2 minutes instead of being rounded down to 1 minute.

Comment 7

[João Neves]

Attachment: Updated test cases

Test case update to deal with the new behavior. Added two tests to specifically test for the desired behavior.

Comment 8

[João Neves]

Attachment: Unified patch (patch + test cases)

Sorry for the attachment spam, still getting used to mercurial, didn't remember I could make a single patch file with all the changes.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 354938 [details] [diff] [review]
Unified patch (patch + test cases)

A few questions about this, and then some comments on the patch:
What happens in the case of 59 minutes and some seconds - do we end up staying at one hour left for a while?

>diff -r 214c605bfb64 toolkit/mozapps/downloads/src/DownloadUtils.jsm
> /**
>  * Private helper for convertTimeUnits that gets the display value of a time
>  *
>  * @param aTime
>  *        Time value for display
>- * @return An integer value for the time rounded down
>+ * @param aIndex
>+ *	  Index into gStr.timeUnits for the appropriate unit
>+ * @return An integer value for the time rounded appropriately
>  */
You've got a tab in here - please only use spaces

>+function convertTimeUnitsValue(aTime, aIndex)
> {
>+  // We should round up only if we're dealing with minutes
>+  if(aIndex == 1)
nit: space after if

Edward should take a look at this too since he wrote this code.

Comment 10

[Ed Lee :Mardak]

Comment on attachment 354938 [details] [diff] [review]
Unified patch (patch + test cases)

>--- a/toolkit/mozapps/downloads/src/DownloadUtils.jsm	Tue Dec 16 14:53:42 2008 -0500
>+    let value = convertTimeUnitsValue(time, unitIndex);
>     let units = convertTimeUnitsUnits(value, unitIndex);
> 
>     let extra = aSecs - value * scale;
We rounded down in convertTimeUnitsValue because we do this calculation here for the leftover time.

>     // Convert the extra time to the next largest unit
>     for (let index = 0; index < nextIndex; index++)
>       extra /= timeSize[index];
This is the only place where extra is used.

When convertTimeUnits is called with say.. 90 seconds, it actually returns 2 pairs of time/units of [1, minute, 30, seconds].

It's only in the caller where we decide to drop off the "30 seconds".

getTimeLeft: function DU_getTimeLeft(aSeconds, aLastSec)
  // Only show minutes for under 1 hour or the second pair is 
  if (aSeconds < 3600 || time2 == 0)
    timeLeft = replaceInsert(gStr.timeLeftSingle, 1, pair1);

Assuming we want to keep the functionality of convertTimeUnits consistent, we should probably change the caller of that method, getTimeLeft. Perhaps a check for something like "60 < aSeconds < 120", use "2" instead of "time1".

Comment 11

[Ed Lee :Mardak]

But then again, if we check only for <120, we're going to show "2 minutes" after "3 minutes" for 2 minutes.. similar to how we show "1 minute" for 2 minutes after "2 minutes".

Comment 12

[João Neves]

First of all, sorry for all the delay on getting some sort of reply from me (MSc thesis getting in the way, so I haven't had much time to look at this :-).

For Shawn, thank you, I seriously got to start writing my code less "automatically", which introduces some stuff like what you pointed out.

(In reply to comment #9)
> (From update of attachment 354938 [details] [diff] [review])
> A few questions about this, and then some comments on the patch:
> What happens in the case of 59 minutes and some seconds - do we end up staying
> at one hour left for a while?

Yes, apparently it does happen. By doing it this way it rounds up every time: not good.

(In reply to comment #11)
> But then again, if we check only for <120, we're going to show "2 minutes"
> after "3 minutes" for 2 minutes.. similar to how we show "1 minute" for 2
> minutes after "2 minutes".

Yes, it's exactly what happens. Going that way will definitely not be too helpful, since we will keep having this error, just happening on a different spot.

Given the way the whole code works, and given some of the options already considered here by Sylvain:

> I see the following options:
> 1) wontfix
> 2) Show minutes and seconds when under like 5 minutes
> 3) Use Math.ceil instead of Math.floor for rounding

I believe the "least worse" situation here would probably be something like #2, at least for the time being.

My reasoning for this is that when there are still a few minutes to complete the download most users will probably do something else, like surf another web page, or leave their computer for a few minutes while they wait for their download to complete, so they aren't really paying attention to how much time is left.

On the other hand, when a user knows his download is almost done, his/her attention will probably be more focused on the download manager window, and as such, things like "1 minute remaining" staying there for almost 2 minutes will be more noticeable (hence the bug report, I suppose :-).

So: the issue will still be there but it will, in my opinion (which may as well be awfully flawed), be much less noticeable.

I may be missing a really obvious way of sorting this out without breaking it somewhere else, but I don't really see it at the moment.

Comment 13

[Shawn Wilsher :sdwilsh]

Let's come some feedback from our UI folks...

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

>I think Math.ceil would be the best option here.

I agree, and this is now my new favorite perception of performance issue in Firefox (where 1 minute literally takes two minutes).

Comment 15

[João Neves]

(In reply to comment #14)
> >I think Math.ceil would be the best option here.
> 
> I agree, and this is now my new favorite perception of performance issue in
> Firefox (where 1 minute literally takes two minutes).

If everyone agrees with that (I certainly take no issue, it's much better to fix it for good) I can try working it out again sometime soon (I personally don't expect to have it done TOO soon though, I'm loaded with work for the next month or so, although I'll use whatever breaks I can for this :-).

I also don't expect this to be a trivial patch, but I'll certainly take my time looking at this carefully from the start before I start doing something overly complicated.

Comment 16

[Ed Lee :Mardak]

Alex: Should we start showing seconds once it goes under some amount of time? I believe Madhava originally suggested not showing "seconds" once we go under 1 hour because otherwise we would show "59 minutes, 59 seconds left" which isn't really useful with that much time left.

Perhaps we could show "1 minute, 30 seconds" up to "4 minutes, 59 seconds" ? (Yes, we would still show 5 minutes for the duration of 5-6 minutes, but perhaps that's better than 1 minute being twice as long.)

Comment 17

[Ariel]

If you want to do it that way, I think it would make more sense to have it say the number of seconds starting with 9 minutes. That way it shows up when there's one less digit, and more numbers (the seconds) don't just pop up at an otherwise arbitrary time.

Comment 18

[João Neves]

Ariel: I don't think 5 is any more arbitrary than 9, but any of those is better than the current state (as I tried to explain in my post above).

Given this, I'd think the 5 minute threshold is more reasonable than the 10 minute one (at least I'd expect most reasonably large downloads to take over 5 minutes rather than over 10 minutes, although I don't have any concrete data to back this up).

Comment 19

[Ariel]

Well now that I think about it, the whole timer thing is a real-world metaphor for something like a kitchen timer. So I got to thinking how a kitchen timer does it: As soon as it's below 1hr, it shows minutes AND seconds. So I'm wondering, what would be so bad about listing the seconds for everything under one hour? Whether or not they care to the second 30 minutes before a download finishes, they get a more accurate and precise count.

Comment 20

[João Neves]

I tend to agree with you. Couldn't we "compromise" on always displaying two time/unit pairs? Display "hours, minutes" when over 1 hour and "minutes, seconds" when under 1 hour, for instance, with the exception of displaying only the seconds when under 1 minute.

Comment 21

[Ed Lee :Mardak]

Attachment: v2

Show seconds when we're under 4 minutes like how we show "few seconds" when we're under 4 seconds. This pushes the "double minute" problem to 4 minutes instead of 1 minute, but the user experience goes from 100% longer to 25% longer. Test by checking low times for seconds, minutes, hours and check that we generate the right string for the boundaries.

Comment 22

[Shawn Wilsher :sdwilsh]

Comment on attachment 376948 [details] [diff] [review]
v2

>+      // Only show minutes for under 1 hour unless there's a few minutes left;
>+      // or the second pair is 0
nit: end of sentence punctuation.

r=sdwilsh

Comment 23

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 376948 [details] [diff] [review]
v2

This looks good to me as long as it works as described. I don't have a build environment yet (next on my list!), but I have read the diff, and it seems sane to me.

Comment 24

[Ed Lee :Mardak]

faaborg: what ux-xxxx did you want to tag this with? (I can't seem to find your 8 ux-thingies anywhere..)

http://hg.mozilla.org/mozilla-central/rev/81de46fe1450

Show seconds when we're under 4 minutes like how we show "few seconds" when we're under 3 seconds. This pushes the "double minute" problem to 4 minutes instead of 1 minute, but the user experience goes from 100% longer to 25% longer. Test by checking low times for seconds, minutes, hours and check that we generate the right string for the boundaries.