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

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: airpingu, Assigned: skao)

Tracking

unspecified
mozilla27
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=gene])

Attachments

(1 attachment, 6 obsolete attachments)

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? :)
Assignee

Comment 2

6 years ago
sure! I'll take it!
Assignee

Updated

6 years ago
Assignee: nobody → skao
Assignee

Comment 3

6 years ago
: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)
Assignee

Comment 4

6 years ago
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)
Assignee

Updated

6 years ago
Blocks: 926823
Assignee

Comment 6

6 years ago
Posted patch 926777.patch (obsolete) — Splinter Review
Assignee

Updated

6 years ago
Attachment #817051 - Flags: review?(gene.lian)
Assignee

Comment 7

6 years ago
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-
Assignee

Comment 9

6 years ago
Posted patch 926777.patch (obsolete) — Splinter Review
Attachment #817051 - Attachment is obsolete: true
Attachment #817588 - Flags: review?(gene.lian)
Assignee

Comment 10

6 years ago
Posted patch 926777.patch (obsolete) — Splinter Review
Attachment #817588 - Attachment is obsolete: true
Attachment #817588 - Flags: review?(gene.lian)
Attachment #817589 - Flags: review?(gene.lian)
Assignee

Comment 11

6 years ago
Posted patch 926777.patch (obsolete) — Splinter Review
Attachment #817589 - Attachment is obsolete: true
Attachment #817589 - Flags: review?(gene.lian)
Assignee

Comment 12

6 years ago
Posted patch 926777.patch (obsolete) — Splinter Review
Assignee

Updated

6 years ago
Attachment #817617 - Attachment is obsolete: true
Assignee

Comment 13

6 years ago
Posted patch 926777.patch (obsolete) — Splinter Review
Attachment #817618 - Attachment is obsolete: true
Assignee

Updated

6 years ago
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+
Assignee

Comment 15

6 years ago
Attachment #817619 - Attachment is obsolete: true
Attachment #818196 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2b2297d5c1e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 19

6 years ago
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.