1331466 - Timezone offset in certificate timestamps is not displayed correctly anmore in FF53

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P2)

Description

[[:MakeMyDay]]

Attachment: time_issue_45win_53linux32.png

While looking at the root cause for bug 1325792, I noticed the following:

FF doesn't display certificate timestamps with respect to the timezone (seen in Windows and Linux) correctly anymore compared to ESR45. The timezone offset doesn't seem to be considered anymore - see the attached screenshot (Win ESR45 on the left, current Nightly L32 on the right, though this doesn't seem to be platform related, Nightly Win behaves the same way).

In Calendar, we're observing some obscure platform specific (L32 and OSX) time display issues since bug 1301640 landed, so there seem to be more uncovered issues in the new time formatter code.

Comment 1

[Jorg K (CEST = GMT+2)]

Gregory and Masatoshi-san, could you please take a look. As you know, TB ships with Calendar/Lightning and that's been busted on Linux32 and Mac ever since bug 1301640 landed. Interesting reading in bug 1325792 comment #12.

Comment 2

[Masatoshi Kimura [:emk]]

Hm, old nsDateTimeFormat*::FormatTime used to call localtime(_r). Why DatetimeICU::FormatTime does not anymore?

Comment 3

[Masatoshi Kimura [:emk]]

Please teach me how ICU (or Intl) deal with operating system time zones.

Comment 4

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

ICU should follow OS TimeZones. It has been a major thing when we were working on Firefox OS and I remember a bunch of refactors went into this to make sure that we also react to OS timezone changes at runtime properly.

NI :waldo, and Andre since they know the ICU bindings more.

Comment 5

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

It was actually Ted Clancy who worked on that, but I hope that :waldo understands the code :)

(see bug 1172609 and bug 1198543)

Comment 6

[Masatoshi Kimura [:emk]]

OK, I found the cause. ICU/Intl are not relevant here.

The certificate viewer does not use FormatTime, it uses FormatPRExplodedTime:
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCertHelper.cpp#1613-1648
It uses PR_ExplodeTime to get both local and GMT times. Old nsDateTimeFormat* implementations did not care about time zone information in PR_ExplodeTime (they just drop tm_params.tp_gmt_offset and tm_params.tp_dst_offset), but DateTimeFormatICU does (it uses PR_ImplodeTime to convert back the time).

We will have to either:
1. Fix all FormatPRExplodedTime callers, or
2. Make FormatPRExplodedTime bug-compatible with the old implementations.

Comment 7

[Masatoshi Kimura [:emk]]

Gregory, could you take a look? This bug would have higher priority than bug 1329841.

All you have to is to port the FormatPRExplodedTime implementation from DateTimeFormatUnix to DateTimeFormatICU, I guess.

Comment 8

[Gregory Moore [:gmoore]]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Gregory, could you take a look? This bug would have higher priority than bug
> 1329841.

Yeah, definitely. I am a little confused about a few things, though.

> It uses PR_ExplodeTime to get both local and GMT times. Old
> nsDateTimeFormat* implementations did not care about time zone information
> in PR_ExplodeTime (they just drop tm_params.tp_gmt_offset and
> tm_params.tp_dst_offset), but DateTimeFormatICU does (it uses PR_ImplodeTime
> to convert back the time).

It seems like before (and in DateTimeFormatUnix.cpp) we were incorrectly ignoring the timezone information stored in PRExplodedTime, which seems like it would treat both the local time and the GMT time for the certificate as the same time. So that would mean it would incorrectly show both times as the same. So why would it work before, but now when we do take into account the timezone, it gives the same time? It seems like it should be the other way around.

> We will have to either:
> 1. Fix all FormatPRExplodedTime callers, or
> 2. Make FormatPRExplodedTime bug-compatible with the old implementations.

> All you have to is to port the FormatPRExplodedTime implementation from
> DateTimeFormatUnix to DateTimeFormatICU, I guess.

We could do something like we were doing before, and possibly add back in a private FormatTMTime function for DateTimeFormatICU.cpp. We would then I guess be converting PRExplodedTime -> tm -> time_t -> PRTime -> UDate. But wouldn't that just be incorrectly ignoring the timezone information in PRExplodedTime? I still don't really understand why that would work. 

It seems like it would make more sense to fix the callers, as there are only 8 of them (3 from mozilla-central and 5 from mailnews). But I don't actually know what's wrong with the callers (they pass PR_LocalTimeParameters into PR_ExplodeTime for local time and PR_GMTParameters for GMT time, which seems correct). Please let me know, thanks.

Comment 9

[Masatoshi Kimura [:emk]]

For example, the notBefore date of the mozilla.org certificate is 2015-10-26T00:00:00Z.

When I view this certificate (in Japan),

PR_ExplodeTime(dispTime, PR_LocalTimeParameters, &explodedTime) will fill explodedTime with following values:
{tm_year=2015, tm_month=9, tm_mday=26, tm_hour=9, tm_min=0, tm_sec=0, tm_params.tp_gmt_offset=9*60*60}

PR_ExplodeTime(dispTime, PR_GMTParameters, &explodedTimeGMT) will fill explodedTimeGMT with:
{tm_year=2015, tm_month=9, tm_mday=26, tm_hour=0, tm_min=0, tm_sec=0, tm_params.tp_gmt_offset=0}

Old implementations will ignore tm_params.tp_gmt_offset and just copy tm_year...tm_sec to struct tm. So we will get the formatted string "2015年10月26日 9:00:00" from explodedTime and "2015年10月26日 0:00:00" from explodedTimeGMT.

DateTimeFormatICU, however, uses PR_ImplodeTime to convert PRExplodedTime to PRTime. PR_ImplodeTime will subtract tp_gmt_offset from tm_sec+tm_min*60+tm_hour*3600+tm_mday*86400, so both explodedTime and explodedTimeGMT will be converted to the same PRTime value that repersents 2015-10-26T00:00:00Z. Because FormatPRTime will take a GMT PRTime and return a local time string, both explodedTime and explodedTimeGMT will be converted to "2015年10月26日 9:00:00", which is incorrect.

Does this clarify the situation?

Comment 10

[Masatoshi Kimura [:emk]]

That is, although callers expect that FormatPRExplodedTime will format the time using the time zone information from the PRExplodedTime parameter, the current FormatPRExplodedTime implementation does not. It will always format the time using the system time zone.

I think we should restore the old behavior.

Comment 11

[Masatoshi Kimura [:emk]]

Passing 4th & 5th parameters to udat_open will do the job. (they are tzID and tzIDLength.) But I don't know how to convert time zone / DST offsets to tzID...

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #11)
> But I don't know how to convert time zone / DST offsets to tzID...

It would be impossible because multiple time zones might have the same time zone offset.
The workaround would be to ignore tm_params (as the old implementations did) and pass GMT to tzID because we don't have to format a correct time zone name.

Comment 13

[André Bargull [:anba]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> ICU should follow OS TimeZones. It has been a major thing when we were
> working on Firefox OS and I remember a bunch of refactors went into this to
> make sure that we also react to OS timezone changes at runtime properly.

I don't think we're already using OS services/events to get notifications when the time zone changes (NSSystemTimeZoneDidChangeNotification, WM_TIMECHANGE, Intent.ACTION_TIMEZONE_CHANGED, file watchers for /etc/{localtime,timezone,TZ}; based on the list at https://codereview.chromium.org/251613002). Instead we're simply re-querying the default time zone at certain points (e.g. in JSCompartment::init()).

Comment 14

[Gregory Moore [:gmoore]]

Attachment: Changed implementation of FormatPRExplodedTime() so that it would ignore timezone information given in the the PRExplodedTime parameter.

Oh okay, this makes more sense now. Thanks for explaining it more. 

(In reply to Masatoshi Kimura [:emk] from comment #12)
> It would be impossible because multiple time zones might have the same time
> zone offset.
> The workaround would be to ignore tm_params (as the old implementations did)
> and pass GMT to tzID because we don't have to format a correct time zone
> name.

Do you think it might be possible if we used the icu::TimeZone class? I haven't looked into it too much but it seems like we could get the local time zone that way. Also, I think we could just use a custom timezone like "GMT+8:00" or "GMT-6:00". I don't think we actually ever include the name of the time zone in the formatted time, so as long as the offset is correct we might be fine. 

Actually, I guess in that case it doesn't really matter how we implement it. So I guess we should just go with the way you said.

I have created a patch that I think will fix the problem. Let me know if this isn't what you meant. I'm still not exactly sure if I should be passing in "GMT" for tzID or just leaving it as nullptr.

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #14)
> Do you think it might be possible if we used the icu::TimeZone class? I
> haven't looked into it too much but it seems like we could get the local
> time zone that way. Also, I think we could just use a custom timezone like
> "GMT+8:00" or "GMT-6:00". I don't think we actually ever include the name of
> the time zone in the formatted time, so as long as the offset is correct we
> might be fine. 

If it is possible, it is definitely better. My workaround will not handle leap seconds correctly.

> I have created a patch that I think will fix the problem. Let me know if
> this isn't what you meant. I'm still not exactly sure if I should be passing
> in "GMT" for tzID or just leaving it as nullptr.

Even if you go with my workaround, you should not pass GMT unconditionally because other Format*Time variants is expected that they will format time using the system timezone. Only FormatPRExplodedTime should use timezone information from the parameter (other type does not have timezone information in the first place).

Comment 16

[Gregory Moore [:gmoore]]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> Even if you go with my workaround, you should not pass GMT unconditionally
> because other Format*Time variants is expected that they will format time
> using the system timezone. Only FormatPRExplodedTime should use timezone
> information from the parameter (other type does not have timezone
> information in the first place).

Oh okay, I see. That makes sense.

> If it is possible, it is definitely better. My workaround will not handle
> leap seconds correctly.

Okay. I have looked into this more and I see two (or maybe more) possible options:

1) Use the information given in the PRExplodedTime's PRTimeParameters (tm_params) member and calculate the GMT offset. It's given in seconds but we can convert it to hours and minutes, and then create a string of this form (from the ICU documentation):
> GMT[+|-]hh[[:]mm]
We can pass that into tzID in udat_open() and it will recognize it as a custom timezone. Thus we will always use the correct timezone (at least down to the minute). We are given tp_gmt_offset and tp_dst_offset (for daylight savings time), but I think we can just add them together and convert them to a string.

Actually I just tried this and we can actually specify down to the second with the custom timezone. So we could use a string like:
> GMT[+|-]hh[[:]mm][[:]ss]
I'm not sure if it would handle leap seconds correctly like you mentioned though.

2) Use icu::TimeZone::createEnumeration(int32_t rawOffset) to enumerate all values with the same GMT offset as tp_gmt_offset. Then we could try each one and use icu::TimeZone::getDSTSavings() to find one where the Daylight savings matched the value in tp_dst_offset. If we didn't find any that matched (assuming they passed in a custom timezone or something), we could just do the method in (1). 

This seems pretty inefficient and weird but it might fix the leap second problem? There might also be another better way to do it; there's a lot of different functions in the icu::TimeZone documentation. Let me know if either of those sound good or if you have any other ideas, thanks.

Comment 17

[Masatoshi Kimura [:emk]]

I guess method 1) would be good enough. We can't select the correct timezone anyway if multiple timezones have the same GMT offset and DST offset. And we should only use the documented format (that is, "GMT[+|-]hh[[:]mm]") for custom timezones.

Comment 18

[Gregory Moore [:gmoore]]

Attachment: Added functionality to FormatPRExplodedTime (in DateTimeFormatICU.cpp) to take into account timezone information given in the PRExplodedTime parameter.

Here is a patch with method (1) above implemented. The code to create the timezone string is kind of long; we might be able to make that part cleaner. I'm not sure if there's already functions somewhere else that do this sort of thing (time formatting operations) that we could use. Let me know what you think or if you see any other problems.

Comment 19

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #18)
> Here is a patch with method (1) above implemented. The code to create the
> timezone string is kind of long; we might be able to make that part cleaner.
> I'm not sure if there's already functions somewhere else that do this sort
> of thing (time formatting operations) that we could use. Let me know what
> you think or if you see any other problems.

I don't know either. But maybe you can use u_snprintf to make the code shorter.

Comment 20

[Masatoshi Kimura [:emk]]

Comment on attachment 8829298 [details] [diff] [review]
Added functionality to FormatPRExplodedTime (in DateTimeFormatICU.cpp) to take into account timezone information given in the PRExplodedTime parameter.

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

::: intl/locale/DateTimeFormatICU.cpp
@@ +129,5 @@
>        NS_ERROR("Unknown nsTimeFormatSelector");
>        return NS_ERROR_ILLEGAL_VALUE;
>    }
>  
> +  nsAutoString timeZoneID(L"GMT");

Use u"..." throughout the code. L"..." (or wchar_t) is not portable.

Comment 21

[Gregory Moore [:gmoore]]

(In reply to Masatoshi Kimura [:emk] from comment #19)
> I don't know either. But maybe you can use u_snprintf to make the code
> shorter.

Good idea. That seems like it will be much cleaner. However, I tried doing this and it said it couldn't find the file 'unicode/ustdio.h' when I tried to compile it. I have it included as:
#include "unicode/ustdio.h"
Which is similar to how I included udat.h, which worked fine. Is there a way to fix this, or is that file just not part of the build process or something?

An alternative might be to something like this:
CopyASCIItoUTF16(nsPrintfCString("GMT%+2d:%2d", hours, minutes), timeZoneID);

Let me know what you think.

Comment 22

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #21)
> An alternative might be to something like this:
> CopyASCIItoUTF16(nsPrintfCString("GMT%+2d:%2d", hours, minutes), timeZoneID);
> 
> Let me know what you think.

Let's use nsPrintfCString. Apparently ustdio.h is not used in the tree outside intl/icu.

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #22)
> (In reply to Gregory Moore from comment #21)
> > An alternative might be to something like this:
> > CopyASCIItoUTF16(nsPrintfCString("GMT%+2d:%2d", hours, minutes), timeZoneID);
> > 
> > Let me know what you think.
> 
> Let's use nsPrintfCString. Apparently ustdio.h is not used in the tree
> outside intl/icu.

.AppendPrintf would be better.

nsAutoString timeZoneID(L"GMT");
if (aTimeParameters) {
  ...
  timeZoneID.AppendPrintf(...);
}

- You don't have to build a string if totalOffset is smaller than one minute.
- You can drop ugly CopyASCIItoUTF16.

Comment 24

[Gregory Moore [:gmoore]]

Attachment: Updated patch to use AppendPrintf() to create the timezone ID string.

Okay, sure, sounds good. I updated the patch and tested it, and it seems to be working correctly.

Comment 25

[Masatoshi Kimura [:emk]]

Comment on attachment 8830078 [details] [diff] [review]
Updated patch to use AppendPrintf() to create the timezone ID string.

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

Because the logic is not trivial, you should write an automated test to ensure the correct behavior.
This is a C++-only non-XPCOM function, so GTest would be the best choice:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest

::: intl/locale/DateTimeFormatICU.cpp
@@ +131,5 @@
>    }
>  
> +  nsAutoString timeZoneID(u"GMT");
> +  if (aTimeParameters) {
> +    PRInt32 totalOffset = aTimeParameters->tp_gmt_offset + aTimeParameters->tp_dst_offset;

Can you divide totalOffset by 60 here? (Because we ignore seconds.)

@@ +134,5 @@
> +  if (aTimeParameters) {
> +    PRInt32 totalOffset = aTimeParameters->tp_gmt_offset + aTimeParameters->tp_dst_offset;
> +    if (totalOffset != 0) {
> +      PRInt32 hours = totalOffset / 3600;
> +      PRInt32 minutes = ((totalOffset > 0 ? totalOffset : -totalOffset) / 60) % 60;

abc(totalOffset) would be more readable.

@@ +135,5 @@
> +    PRInt32 totalOffset = aTimeParameters->tp_gmt_offset + aTimeParameters->tp_dst_offset;
> +    if (totalOffset != 0) {
> +      PRInt32 hours = totalOffset / 3600;
> +      PRInt32 minutes = ((totalOffset > 0 ? totalOffset : -totalOffset) / 60) % 60;
> +      if (minutes == 0) {

I think we don't have to differentiate minute==0 case (up to you).

Comment 26

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #25)
> abc(totalOffset) would be more readable.

Sorry, I meant abs(totalOffset).

Comment 27

[Masatoshi Kimura [:emk]]

Comment on attachment 8830078 [details] [diff] [review]
Updated patch to use AppendPrintf() to create the timezone ID string.

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

Ah, I forgot one more review comment.

::: intl/locale/DateTimeFormatICU.cpp
@@ +147,5 @@
>    // generate date/time string
>  
>    UErrorCode status = U_ZERO_ERROR;
>  
> +  UDateFormat* dateTimeFormat = udat_open(timeStyle, dateStyle, mLocale->get(), (aTimeParameters ? reinterpret_cast<const UChar*>(timeZoneID.BeginReading()) : nullptr), -1, nullptr, -1, &status);

Is reinterpret_cast really needed? Both nsAString::char_type and UChar are typedef'ed to char16_t IIRC.

Also, you should pass a string length because you already know the length.

Comment 28

[Gregory Moore [:gmoore]]

(In reply to Masatoshi Kimura [:emk] from comment #25)
> Because the logic is not trivial, you should write an automated test to
> ensure the correct behavior.
> This is a C++-only non-XPCOM function, so GTest would be the best choice:

Okay, sure. I have been using GTest to test the code I've been writing anyway, so I will just create a new test and add it to the patch. Which exact part should I be testing though? The part that creates the timezone string? Should I split that off into a separate function so we can call it directly from the GTest file? But I guess it would be a private function so not really sure how we would call it anyway.

Or should I just be testing the function FormatPRExplodedTime(), and pass in PRExplodedTime values set to different timezones? In that case we will need to take into account if ICU is enabled, because if not the formatted string will look different (because it's using DateTimeFormatUnix.cpp). 

> I think we don't have to differentiate minute==0 case (up to you).

Yeah, it seems like we could probably just have one case. A potential reason to have two is that if minutes == 0 then AppendPrintf may just print nothing for the "%02d" part. Then we would have something like "GMT+08:" in that case, which I don't think would be of the form:
> GMT[+|-]hh[[:]mm]
I'll test it to make sure it still works in that case though.

> Is reinterpret_cast really needed? Both nsAString::char_type and UChar are
> typedef'ed to char16_t IIRC.

nsAString::char_type is just char16_t:
https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#36
But I'm not sure about UChar:
http://icu-project.org/apiref/icu4c/umachine_8h_source.html#l00337
It looks like it can potentially be other stuff maybe? Not sure if it would matter even if it was something else though, or if it was changed further along to be defined as something else. If no cast is cleaner and won't cause any problems we should do that though. Let me know, thanks.

Also, couldn't the define's be changed eventually? I'm not sure if that would matter if we weren't casting them in that case though. I don't really know

Comment 29

[Gregory Moore [:gmoore]]

Oops, sorry, ignore the last paragraph in my last message. I forgot to erase that before I clicked submit.

Comment 30

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #28)
> (In reply to Masatoshi Kimura [:emk] from comment #25)
> > Because the logic is not trivial, you should write an automated test to
> > ensure the correct behavior.
> > This is a C++-only non-XPCOM function, so GTest would be the best choice:
> 
> Okay, sure. I have been using GTest to test the code I've been writing
> anyway, so I will just create a new test and add it to the patch. Which
> exact part should I be testing though? The part that creates the timezone
> string? Should I split that off into a separate function so we can call it
> directly from the GTest file? But I guess it would be a private function so
> not really sure how we would call it anyway.
> 
> Or should I just be testing the function FormatPRExplodedTime(), and pass in
> PRExplodedTime values set to different timezones? In that case we will need
> to take into account if ICU is enabled, because if not the formatted string
> will look different (because it's using DateTimeFormatUnix.cpp). 

I think it is sufficient to test public functions (FormatPRExplodedTime in this case).
I'm fine with skipping the test when ICU is disabled.

> > I think we don't have to differentiate minute==0 case (up to you).
> 
> Yeah, it seems like we could probably just have one case. A potential reason
> to have two is that if minutes == 0 then AppendPrintf may just print nothing
> for the "%02d" part. Then we would have something like "GMT+08:" in that
> case, which I don't think would be of the form:
> > GMT[+|-]hh[[:]mm]
> I'll test it to make sure it still works in that case though.

"%02d" should work with the integer value zero.

> > Is reinterpret_cast really needed? Both nsAString::char_type and UChar are
> > typedef'ed to char16_t IIRC.
> 
> nsAString::char_type is just char16_t:
> https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.h#36
> But I'm not sure about UChar:
> http://icu-project.org/apiref/icu4c/umachine_8h_source.html#l00337
> It looks like it can potentially be other stuff maybe? Not sure if it would
> matter even if it was something else though, or if it was changed further
> along to be defined as something else. If no cast is cleaner and won't cause
> any problems we should do that though. Let me know, thanks.

Hm, you are right. I thought we defined UCHAR_TYPE or U_USE_CHAR16_T, but apparently we do not.

Comment 31

[Gregory Moore [:gmoore]]

Attachment: Added an automated test for FormatPRExplodedTime() and made some changes to FormatUDateTime().

Alright, here is a patch with the above suggestions implemented. I'm not really sure about the GTest part; it's kind of confusing and I'm not sure what the best cases to test would be. If you have suggestions for improving it please let me know. Thanks.

Comment 32

[Masatoshi Kimura [:emk]]

Comment on attachment 8832687 [details] [diff] [review]
Added an automated test for FormatPRExplodedTime() and made some changes to FormatUDateTime().

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

I think you should test at least:
1. the timezone is GMT,
2. offset hours are zero and offset minutes are nonzero,
3. offset hours are nonzero and offset minutes are zero, or
4. both offset hours and offset minutes are nonzero
for both negative and positive timezone offset values.

::: intl/locale/moz.build
@@ +14,5 @@
>      DIRS += ['mac']
>  else:
>      DIRS += ['unix']
>  
> +if CONFIG['ENABLE_TESTS']:

Please test ENABLE_INTL_API here,

::: intl/locale/tests/gtest/TestDateTimeFormat.cpp
@@ +1,4 @@
> +#include "gtest/gtest.h"
> +#include "DateTimeFormat.h"
> +
> +#ifdef ENABLE_INTL_API

instead of enclosing the entire source with #ifdef.

@@ +1,5 @@
> +#include "gtest/gtest.h"
> +#include "DateTimeFormat.h"
> +
> +#ifdef ENABLE_INTL_API
> +TEST(TestFormatPRExplodedTime, Regular) {

I recommend TEST(DateTimeFormat, FormatPRExplodedTime)

@@ +7,5 @@
> +  PRExplodedTime prExplodedTime;
> +  PR_ExplodeTime(prTime, PR_GMTParameters, &prExplodedTime);
> +
> +  nsAutoString formattedTime;
> +  nsresult rv = mozilla::DateTimeFormat::FormatPRExplodedTime(kDateFormatLong, kTimeFormatSeconds, &prExplodedTime, formattedTime);

Because DateTimeFormat depends on the current operating system locale, this test didn't pass on my local build.
Could you use FRIEND_TEST or mock to set mLocale to ensure the test will produce the consistent result on all locales?

@@ +9,5 @@
> +
> +  nsAutoString formattedTime;
> +  nsresult rv = mozilla::DateTimeFormat::FormatPRExplodedTime(kDateFormatLong, kTimeFormatSeconds, &prExplodedTime, formattedTime);
> +  ASSERT_TRUE(NS_SUCCEEDED(rv));
> +  ASSERT_TRUE(formattedTime.Equals(u"January 1, 1970 at 12:00:00 AM"));

Please use ASSERT_STREQ for string comparisons.
Note: because GTest does not support char16_t, you should use NS_ConvertUTF16toUTF8 and the char* version of override.

@@ +14,5 @@
> +
> +  prExplodedTime.tm_params.tp_gmt_offset = (7 * 60 * 60) + (37 * 60);
> +  prExplodedTime.tm_params.tp_dst_offset = 1 * 60 * 60;
> +  prExplodedTime.tm_hour = 8;
> +  prExplodedTime.tm_min = 37;

Could you use list initialization to initialize a PRExplodedTime variable? It will make test parameters much easier to read.

Comment 33

[Gregory Moore [:gmoore]]

(In reply to Masatoshi Kimura [:emk] from comment #32)
> I think you should test at least:
> 1. the timezone is GMT,
> 2. offset hours are zero and offset minutes are nonzero,
> 3. offset hours are nonzero and offset minutes are zero, or
> 4. both offset hours and offset minutes are nonzero
> for both negative and positive timezone offset values.

Okay, sure. Just to be certain, I should test (3) OR (4), but not both, as that would unnecessary, right?

> Because DateTimeFormat depends on the current operating system locale, this
> test didn't pass on my local build.
> Could you use FRIEND_TEST or mock to set mLocale to ensure the test will
> produce the consistent result on all locales?

Oh right, I forgot about that. We could use FRIEND_TEST and do:
> FRIEND_TEST(DateTimeFormat, FormatPRExplodedTime)
within DateTimeFormat.h. Then we could set mLocale to "en-US" before running the tests. However, from DXR it doesn't look like FRIEND_TEST is used that much:
https://dxr.mozilla.org/mozilla-central/search?q=FRIEND_TEST&redirect=true
and when it used, it's usually a different version (FRIEND_TEST_ALL_PREFIXES or FRIEND_TEST_WEBRTC). So just want to make sure that was the best option.

In case we shouldn't use FRIEND_TEST, by mock do you mean Google Mock? Or something else? I looked into Google Mock a little but I'm not sure how exactly we would change mLocale that way. Let me know, thanks.

Comment 34

[Masatoshi Kimura [:emk]]

(In reply to Gregory Moore from comment #33)
> Okay, sure. Just to be certain, I should test (3) OR (4), but not both, as
> that would unnecessary, right?

I meant (3) "and" (4). I don't recall why I wrote "or"...

> Oh right, I forgot about that. We could use FRIEND_TEST and do:
> > FRIEND_TEST(DateTimeFormat, FormatPRExplodedTime)
> within DateTimeFormat.h. Then we could set mLocale to "en-US" before running
> the tests. However, from DXR it doesn't look like FRIEND_TEST is used that
> much:
> https://dxr.mozilla.org/mozilla-central/search?q=FRIEND_TEST&redirect=true
> and when it used, it's usually a different version (FRIEND_TEST_ALL_PREFIXES
> or FRIEND_TEST_WEBRTC). So just want to make sure that was the best option.

At least the Gecko Profiler component uses FRIEND_TEST:
https://dxr.mozilla.org/mozilla-central/search?q=FRIEND_TEST+path%3Atools%2Fprofiler%2Fcore&redirect=false

Comment 35

[Gregory Moore [:gmoore]]

Attachment: Made improvements to TestDateTimeFormat.cpp, and fixed bug in FormatUDateTime().

Okay, thanks. Here is a patch with the changes. The test case where the minutes are negative and the hours are zero (e.g., offset of -23 minutes) revealed a bug where because we would just get the absolute value of the minutes, the offset would be positive (e.g., GMT+00:23). I changed it to account for that in the patch.

Comment 36

[Masatoshi Kimura [:emk]]

Comment on attachment 8833706 [details] [diff] [review]
Made improvements to TestDateTimeFormat.cpp, and fixed bug in FormatUDateTime().

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

r=me with the following comments addressed.

::: intl/locale/DateTimeFormatICU.cpp
@@ +137,5 @@
> +  UDateFormat* dateTimeFormat;
> +  if (aTimeParameters) {
> +    nsAutoString timeZoneID(u"GMT");
> +
> +    PRInt32 totalOffsetMinutes = (aTimeParameters->tp_gmt_offset + aTimeParameters->tp_dst_offset) / 60;

Please use standard integer types (such as int32_t) rather than NSPR types (such as PRInt32).
(Sorry, I should have caught this earlier.)

@@ +141,5 @@
> +    PRInt32 totalOffsetMinutes = (aTimeParameters->tp_gmt_offset + aTimeParameters->tp_dst_offset) / 60;
> +    if (totalOffsetMinutes != 0) {
> +      PRInt32 hours = totalOffsetMinutes / 60;
> +      PRInt32 minutes = abs(totalOffsetMinutes) % 60;
> +      if (hours == 0 && totalOffsetMinutes < 0) {

I'd prefer:
  char sign = totalOffsetMinutes < 0 ? '-' : '+';
  int32_t hours = abs(totalOffsetMinutes) / 60;
  ...
  timeZoneID.AppendPrintf("%c%02d:%02d", sign, hours, minutes);

Comment 37

[Gregory Moore [:gmoore]]

Attachment: Updated patch to use standard integer types, and made small change to offset string calculation.

Cool, thanks. Here's the patch.

Comment 38

[Masatoshi Kimura [:emk]]

Comment on attachment 8833812 [details] [diff] [review]
Updated patch to use standard integer types, and made small change to offset string calculation.

Sorry, this patch did not apply to the latest trunk because bug 1332207 added intl/locale/tests/gtest/ on ahead. Could you rebase the patch?

Comment 39

[Gregory Moore [:gmoore]]

Attachment: Rebased patch.

Oh right, sorry about that. I hadn't rebased in a while.

Comment 40

[Gregory Moore [:gmoore]]

Attachment: Fixed build error.

Oh woops, the last patch had a build error that I didn't see. This one should work though.

Comment 41

[Masatoshi Kimura [:emk]]

Comment on attachment 8834177 [details] [diff] [review]
Fixed build error.

Thank you! Kicked a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead15ffc6e11df67ec6cd24ab993bf35417d6d1a

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27775393a94e
Added functionality to FormatPRExplodedTime to take into account timezone information given in the PRExplodedTime parameter. r=emk

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/27775393a94e