Closed Bug 653261 Opened 10 years ago Closed 10 years ago

Add a function to format the date and time displayed in the Download Manager

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Patch with test case (obsolete) — Splinter Review
Follow-up to bug 564934 comment 170.

Shawn, feel free to suggest new names for the functions or their arguments if
there is a particular convention I missed.
Attachment #528717 - Flags: review?(sdwilsh)
Comment on attachment 528717 [details] [diff] [review]
Patch with test case

Review of attachment 528717 [details] [diff] [review]:

r=sdwilsh, but this will need sr

::: toolkit/mozapps/downloads/DownloadUtils.jsm
@@ +327,5 @@
 
   /**
+   * Generate a compact, readable, though not accurate representation of the
+   * given date and time, relative to the current date. For example, only the
+   * hour is displayed for today. Generate also a complete, readable version

nit: "Also, generate..." instead of "Generate also..."

@@ +336,5 @@
+   *        Date object representing the date and time to format. It is assumed
+   *        that this value represents a past date.
+   * @param [optional] aNow
+   *        Date object representing the current date and time. The real date
+   *        and time of invocation is used if this parameter is omitted.

Global nit: two spaces after a period as per the rest of this file please :)

@@ +342,5 @@
+   */
+  getRecentDateTime: function DU_getRecentDateTime(aDateTime, aNow)
+  {
+    if (!aNow)
+      aNow = new Date();

nit: brace one line ifs please (even if the rest of this file doesn't, new code should)

::: toolkit/mozapps/downloads/tests/unit/test_DownloadUtils.js
@@ +137,5 @@
 }
 
 function run_test()
 {
+  testAllGetRecentDateTime();

nit: I'd prefer it if you added this to the end of the list, and had the function body in the same order as it is called in in the file (so after all the exists tests).
Attachment #528717 - Flags: superreview?(mconnor)
Attachment #528717 - Flags: review?(sdwilsh)
Attachment #528717 - Flags: review+
(In reply to comment #1)
> r=sdwilsh, but this will need sr

To whom should I ask?

> Global nit: two spaces after a period as per the rest of this file please :)

The rest of this file has only one space after each period...
Attached patch Nits addressed (obsolete) — Splinter Review
(In reply to comment #2)
> (In reply to comment #1)
> > r=sdwilsh, but this will need sr
> 
> To whom should I ask?

Sorry, I've overlooked that you already set the flag.
Comment on attachment 528717 [details] [diff] [review]
Patch with test case


>   /**
>+   * Generate a compact, readable, though not accurate representation of the
>+   * given date and time, relative to the current date. For example, only the
>+   * hour is displayed for today. Generate also a complete, readable version
>+   * of the same information, often shown when hovering the cursor over the
>+   * compact representation.

Burying the lead a little here, this returns two strings in all cases.

"Converts a Date object to two readable formats, one compact, one complete." etc.

>+   * @param aDateTime
>+   *        Date object representing the date and time to format. It is assumed
>+   *        that this value represents a past date.

it's a Date object, we can just call it aDate

>+   * @param [optional] aNow
>+   *        Date object representing the current date and time. The real date
>+   *        and time of invocation is used if this parameter is omitted.
>+   * @return A pair: [compact text, full text]
>+   */

the "Recent" part of the function name is misleading.  How about getReadableDates?

Why do we need the optional param?  What's the use-case for generating times relative to not-now?
(In reply to comment #4)
> the "Recent" part of the function name is misleading.  How about
> getReadableDates?

The first return value of the function is a string that sometimes contains
a readable date, sometimes contains a readable time of the day, based on how
recent the provided date and time is. Although I agree that there can be a
better name than "getRecentDateTime", it seems to me that "getReadableDates"
doesn't tell that this isn't an ordinary date formatting function. Maybe
"getRelativeReadableDates"? Or do you have any other suggestion?

> Why do we need the optional param?  What's the use-case for generating times
> relative to not-now?

We need that for testing, otherwise the function results wouldn't be
deterministic.

Maybe there are other use cases, for example obtaining the current date and
time once and then invoking the function multiple times, ensuring that we get
consistent results (we don't do that now, though).
(In reply to comment #5)
> (In reply to comment #4)
> > the "Recent" part of the function name is misleading.  How about
> > getReadableDates?
> 
> The first return value of the function is a string that sometimes contains
> a readable date, sometimes contains a readable time of the day, based on how
> recent the provided date and time is. Although I agree that there can be a
> better name than "getRecentDateTime", it seems to me that "getReadableDates"
> doesn't tell that this isn't an ordinary date formatting function. Maybe
> "getRelativeReadableDates"? Or do you have any other suggestion?

Only the first one is relative.  The latter is returning the full date string, in a readable format.  Maybe getReadableDateStrings?  Feels like that's sufficiently descriptive and accurate.

> > Why do we need the optional param?  What's the use-case for generating times
> > relative to not-now?
> 
> We need that for testing, otherwise the function results wouldn't be
> deterministic.

I'm not a huge fan of this, but the second use case is barely plausible enough to carry this for me.  I'll review a new patch with the function name change.
Blocks: 581012
(In reply to comment #6)
> Maybe getReadableDateStrings?

Well, since this defeats my point in any case, I think I'll go for your first
suggestion, getReadableDates, if it's OK for you. I appreciate the diplomacy
of proposing a third alternative though :-)
Attachment #528968 - Attachment is obsolete: true
Attachment #536197 - Flags: superreview?(mconnor)
Attachment #528717 - Flags: superreview?(mconnor)
Attachment #536197 - Flags: superreview?(mconnor) → superreview+
Attached patch The patchSplinter Review
Updated by removing the test locale fixes already addressed in bug 671533.
Attachment #528717 - Attachment is obsolete: true
Attachment #536197 - Attachment is obsolete: true
I've run this through tryserver and I see some JetPack tests failing, I don't
think they're related to this patch, but I'm not sure since the failing tests
aren't shown in the "Build Error Summary" section:

http://tbpl.mozilla.org/?tree=Try&rev=64a5ad110294

If someone can confirm that these failures are unrelated, then this patch is
probably ready for check-in.
Did you pull central before pushing to try? jetpack is going to be orange if you're pushing from a tree older than 8.0 version bump.
ehr, looks like JP is currently permaorange on mozilla central as well, indeed it's hidden there. I think you may ignore those.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/69913a1505df
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.