801096 - [settings] Set-Automatically for time should take effect *immediately* instead of waiting on next NITZ coming

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Andreas Gal :gal]

Also, it seems to take quite some time for this setting to take effect and see the time update. Still not seeing the change after flipping the switch.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I think comment 1 might have to do with a lot of the invalidation issues we see in the settings.  bug 796831

Comment 3

[Gene Lian (I already quit Mozilla)]

(In reply to Andreas Gal :gal from comment #1)
> Also, it seems to take quite some time for this setting to take effect and
> see the time update. Still not seeing the change after flipping the switch.

The current auto-update mechanism (Bug 789973) is we need to receive a real NITZ timestamp to dynamically update the time-clock/timezone even if the auto-update setting is enabled (the setting is only in charge of whether to apply NITZ whenever receiving it).

I think I can cache the latest NITZ timestamp and apply it when the auto-update setting is enabled, so that users can get an immediate effect on the screen.

We also need Bug 790499 to be done so that everything we set for the B2G system time can be correctly reflected on the UI, which is an essential part I think.

Comment 4

[Gene Lian (I already quit Mozilla)]

Just taking a note here. This issue is much difficult than we thought. The problem and the solution is:

1. Supposing we got the following NITZ and cache that:

  nitz.networkTimeInMS = 100
  nitz.receiveTimeInMS = 100

2. Users manually set the system time to 500.

3. When applying auto-update to restore the cached NITZ. We shouldn't set it to:

  nitz.networkTimeInMS + (Now() - nitz.receiveTimeInMS) = 100 + (500 - 100) = 500
  
This is wrong because the |nitz.receiveTimeInMS| has been rebased because of step #2. So the correct way is we need to dynamically update |nitz.receiveTimeInMS| whenever the system time is manually reset:

  nitz.receiveTimeInMS = nitz.receiveTimeInMS + (500 - 100) = 500

As a result, the correct time we want to restore is:
  
  nitz.networkTimeInMS + (Now() - nitz.receiveTimeInMS) = 100 + (500 - 500) = 100

In summary, the internal Gecko need a way to dynamically listen to the clock time change so that we can update the cached |nitz.receiveTimeInMS|.

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Notify Time Change for Internal Use

Hi Justin,

We need to fire an observer message to notify the time clock change for the internal use within Gecko (please refer to comment #4 for why we need that). The changes are summarized as below:

1. It doesn't seem very comfortable that sometimes we use |SystemTime| and sometimes we use |SystemTimeChange|. Let's clean up s/SystemTime/SystemTimeChange for everywhere uniformly.

2. The hal-level observer will now notify the following new structure, where the new second variable |clockDeltaTimeInMS| will carry the clock time diff.

  struct SystemTimeChangeInformation {
    SystemTimeChangeReason reason;
    int64_t                clockDeltaTimeInMS;
  };

3. We'll fire an observer message "system-time-change-clock" to notify the time clock change when calling |nsSystemTimeChangeObserver::Notify()|.

Could you please help review this one when you have a chance? Really appreciate!

Comment 6

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Apply Cached NITZ whenever Auto-Update is Enabled

Hi Philikon,

Please see comment #3 for the main purpose of this issue. We need to immediately restore the latest cached NITZ whenever the auto-update is enabled, so that users can have a better experience of using the auto-update instead of waiting on the next NITZ coming to trigger the time update.

Please also see comment #4 for why we need to dynamically rebase the |nitz.receiveTimeInMS|, which needs to listen to an "system-time-change-clock" observer message sent from the system whenever the system time is reset.

Could you please take a review on this patch when you're available? Thanks a lot!

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 672239 [details] [diff] [review]
Part 1, Notify Time Change for Internal Use

The code here looks fine, but it seems to me that we actually want two
separarate events:

  SystemTimeChange(clockDeltaMS) (maybe SystemClockChange would be better)
  SystemTimezoneChange() (or maybe it has (oldTimezone, newTimezone) as args)

Because as it is, one of the observers cares only about reason != TZ, and the
other has special code only for reason == SYSTEM_CLOCK.

Also, as it is, we send clockDeltaTimeInMS == 0 when the timezone changes,
which is bad.  But the fact that we're not using all our args when the timezone
changes indicates to me that timezone should probably be a separate event.

What do you think?

>diff --git a/dom/time/TimeChangeObserver.h b/dom/time/TimeChangeObserver.h
>-typedef mozilla::Observer<mozilla::hal::SystemTimeChange> SystemTimeChangeObserver;
>+using namespace mozilla::hal;

Please don't do |using namespace| inside a header file.  This causes all code
processed after this header to also have |using namespace mozilla::hal|, which
largely defeats the purpose of namespaces.

>diff --git a/dom/time/TimeChangeObserver.cpp b/dom/time/TimeChangeObserver.cpp
> void
>-nsSystemTimeChangeObserver::Notify(const SystemTimeChange& aReason)
>+nsSystemTimeChangeObserver::Notify(const SystemTimeChangeInformation& aSystemTimeChangeInfo)
> {
>+  // Fire an observer message to notify the system time clock has been changed.

Maybe:

       Notify observers that the system clock has been adjusted.

(At the very least, "time clock" is redundant, and you need some word, probably
"that", between "notify" and "the".)

ALso, please move this comment inside the if statement.

>+  if (aSystemTimeChangeInfo.reason() == SYS_TIME_CHANGE_CLOCK) {
>+    nsCOMPtr<nsIObserverService> observerService = GetObserverService();
>+    if (observerService) {
>+      nsString dataStr;
>+      dataStr.AppendFloat(
>+        static_cast<double>(aSystemTimeChangeInfo.clockDeltaTimeInMS()));
>+      observerService->NotifyObservers(
>+        nullptr, "system-time-change-clock", dataStr.get());

Maybe just "system-clock-change"?

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 672241 [details] [diff] [review]
Part 2, Apply Cached NITZ whenever Auto-Update is Enabled

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

Nice work!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1360,5 @@
>          Services.obs.removeObserver(this, kMozSettingsChangedObserverTopic);
> +        Services.obs.removeObserver(this, kSysTimeChangeClockObserverTopic);
> +        break;
> +      case kSysTimeChangeClockObserverTopic:
> +        this._nitzMessage.receiveTimeInMS += parseInt(data);

Always pass in the base, please. Like so:

  parseInt(data, 10);

r=me with that.

@@ +1393,5 @@
>    // Flag to determine whether to use NITZ. It corresponds to the
>    // 'time.nitz.automatic-update.enabled' setting from the UI.
>    _nitzAutomaticUpdateEnabled: null,
>  
> +  // Variable to cache the latest NITZ message whenever recerving it.

Nit: typo in "recerving". Also, perhaps this might be a better comment:

  // Remember the last NITZ message so that we can set the time according to the network immediately when the user enabled network-based time.

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to Justin Lebar [:jlebar] from comment #7)
> Comment on attachment 672239 [details] [diff] [review]
> Part 1, Notify Time Change for Internal Use
> 
> The code here looks fine, but it seems to me that we actually want two
> separarate events:
> 
>   SystemTimeChange(clockDeltaMS) (maybe SystemClockChange would be better)
>   SystemTimezoneChange() (or maybe it has (oldTimezone, newTimezone) as args)
> 
> Because as it is, one of the observers cares only about reason != TZ, and the
> other has special code only for reason == SYSTEM_CLOCK.
> 
> Also, as it is, we send clockDeltaTimeInMS == 0 when the timezone changes,
> which is bad.  But the fact that we're not using all our args when the
> timezone
> changes indicates to me that timezone should probably be a separate event.
> 
> What do you think?

Thanks Justin for the quick review and good points! :D

Actually, I don't know either why it was designed as a single observer in the first place (Steven might be able to explain more about it), but I guess both clock or timezone changes imply a kind of system time change, which means system always needs to have some corresponding behaviors no matter either of them is reset.

So do you prefer let's separate the current observer into separate ones from now on (which might be a big change but I'm glad to do it)? Or just keeping the current work? If latter, the following constructure is more like a concept of time-changing *snapshot*, though.

  struct SystemTimeChangeInformation {
    SystemTimeChangeReason reason;
    int64_t                clockDeltaTimeInMS;
    int32_t                oldTimezoneInMinutes;
    int32_t                newTimezoneInMinutes;
  };

For each SYS_TIME_CHANGE_CLOCK change: {SYS_TIME_CHANGE_CLOCK, 9999, -480, -480}
For each SYS_TIME_CHANGE_TZ change: {SYS_TIME_CHANGE_TZ, 0, -480, 360}

Please feel free to let me know your preference. :)

Comment 10

[Justin Lebar (not reading bugmail)]

> but I guess both clock or timezone changes imply a kind of system time change, which means system 
> always needs to have some corresponding behaviors no matter either of them is reset.

That was probably the original thinking.  But it's not born out by your code; one of the observers explicitly only cares about timezone notifications!


I don't think having a struct which says "if reason == X, then ignore fields A and B, and if reason == Y, ignore fields C and D".  That just means we should have two structs, and in this case, two events.

If that turns out to be a huge amount of work, then I'd be happy to consider our alternatives.  But ISTM that you touch all of the code that would need to change in this patch, since you changed the name of this struct in the patch.  I hope I'm not underestimating the amount of work from my spot on the sidelines!

If this is a week-long rabbit hole, let's do something easy.  But if we can do it in a day or two, I'd much rather have a better design.

Comment 11

[Gene Lian (I already quit Mozilla)]

(In reply to Justin Lebar [:jlebar] from comment #10)
> If this is a week-long rabbit hole, let's do something easy.  But if we can
> do it in a day or two, I'd much rather have a better design.

Thanks Justin! No problem! Let's do it :) I should be able to refine everything today. ;)

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Notify Time Change for Internal Use, V2

Hi Justin,

1. Now we have the 2 separate observers (one for system clock and the other for system timezone):

typedef Observer<int64_t> SystemClockChangeObserver;
typedef Observer<SystemTimezoneChangeInformation> SystemTimezoneChangeObserver;

2. I move the AlarmHalService::GetTimezoneOffset() to the SetTimezone(), which is very useful to calculate the |oldTimezoneMinutes| and |newTimezoneMinutes|.

3. Remove some unnecessary |mozilla::| or |hal::| that already have namespaces included.

4. s/nsIAlarmHalService/AlarmHalService for AlarmHalService::GetInstance().

Please feel free to let me know if any tiny details can be improved. Thanks! :)

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Apply Cached NITZ whenever Auto-Update is Enabled, V1.1

Addressing comment 8. r=philikon

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Notify Time Change for Internal Use, V2.1

Sorry. Fixing some building errors for try builds. Please comment 12. Thanks!

Comment 15

[Gene Lian (I already quit Mozilla)]

Change the title to a more descriptive one.

Comment 16

[Justin Lebar (not reading bugmail)]

Looks great; just a few nits.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
>+struct SystemTimezoneChangeInformation {
>+  int32_t oldTimezoneMinutes;
>+  int32_t newTimezoneMinutes;
>+};

I think oldTimezoneOffsetMinutes and newTimezoneOffsetMinutes would be better
names -- timezones don't have minutes; they have offsets from UTC, which you
can measure in minutes.  I'd also be OK with {old,new}OffsetMinutes, if you
wanted something more concise.

Also please add a comment indicating that these offsets are relative to UTC
(assuming that's true!) and that these offsets take daylight savings time into
account.

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>-hal::SwitchState GetCurrentSwitchState(hal::SwitchDevice aDevice)
>+SwitchState GetCurrentSwitchState(hal::SwitchDevice aDevice)

Missed a spot.  :)

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp
>@@ -637,48 +637,75 @@ AdjustSystemClock(int64_t aDeltaMillisec
>+int32_t
>+GetTimezoneOffset(bool aIgnoreDST)
>+{
>+  PRExplodedTime prTime;
>+  PR_ExplodeTime(PR_Now(), PR_LocalTimeParameters, &prTime);
>+
>+  int32_t offset = prTime.tm_params.tp_gmt_offset;
>+  if (!aIgnoreDST) {
>+    offset += prTime.tm_params.tp_dst_offset;
>+  }
>+
>+  return -(offset / 60);
> }

Can we get rid of the aIgnoreDST argument, since you always call this with |false|?

Also, please make this method static (or put it in an anonymous namespace),
because you don't want to export it out of this file.

Comment 17

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Notify Time Change for Internal Use, V2.2

Addressing comment 16. r=jlebar.

Thanks Justin for the review!

Comment 18

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Apply Cached NITZ whenever Auto-Update is Enabled, V1.2

Rebased. r=philikon.

Thanks Philikon for the review!

Comment 19

[Gene Lian (I already quit Mozilla)]

Try-server results:

"-b do -p all -u none -t none"
https://tbpl.mozilla.org/?tree=Try&rev=bb7f6aacb59f

"-b d -p linux64 -u all -t none"
https://tbpl.mozilla.org/?tree=Try&rev=0d7c2011e071

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Apply Cached NITZ whenever Auto-Update is Enabled, V1.3

Rebased for mozilla-inbound. r=philikon.

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19e5770f482a
https://hg.mozilla.org/integration/mozilla-inbound/rev/97474744374c

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19e5770f482a
https://hg.mozilla.org/mozilla-central/rev/97474744374c

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6bf509795bc8
https://hg.mozilla.org/releases/mozilla-aurora/rev/597e5d695e46