Closed Bug 926777 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: airpingu, Assigned: skao)

References

Details

(Whiteboard: [mentor=gene])

Attachments

(1 file, 6 obsolete files)

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.
Hi :skao, are you interested in taking this? :)
sure! I'll take it!
Assignee: nobody → skao
:gene, I'd like to confirm currently only NITZ or SNTP set the time zone with the "UTC[+|-]hh:mm" format right?
Flags: needinfo?(gene.lian)
correct my last comment: only NITZ, SNTP has no idea about the timezone
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?
Flags: needinfo?(gene.lian)
Blocks: 926823
Attached patch 926777.patch (obsolete) — Splinter Review
Attachment #817051 - Flags: review?(gene.lian)
Created bug 926823 for gaia part.
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)
Attachment #817051 - Flags: review?(gene.lian) → review-
Attached patch 926777.patch (obsolete) — Splinter Review
Attachment #817051 - Attachment is obsolete: true
Attachment #817588 - Flags: review?(gene.lian)
Attached patch 926777.patch (obsolete) — Splinter Review
Attachment #817588 - Attachment is obsolete: true
Attachment #817588 - Flags: review?(gene.lian)
Attachment #817589 - Flags: review?(gene.lian)
Attached patch 926777.patch (obsolete) — Splinter Review
Attachment #817589 - Attachment is obsolete: true
Attachment #817589 - Flags: review?(gene.lian)
Attached patch 926777.patch (obsolete) — Splinter Review
Attachment #817617 - Attachment is obsolete: true
Attached patch 926777.patch (obsolete) — Splinter Review
Attachment #817618 - Attachment is obsolete: true
Attachment #817619 - Flags: review?(gene.lian)
Comment on attachment 817619 [details] [diff] [review]
926777.patch

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

Thank you! r=gene
Attachment #817619 - Flags: review?(gene.lian) → review+
Attachment #817619 - Attachment is obsolete: true
Attachment #818196 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2b2297d5c1e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Dears, would you please also land it to v1.2?
blocking-b2g: --- → koi?
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.
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.