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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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?

Comment 1

[Gene Lian (I already quit Mozilla)]

(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.

Comment 2

[Gene Lian (I already quit Mozilla)]

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?

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

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.

Comment 4

[Philipp von Weitershausen [:philikon]]

(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.

Comment 5

[Gene Lian (I already quit Mozilla)]

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. ;)

Comment 6

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, hal::GetTimezone()

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. :)

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Hook up Timezone Settings

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! :)

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Hook up Timezone Settings, V2

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+?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, hal::GetTimezone(), V1.1

Rebase and correct a minor typo error. :cjones already has superreview+ at comment 8.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Hook up Timezone Settings, V3

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.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, hal::GetTimezone(), V1.2

Rebase with the latest codes. :cjones already has superreview+ at comment 8.

Comment 17

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Hook up Timezone Settings, V3.1

Addressing the comment 15.

:khuey already had review+ at comment 15.
:cjones already had superreview+ at comment 11.

Comment 18

[Gene Lian (I already quit Mozilla)]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=afb67c38361a

Comment 19

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b122d9184be
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7f78ac0e78

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b122d9184be
https://hg.mozilla.org/mozilla-central/rev/1e7f78ac0e78