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.

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).

(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...

(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.

(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 :-)

Updated by removing the test locale fixes already addressed in bug 671533.

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.