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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gal, Assigned: Gene Lian (I already quit Mozilla))

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
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.
I think comment 1 might have to do with a lot of the invalidation issues we see in the settings.  bug 796831
(Assignee)

Comment 3

5 years ago
(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.
Assignee: nobody → clian
Blocks: 714349, 789973
Depends on: 790499
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
(Assignee)

Updated

5 years ago
Blocks: 801838
(Assignee)

Comment 4

5 years ago
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|.
(Assignee)

Comment 5

5 years ago
Created attachment 672239 [details] [diff] [review]
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!
Attachment #672239 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 6

5 years ago
Created attachment 672241 [details] [diff] [review]
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!
Attachment #672241 - Flags: review?(philipp)
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"?
Attachment #672239 - Flags: review?(justin.lebar+bug)
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.
Attachment #672241 - Flags: review?(philipp) → review+
(Assignee)

Comment 9

5 years ago
(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. :)
> 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.
(Assignee)

Comment 11

5 years ago
(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. ;)
(Assignee)

Comment 12

5 years ago
Created attachment 673188 [details] [diff] [review]
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! :)
Attachment #672239 - Attachment is obsolete: true
Attachment #673188 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 13

5 years ago
Created attachment 673190 [details] [diff] [review]
Part 2, Apply Cached NITZ whenever Auto-Update is Enabled, V1.1

Addressing comment 8. r=philikon
Attachment #672241 - Attachment is obsolete: true
Attachment #673190 - Flags: review+
(Assignee)

Comment 14

5 years ago
Created attachment 673191 [details] [diff] [review]
Part 1, Notify Time Change for Internal Use, V2.1

Sorry. Fixing some building errors for try builds. Please comment 12. Thanks!
Attachment #673188 - Attachment is obsolete: true
Attachment #673188 - Flags: review?(justin.lebar+bug)
Attachment #673191 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 15

5 years ago
Change the title to a more descriptive one.
Summary: [settings] set time automatically should be enabled by default → [settings] Set-Automatically for time should take effect *immediately* instead of waiting on next NITZ coming
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.
Attachment #673191 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 674146 [details] [diff] [review]
Part 1, Notify Time Change for Internal Use, V2.2

Addressing comment 16. r=jlebar.

Thanks Justin for the review!
Attachment #673191 - Attachment is obsolete: true
Attachment #674146 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 674147 [details] [diff] [review]
Part 2, Apply Cached NITZ whenever Auto-Update is Enabled, V1.2

Rebased. r=philikon.

Thanks Philikon for the review!
Attachment #673190 - Attachment is obsolete: true
Attachment #674147 - Flags: review+
(Assignee)

Comment 19

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

Updated

5 years ago
Duplicate of this bug: 796265
(Assignee)

Updated

5 years ago
Duplicate of this bug: 798752
(Assignee)

Comment 22

5 years ago
Created attachment 674173 [details] [diff] [review]
Part 2, Apply Cached NITZ whenever Auto-Update is Enabled, V1.3

Rebased for mozilla-inbound. r=philikon.
Attachment #674147 - Attachment is obsolete: true
Attachment #674173 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e5770f482a
https://hg.mozilla.org/integration/mozilla-inbound/rev/97474744374c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/19e5770f482a
https://hg.mozilla.org/mozilla-central/rev/97474744374c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
status-firefox18: --- → affected
Whiteboard: [checkin-needed:aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/6bf509795bc8
https://hg.mozilla.org/releases/mozilla-aurora/rev/597e5d695e46
status-firefox18: affected → fixed
status-firefox19: --- → fixed
Whiteboard: [checkin-needed:aurora]
You need to log in before you can comment on or make changes to this bug.