Closed Bug 857545 Opened 11 years ago Closed 11 years ago

Gecko should set |time.timezone| in the settings DB with the format of "UTC(+/-)hh:mm)"

Categories

(Firefox OS Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: airpingu, Assigned: airpingu)

Details

Attachments

(1 file, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #847342 +++

Please see Bug 847342, comment #14. We have to ensure the |time.timezone| is uniformly set in the format of "UTC(+/-)hh:mm)" from Gecko, so that the Gaia end can know how to properly parse and display the time zone info on the UI. Currently, the |time.timezone| can only be set by the following 2 possibilities within Gecko:

  - NITZ (RadioInterfaceLayer.js)
  - Initialized by the system time zone (TimeZoneSettingObserver.cpp)

The NITZ is doing that in the correct way. We need to do the same thing for the TimeZoneSettingObserver as well.

This issue blocks another P1-tef+ issue at bug 847342. Nominating for tef+.
Attached patch Patch (obsolete) — Splinter Review
Hi Justin, would you mind taking this review? Thanks!
Attachment #732781 - Flags: review?(justin.lebar+bug)
Blocks a blocker
blocking-b2g: tef? → tef+
># HG changeset patch
># User Gene Lian <clian@mozilla.com>
># Date 1364991800 -28800
># Node ID 12e141d400ed15d03adbe7823d448c95b5ee10cc
># Parent  178a4a770bb1664d431eb0428f057bb48f0ad1ba
>Bug 857545 - Gecko should set |time.timezone| in the format of "UTC(+/-)hh:mm)". r=jlebar

Nit: Could you please say "|time.timezone| in the settings DB" or something?  I was initially pretty confused about what we were setting here.

>diff --git a/dom/system/gonk/TimeZoneSettingObserver.cpp b/dom/system/gonk/TimeZoneSettingObserver.cpp
>--- a/dom/system/gonk/TimeZoneSettingObserver.cpp
>+++ b/dom/system/gonk/TimeZoneSettingObserver.cpp

>@@ -47,28 +47,50 @@ public:
> class TimeZoneSettingCb MOZ_FINAL : public nsISettingsServiceCallback
> {
> public:
>   NS_DECL_ISUPPORTS
> 
>   TimeZoneSettingCb() {}
> 
>   NS_IMETHOD Handle(const nsAString &aName, const JS::Value &aResult) {
>-
>     JSContext *cx = nsContentUtils::GetCurrentJSContext();
>     NS_ENSURE_TRUE(cx, NS_OK);
> 
>     // If we don't have time.timezone value in the settings, we need
>     // to initialize the settings based on the current system timezone
>     // to make settings consistent with system. This usually happens
>     // at the very first boot. After that, settings must have a value.
>     if (aResult.isNull()) {
>-      // Get the current system timezone and convert it to a JS string.
>-      nsCString curTimezone = hal::GetTimezone();
>-      NS_ConvertUTF8toUTF16 utf16Str(curTimezone);
>+      // Get the current system time zone offset. Note that we need to 
>+      // convert the value to a UTC representation in the format of
>+      // "UTC(+/-)hh:mm". Ex, -480 is "UTC-08:00"; 630 is "UTC+10:30".

Nit: "Ex" is not commonly used this way.  "E.g.," or "For example," would be
better.

>+      int32_t timeZoneOffset = hal::GetTimezoneOffset();
>+      uint32_t absTimeZoneOffset = abs(timeZoneOffset);
>+
>+      nsCString curTimeZone;
>+      curTimeZone.AppendLiteral("UTC");
>+      curTimeZone.AppendLiteral(timeZoneOffset >= 0 ? "+" : "-");
>+
>+      nsCString hours;
>+      hours.AppendLiteral("0");
>+      hours.AppendInt(static_cast<uint32_t>
>+        (floor(static_cast<double>(absTimeZoneOffset) / 60)));

Isn't this the same as |absTimeZoneOffset / 60|?

>+      hours.Cut(0, hours.Length() - 2);
>+      curTimeZone.Append(hours);
>+      curTimeZone.AppendLiteral(":");
>+
>+      nsCString mins;
>+      mins.AppendLiteral("0");
>+      mins.AppendInt(absTimeZoneOffset % 60);
>+      mins.Cut(0, mins.Length() - 2);
>+      curTimeZone.Append(mins);

I think this would be a lot simpler if we used nsPrintfCString.  You may even
be able to ensure that all numbers are two digits by using the "%2d" formatter.

This looks great otherwise, but I'd like to take another look at the string
processing after it's converted to nsPrintfCString (or after you determine that
won't work for some reason).
Attachment #732781 - Flags: review?(justin.lebar+bug)
Summary: Gecko should set |time.timezone| in the format of "UTC(+/-)hh:mm)" → Gecko should set |time.timezone| in the settings DB with the format of "UTC(+/-)hh:mm)"
Attached patch Patch, V2 (obsolete) — Splinter Review
Thanks Justin for the nice suggestion!
Attachment #732781 - Attachment is obsolete: true
Attachment #733290 - Flags: review?(justin.lebar+bug)
Comment on attachment 733290 [details] [diff] [review]
Patch, V2

Looks great, with one nit.

>+      nsCString curTimeZone = nsPrintfCString("UTC%+03d:%02d",
>+                                              timeZoneOffset / 60,
>+                                              abs(timeZoneOffset) % 60);

You can do

        nsPrintfCString curTimeZone("UTC%+03d:%02d", ...);
Attachment #733290 - Flags: review?(justin.lebar+bug) → review+
Attached patch Patch, V2.1 (obsolete) — Splinter Review
Addressing the NIT. Carry on r=jlebar. Ready to land.
Attachment #733290 - Attachment is obsolete: true
Attachment #734208 - Flags: review+
Hi Rudy, the Gecko part is ready to land. Just hoping to make sure if we can land this first before the Gaia end is ready. Any backward compatibility issues?
Flags: needinfo?(rlu)
Hi Gene,

Yes, I think it is OK to land this first.
Thanks for looking into this.
Flags: needinfo?(rlu)
Hi Gene,

Beside the format, I remembered that you mentioned that the value set in "time.timezone" in settings will be adjusted by DST (Daylight Saving Time), right?

I am wondering if we could get an unadjusted value, i.e. not affected by DST, so that we could do the mapping to a region, which is needed by Bug 847342.

Say, we need to map the time offset UTC-08:00 to MV,
While during DST, we will get -07:00 in "time.timezone", and I guess we could not map it into MV.
(In reply to Rudy Lu [:rudyl] from comment #9)
> Hi Gene,
> 
> Beside the format, I remembered that you mentioned that the value set in
> "time.timezone" in settings will be adjusted by DST (Daylight Saving Time),
> right?

Yes! The |time.timezone| result has already included the DST.

> 
> I am wondering if we could get an unadjusted value, i.e. not affected by
> DST, so that we could do the mapping to a region, which is needed by Bug
> 847342.

That's possible but are we hoping to use the same setting entry?

> 
> Say, we need to map the time offset UTC-08:00 to MV,
> While during DST, we will get -07:00 in "time.timezone", and I guess we
> could not map it into MV.

I'd like to clarify this more. How do you set the |time.timezone| setting from the Gaia end for a DST or a non-DST timezone? Is that something like |America/Chicago| and you don't need to worry about the DST thing?
(In reply to Rudy Lu [:rudyl] from comment #9)
> Say, we need to map the time offset UTC-08:00 to MV,
> While during DST, we will get -07:00 in "time.timezone", and I guess we
> could not map it into MV.

Btw, during DST, you should get UTC-09:00 instead of UTC-07:00 (for a UTC-08:00 country). Right?
(In reply to Gene Lian [:gene] from comment #10)

> 
> That's possible but are we hoping to use the same setting entry?
> 

I think we could still use the same entry.
It might be good if we could separate the value that Gecko set (from NITZ or system setting) and the value Gaia set to inform the Gecko to change the time zone.
That will make debugging easier, but not a requirement. 

> > 
> > Say, we need to map the time offset UTC-08:00 to MV,
> > While during DST, we will get -07:00 in "time.timezone", and I guess we
> > could not map it into MV.
> 
> I'd like to clarify this more. How do you set the |time.timezone| setting
> from the Gaia end for a DST or a non-DST timezone? Is that something like
> |America/Chicago| and you don't need to worry about the DST thing?

Yes, as you said, we only set it using the "region/city" format.


(In reply to Gene Lian [:gene] from comment #11)

> Btw, during DST, you should get UTC-09:00 instead of UTC-07:00 (for a UTC-08:00 
> country). Right?

Now, I am a little confused by the formatting again. :)

I think CST (the time zone Taiwan is in) should be UTC+08:00?
However, from my test (in Taiwan, NITZ from CHT mobile network), it will get UTC-08:00.

We can talk about this on IRC if you want.
Thanks.
(In reply to Rudy Lu [:rudyl] from comment #12)
> I think CST (the time zone Taiwan is in) should be UTC+08:00?
> However, from my test (in Taiwan, NITZ from CHT mobile network), it will get
> UTC-08:00.

In general, we use "UTC+08:00" for Taiwan (UTC+ for the eastern side of International Date Line [1]). However, we have to use "UTC-08:00" to set for the Android Kernel. I think exposing "UTC+08:00" to the setting value for Taiwan sounds more correct. We need to correct it as well.

[1] http://en.wikipedia.org/wiki/Time_zone#Offsets_from_UTC
Hi Gene,

For your question on IRC,
> , if I also correct the sign of time.timezone, is there any side effect for Gaia

No, right now, we did not read this value when its format is UTC[-+]xx:xx, or CST+08.

> how do you map the time.timezone back to MV? Do we really need to separate the DST info?

Yes, DST should be separated if possible, we need to get UTC-08:00 for MV all the time, then we can map -08:00 to MV.

> why not just showing "UTC..."
Yes, that's another proposal, but per our UX design, right now we are going to implement the mapping.

Thank you.
All right. The objective is getting much more complicated now. I need more time to provide a new solution to address the new requirements. Please stay tuned.
Hi Gene,

On a second thought, it turns out that we might not need 2 entries for the info.
One with DST and one without, please see the example below.

               w/o DST   w/ DST
MV             -08:00    -07:00
Metlakatla     -08:00    -08:00

During DST, we would get -08:00, if that is info we could get from Gaia, we would not know which city to map to.

Do you think this makes sense from your point of view?

Thanks.
(In reply to Rudy Lu [:rudyl] from comment #16)
> Hi Gene,
> 
> On a second thought, it turns out that we might not need 2 entries
                                            ^^^^^^^^^
Do you mean "might" instead of "might not"?

If it's just a typo, my initiative design is we can still use the same entry but will give you a something like:

  UTC-08:00+01:00

The first part is its original DST-excluded time zone and the second part represents the DST effect. What do you think? Please let me know. Thanks!
Hi Gene,

Sorry for typo.
It should be "might need 2 entries for the info.".
Yes, the format you proposed should be able to work.

Thanks.
Hi Gene,

FYI, we may take a simpler way to resolve Bug 847342, see Bug 847342#Comment 19 for details.

So, I think no rush for this issue to be resolved.
Thank you.
(In reply to Rudy Lu [:rudyl] from comment #19)
> FYI, we may take a simpler way to resolve Bug 847342, see Bug 847342#Comment
> 19 for details.

I don't like that work-around. :(

> 
> So, I think no rush for this issue to be resolved.

I'm hoping to provide a patch before this weekend at least and start the review. Sorry I'm busy with the W3C SysApps Meeting recently.
Attached patch Patch, V3 (obsolete) — Splinter Review
Hi Justin,

I provided a new patch because two additional objectives need to be considered:

1. Please see comment #16 and comment #17. We need a way to expose both the ordinary time zone and the DST info, so that the Gaia can map it to a more accurate region/city. To do so, I decided to make the |time.timezone| setting be a UTC representation in the format of "UTC{+,-}hh:mm{+,-}hh:mm", where the first "{+,-}hh:mm" part represents the ordinary time zone and the second part represents the DST shift.

2. Please see comment #13. 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]). However, we have to use the opposite sign to set the time zone in the Android Kernel (just like what the original code does). I suggest we should expose the the UTC representation in a more correct way to the |time.timezone| setting.

Other notes:

1. I found we cannot use nsPrintfCString("%+03d:%02d", ...) for the time zone offset -30 because "hh" is actually "00" which cannot decide the right sign.

2. I've not yet tested the code but I'll make sure it does work tomorrow. Could you please return me some quick feedback to let me know this approach is reasonable or not? Thanks!

3. The issue title and commit message is not yet changed.

[1] http://en.wikipedia.org/wiki/Time_zone#Offsets_from_UTC
Attachment #734208 - Attachment is obsolete: true
Attachment #736573 - Flags: feedback?(justin.lebar+bug)
Attached patch Patch, V3.1Splinter Review
Please see comment #21 for the summary. Thanks for your time!
Attachment #736573 - Attachment is obsolete: true
Attachment #736573 - Flags: feedback?(justin.lebar+bug)
Attachment #736582 - Flags: feedback?(justin.lebar+bug)
It occurs to me that this data probably shouldn't be in a setting.

If Gaia wants a timezone that's better than |new Date().getTimezoneOffset()|, we should expose that to the web, instead of keeping it to ourselves.

I wonder if all we need to do here is write something like Date.getDSTOffset().  With that, plus getTimezoneOffset(), you should be able to get exactly as much information as we're trying to expose via the settings API.

There's a separate question about whether and how to notify code that the timezone / DST has changed; the settings API may or may not be the right API for that.

dbaron, do you have any thoughts on this?
(In reply to Justin Lebar [:jlebar] from comment #23)
> It occurs to me that this data probably shouldn't be in a setting.

I assume you're talking about the DST part. Right? Because we still need an entry in the settings to hook up to the system time zone.

> I wonder if all we need to do here is write something like
> Date.getDSTOffset().  With that, plus getTimezoneOffset(), you should be
> able to get exactly as much information as we're trying to expose via the
> settings API.

Indeed, having an extra Date.getDSTOffset() may solve the issue. However, I'm not sure how long it'll take to finish the patch and reviews since it must touch the DOM interface, though. I'm fine with that if we don't really need to consider the tef+ timeline for now.

> There's a separate question about whether and how to notify code that the
> timezone / DST has changed; the settings API may or may not be the right API
> for that.

Setting API can already do that. Whenever the |time.timezone| is modified (no matter it's modified by Gaia's manual setting or by Gecko's NITZ, an "mozsettings-changed" observer topic will be fired to all the observers that used to register to listen to the |time.timezone| change. The content end can also listen to that through:

  navigator.mozSettings.addObserver('time.timezone', ...)
>> There's a separate question about whether and how to notify code that the
>> timezone / DST has changed; the settings API may or may not be the right API
>> for that.
> 
> Setting API can already do that.

Right, but the idea is to use the settings API as little as possible, because it's a privileged API that is not exposed to web content.

If we've established that there is a use-case for noticing when the system timezone changes, we should allow non-privileged apps and normal web content to get that same notification.

> we still need an entry in the settings to hook up to the system time zone.

With our current design, yes.  But it's obviously causing us problems, so we should think about alternatives.

> However, I'm not sure how long it'll take to finish the patch and reviews since it must touch the 
> DOM interface, though. I'm fine with that if we don't really need to consider the tef+ timeline 
> for now.

The hack here isn't a big change; let's do this and then file a bug for doing this the right way.

> 1. I found we cannot use nsPrintfCString("%+03d:%02d", ...) for the time zone offset -30 because 
> "hh" is actually "00" which cannot decide the right sign.

How about nsPrintfCString("%s%02d:%02d", signStr, ...), where signStr is "+", "-", or "", as appropriate?  I'm not wed to using nsPrintfCString, but I'd like the code to be clean.
(In reply to Justin Lebar [:jlebar] from comment #25)
> > we still need an entry in the settings to hook up to the system time zone.
> 
> With our current design, yes.  But it's obviously causing us problems, so we
> should think about alternatives.

Yes. I think it's a general issue for lots of settings within the Settings API (not only for the time zone setting). Not sure if we currently have any road map to get rid the overuse of the Settings API.

> > However, I'm not sure how long it'll take to finish the patch and reviews since it must touch the 
> > DOM interface, though. I'm fine with that if we don't really need to consider the tef+ timeline 
> > for now.
> 
> The hack here isn't a big change; let's do this and then file a bug for
> doing this the right way.

Sounds good! I'll fire that later. For this bug, I will hope to simply correct the issue #2 at comment #21 to flip the sign to have a better UTC representation in general.
 
> How about nsPrintfCString("%s%02d:%02d", signStr, ...), where signStr is
> "+", "-", or "", as appropriate?  I'm not wed to using nsPrintfCString, but
> I'd like the code to be clean.

Yes! You're saying what I'm doing. :)
> Yes. I think it's a general issue for lots of settings within the Settings API (not only for the 
> time zone setting). Not sure if we currently have any road map to get rid the overuse of the 
> Settings API.

Let's file a metabug and get to work!  We can start with timezones.  That's enough of a roadmap for me.  :)
This setting is becoming pretty complicated.  I think it's becoming complicated because we're storing a user-consumable value in the setting.  Things will break if one of our consumers doesn't store the value in the setting with the exact formatting we're expecting, and I think that's bad.

What if we had two settings, "timezone" and "dst_offset".  They'd each be an integer number of minutes.  So for example, Eastern Standard Time is GMT-5, and during daylight savings time, it changes to Eastern Daylight Time, GMT-4.

So if we're in EST, we'd store timezone=300, dst_offset=0.  But if we're in EDT, we'd store timezone=240, dst_offset=-60.

Gaia can then format this however it wants.
Attachment #736582 - Flags: feedback?(justin.lebar+bug)
not blocking as it no longer blocks 847342
blocking-b2g: tef+ → ---
(In reply to Justin Lebar [:jlebar] from comment #28)
> So if we're in EST, we'd store timezone=300, dst_offset=0.  But if we're in
> EDT, we'd store timezone=240, dst_offset=-60.

Hi Justin,

Actually, in the current design, Gaia can set |time.timezone| to a value like "America/Chicago". This kind of format can be directly accepted by the Android Kernel. On the other hand, the value set by NITZ from internal Gecko cannot know the "region/city" format, because the carrier simply sends a number-like offset to us.

If we only allow number-like offsets for this setting, then Gaia might need to maintain a "region/city" -> number table, which doesn't seem quite reasonable.

Since the bug 847342 has another way to work around, for this bug I'm hoping to simply correct the |time.timezone| to be in the format of "UTC{+,-}hh:mm" (DST included), which means the |time.timezone| can only be either the following 2 types of strings:

  - "region/city" (set by Gaia)
  - "UTC{+,-}hh:mm" (set by Gecko)

In the long term, we need to fix the following 2 bugs to support the time zone setting without the usage of Settings API:

  - Bug 861812 - Provide .getDSTOffset() for the Date object
  - Bug 861795 - Provide a new way to get the system time zone change instead of overusing the Settings API

Does that make sense to you? Anyway, this bug is no longer tef+. I'll put lower priority for this and switch to support the MMS-related bugs.
Flags: needinfo?(justin.lebar+bug)
If we don't need this hack, should we just close this bug?
Flags: needinfo?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #31)
> If we don't need this hack, should we just close this bug?

Indeed, it doesn't solve a real bug. All right, let's close this bug. Thanks for your time. :)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
(In reply to Justin Lebar [:jlebar] from comment #23)
> It occurs to me that this data probably shouldn't be in a setting.
> 
> If Gaia wants a timezone that's better than |new
> Date().getTimezoneOffset()|, we should expose that to the web, instead of
> keeping it to ourselves.
> 
> I wonder if all we need to do here is write something like
> Date.getDSTOffset().  With that, plus getTimezoneOffset(), you should be
> able to get exactly as much information as we're trying to expose via the
> settings API.
> 
> There's a separate question about whether and how to notify code that the
> timezone / DST has changed; the settings API may or may not be the right API
> for that.
> 
> dbaron, do you have any thoughts on this?

I think the bug as stated is probably a bad idea -- for any device not getting its time data from NITZ, you're going to need more complex timezone rules than UTC(+/-)hh:mm -- you need the information that's encoded behind the TZ database identifiers, which say when the UTC offset changes.


As far as exposing it to the Web, there's some ongoing work in the Ecmascript internationalization work to do that, although I'm not a huge fan of what I've seen there so far; see https://mail.mozilla.org/pipermail/es-discuss/2013-March/028978.html
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: