926777 - B2G system time: |time.timezone| should save the UTC value with the correct sign

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

In general, we use "UTC+hh:mm" for the time zone on the eastern side of International Time Line instead of "UTC-hh:mm" [1]. For example, the UTC representation for China should be UTC+08:00. However, the Android Kernel has to set the time zone with an opposite sign, so that's why the NITZ has to flip the sign to set the |time.timezone| setting.

Since the |time.timezone| value is exposed to the content, I think we should make the |time.timezone| save the correct sign and flip the sign in TimeZoneSettingObserver, so that the content side can have a better interpretation on the |time.timezone| setting.

AFAIK, the Gaia always sets the |time.timezone| in the format of "region/city", so fixing the bug shouldn't break the compatibility with Gaia.

Comment 1

[Gene Lian (I already quit Mozilla)]

Hi :skao, are you interested in taking this? :)

Comment 2

[Shao Hang Kao [:skao]]

sure! I'll take it!

Comment 3

[Shao Hang Kao [:skao]]

:gene, I'd like to confirm currently only NITZ or SNTP set the time zone with the "UTC[+|-]hh:mm" format right?

Comment 4

[Shao Hang Kao [:skao]]

correct my last comment: only NITZ, SNTP has no idea about the timezone

Comment 5

[Gene Lian (I already quit Mozilla)]

Yes, only NITZ in the Gecko. 

Hi Rudy, I'd like to double check with you. So far, the Gaia end only sets |time.timezone| in the format of "region/city". Right? Do you also set it by the UTC format?

Comment 6

[Shao Hang Kao [:skao]]

Attachment: 926777.patch

Comment 7

[Shao Hang Kao [:skao]]

Created bug 926823 for gaia part.

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 817051 [details] [diff] [review]
926777.patch

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

I think you also need to deal with the initialization. I used to try to add that before at [1]. ;)

[1] https://bugzilla.mozilla.org/attachment.cgi?id=734208&action=diff

Btw, could you please add more comments in the TimeZoneSettingObserver.cpp for "America/Chicago":

// The string that we're interested in will be a JSON string that looks like:
// {"key":"time.timezone","value":"America/Chicago"} or
// {"key":"time.timezone","value":"UTC-05:00"}

and 

// The value should be a JS string like "America/Chicago" or "UTC-05:00".

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1934,5 @@
>      // message.networkTimeZoneInMinutes -= message.networkDSTInMinutes;
>      if (message.networkTimeZoneInMinutes != (new Date()).getTimezoneOffset()) {
>        let absTimeZoneInMinutes = Math.abs(message.networkTimeZoneInMinutes);
>        let timeZoneStr = "UTC";
> +      timeZoneStr += (message.networkTimeZoneInMinutes >= 0 ? "-" : "+");

How about changing ">=" to ">" for "+00:00"?

::: dom/system/gonk/TimeZoneSettingObserver.cpp
@@ +134,5 @@
>    }
>    NS_ConvertUTF16toUTF8 newTimezone(valueStr);
>  
> +  // hal expect different sign from general notations,
> +  // we need to flip it.

s/hal/Hal/
s/expect/expects/
s/different/opposite/
s/we/so we/

@@ +135,5 @@
>    NS_ConvertUTF16toUTF8 newTimezone(valueStr);
>  
> +  // hal expect different sign from general notations,
> +  // we need to flip it.
> +  if (newTimezone.CharAt(3) == '+') {

How about:

if (newTimezone.Find(NS_LITERAL_CSTRING("UTC+")) == 0)

@@ +140,5 @@
> +    if (!newTimezone.SetCharAt('-', 3)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +  }
> +  else if (newTimezone.CharAt(3) == '-') {

else if (newTimezone.Find(NS_LITERAL_CSTRING("UTC-")) == 0)

Comment 9

[Shao Hang Kao [:skao]]

Attachment: 926777.patch

Comment 10

[Shao Hang Kao [:skao]]

Attachment: 926777.patch

Comment 11

[Shao Hang Kao [:skao]]

Attachment: 926777.patch

Comment 12

[Shao Hang Kao [:skao]]

Attachment: 926777.patch

Comment 13

[Shao Hang Kao [:skao]]

Attachment: 926777.patch

Comment 14

[Gene Lian (I already quit Mozilla)]

Comment on attachment 817619 [details] [diff] [review]
926777.patch

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

Thank you! r=gene

Comment 15

[Shao Hang Kao [:skao]]

Attachment: Gecko Patch (ready for checkin)

Comment 16

[Shao Hang Kao [:skao]]

try result: https://tbpl.mozilla.org/?tree=Try&rev=517898f76cb1

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e2b2297d5c1e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2b2297d5c1e

Comment 19

[Jack Liu]

Dears, would you please also land it to v1.2?

Comment 20

[Gene Lian (I already quit Mozilla)]

I don't think we need to land this. Note that we just flip the sign in a more reasonable way. The original codes didn't cause any bugs or regression.

If you really want to do that, we have to land Bug 926823 (Gaia side) as well. These 2 bugs are supposed to be a pair.