Closed Bug 783021 Opened 7 years ago Closed 7 years ago

System time: listen to timezone settings changes and update system timezone

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: philikon, Assigned: airpingu)

References

Details

(Whiteboard: [LOE:M])

Attachments

(2 files, 6 obsolete files)

As a precursor to bug 714357 we can let Gaia set the timezone using the Settings API. Gecko should then just listen to that setting change and update the system timezone accordingly. This should be all that's required for v1. At a later time we could revisit bug 714357 to also provide timezone enumeration.

Tentatively assigning to Gene. Gene, would you mind taking a look at this?
blocking-basecamp: ? → +
(In reply to Philipp von Weitershausen [:philikon] from comment #0)
> As a precursor to bug 714357 we can let Gaia set the timezone using the
> Settings API. Gecko should then just listen to that setting change and
> update the system timezone accordingly. This should be all that's required
> for v1. At a later time we could revisit bug 714357 to also provide timezone
> enumeration.
> 
> Tentatively assigning to Gene. Gene, would you mind taking a look at this?

Pleasure :) I used to have some experiences on both settings hooking up and timezone handling (when doing Alarm API). Btw, I believe this one should highly depend on Bug 714358, which provides the system-level functions for updating timezone and listening to timezone changed.
Depends on: 714358
Whiteboard: [LOE:M]
Hi :philikon :)

I'd like to ask one quick question: do we need to set the timezone based on the current settings when powering up? I guess we need to do this so that we can make sure the setting and the system timezones are always consistent under any circumstances. Right?
Attached patch WIP Patch (obsolete) — Splinter Review
Just a WIP patch and not yet cleaned-up. We still need to find a way to get the system timezone to be assigned to the settings (time.timezone) at the very first powering up.
(In reply to Gene Lian [:gene] from comment #2)
> I'd like to ask one quick question: do we need to set the timezone based on
> the current settings when powering up? I guess we need to do this so that we
> can make sure the setting and the system timezones are always consistent
> under any circumstances. Right?

Yeah that sounds right to me.
Some objectives summarized as below:

1. If Gaia sets the |time.timezone| by settings API, then Gecko will dynamically set the system timezone based on the new setting value.

2. At boot time, Gecko also needs to check the current settings in order to start with a known value, so that users can restore the last timezone setting after rebooting.

3. If we don't have time.timezone value in the settings at boot time, we need to intilize the settings based on the current system timezone. This usually happens at the first boot. After that, settings must have a value.

I should be able to upload patches for solving this issue. ;)
No longer depends on: 714358
Attached patch Part 1, hal::GetTimezone() (obsolete) — Splinter Review
Hi Cjones,

May I invite you to review the hal::GetTimezone() part? :) We must need this API because of the objective #3 in comment #5. Another benefit is if we can pre-know the system timezone before setting it, we can avoid setting the identical value and avoid sending unnecessary timezone-changed event (bug 714358). Please feel free to let me know if you don't have enough bandwidth to review this one. :)
Attachment #654551 - Flags: review?(jones.chris.g)
Hi :qDot and :cjones,

May I invite you guys to review this patch? You can find the detailed objectives at comment #5 for your convenience of reviews. Please feel free to let me know if you have any suggestions or comments. Also, please recommend other reviewers you don't have enough bandwidth to go through this patch. Thanks for your help in advance! :)
Attachment #654176 - Attachment is obsolete: true
Attachment #654555 - Flags: superreview?(jones.chris.g)
Attachment #654555 - Flags: review?(kyle)
Comment on attachment 654551 [details] [diff] [review]
Part 1, hal::GetTimezone()

I wish we could use libc for this and just use hal to set the timezone, but this is the safer way to go.  We can revisit in the future.

Looks good.
Attachment #654551 - Flags: review?(jones.chris.g) → review+
Comment on attachment 654555 [details] [diff] [review]
Part 2, Hook up Timezone Settings

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

>+#ifdef MOZ_WIDGET_GONK
>+static nsRefPtr<TimeSetting> sTimeSetting;

Use StaticRefPtr instead.

> #ifdef MOZ_WIDGET_GONK
>   ShutdownAutoMounter();
>+  sTimeSetting = NULL;

nullptr

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

>+NS_IMPL_THREADSAFE_ISUPPORTS1(InitTimezoneCb, nsISettingsServiceCallback)
>+

Why does this need to be thread safe?  It shouldn't, let's remove
THREADSAFE.

>+TimeSetting::TimeSetting()
>+{
>+  // Setup an observer to watch changes to the setting.
>+  nsCOMPtr<nsIObserverService> observerService =
>+    mozilla::services::GetObserverService();

s/mozilla::services/services::/.

>+TimeSetting::~TimeSetting()
>+{
>+  nsCOMPtr<nsIObserverService> observerService =
>+    mozilla::services::GetObserverService();

s/mozilla::services/services::/.

Overall looks OK.  sr=me with that.
Attachment #654555 - Flags: superreview?(jones.chris.g) → superreview+
Hi Cjones,

Thanks for your quick reviews. New patch is uploaded to include your suggested changes. Please feel free to let me know if I can do anything better. :) Btw, do we need to ask another reviewer if I've already had your superreview+?
Attachment #654555 - Attachment is obsolete: true
Attachment #654555 - Flags: review?(kyle)
Attachment #655939 - Flags: superreview?(jones.chris.g)
Comment on attachment 655939 [details] [diff] [review]
Part 2, Hook up Timezone Settings, V2

Yes, unfortunately you need someone to take a close look at your JSAPI usage here; I don't know JSAPI well enough.
Attachment #655939 - Flags: superreview?(jones.chris.g)
Attachment #655939 - Flags: superreview+
Attachment #655939 - Flags: review?(khuey)
Comment on attachment 655939 [details] [diff] [review]
Part 2, Hook up Timezone Settings, V2

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

I'm a bit confused here.  In multiple places you have a JSContext that is passed in via an IDL function that is [implicit_jscontext], but you get and use the safe JSContext.  Why?

::: dom/system/gonk/TimeSetting.cpp
@@ +36,5 @@
> +  InitTimezoneCb() {}
> +
> +  NS_IMETHOD Handle(const nsAString &aName, const JS::Value &aResult, JSContext *aContext) {
> +    // If we don't have time.timezone value in the settings, we need 
> +    // to intilize the settings based on the current system timezone 

"initialize"

@@ +37,5 @@
> +
> +  NS_IMETHOD Handle(const nsAString &aName, const JS::Value &aResult, JSContext *aContext) {
> +    // If we don't have time.timezone value in the settings, we need 
> +    // to intilize the settings based on the current system timezone 
> +    // to make settings consistent with system. This usually happens 

Extra whitespace at the end of line here.

@@ +47,5 @@
> +      if (!stack) {
> +        ERR("Failed to get JSContextStack");
> +        return NS_OK;
> +      }
> +      JSContext *cx = stack->GetSafeJSContext();

Why are you using the safe JSContext here when you receive aContext?
Attachment #655939 - Flags: review?(khuey) → review-
Attached patch Part 1, hal::GetTimezone(), V1.1 (obsolete) — Splinter Review
Rebase and correct a minor typo error. :cjones already has superreview+ at comment 8.
Attachment #654551 - Attachment is obsolete: true
Attachment #656812 - Flags: superreview+
Hi Kyle,

As discussed on the IRC, I tried to reuse the existing JS context instead of creating a new one. Thanks for your suggestions and reviews. :)

Cjones already had superreview+ at comment 11.
Attachment #655939 - Attachment is obsolete: true
Attachment #656814 - Flags: superreview+
Attachment #656814 - Flags: review?(khuey)
Comment on attachment 656814 [details] [diff] [review]
Part 2, Hook up Timezone Settings, V3

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

::: dom/system/gonk/TimeSetting.cpp
@@ +177,5 @@
> +    return NS_OK;
> +  }
> +  JSBool match;
> +  if (!JS_StringEqualsAscii(cx, key.toString(), TIME_TIMEZONE, &match) ||
> +      (match != JS_TRUE)) {

!= has higher precedence than ||, so that set of parentheses is unnecessary.

::: dom/system/gonk/TimeSetting.h
@@ +4,5 @@
> +
> +#ifndef mozilla_system_timesetting_h__
> +#define mozilla_system_timesetting_h__
> +
> +#include "jsapi.h"

Please just include jspubtd.h here.  It should have everything you need.
Attachment #656814 - Flags: review?(khuey) → review+
Rebase with the latest codes. :cjones already has superreview+ at comment 8.
Attachment #656812 - Attachment is obsolete: true
Attachment #658016 - Flags: superreview+
Addressing the comment 15.

:khuey already had review+ at comment 15.
:cjones already had superreview+ at comment 11.
Attachment #656814 - Attachment is obsolete: true
Attachment #658018 - Flags: superreview+
Attachment #658018 - Flags: review+
Flags: in-testsuite-
Keywords: checkin-needed
Blocks: 789973
Blocks: 792290
You need to log in before you can comment on or make changes to this bug.